From 062f302b4366c791ae70141d89549e48dbbfddd7 Mon Sep 17 00:00:00 2001 From: Shane B Date: Sun, 23 Aug 2026 17:07:12 +0200 Subject: [PATCH 1/5] Give a failure nobody can name a page that can be reported UnhandledProtocolError and anything reaching REASON_UNKNOWN had no screen of their own. They arrived at a toast saying to restart the app, which cannot help and asks somebody to repeat what just failed. The page carries the three things a report needs and a person otherwise has to hunt for: which build this is, which exporter made their bundle, and the failure verbatim. The last is why somebody would open an export zip - a file holding the private keys to their tags - to read a version line out of it. Shown only when the app cannot name the cause. Never for a rejected passcode, an account with no tags, a network that is down, or a session wanting a code: each of those has a screen saying what to do, and this one would be strictly worse than the advice they already give. A page that appears for ordinary mistakes is one people learn to dismiss, and then it is worth nothing on the day it is right. Get the log asks which of two things is wanted, because they are not the same: copy it for pasting into the issue form's log box, or save it through the document picker to attach. It cannot hand over a log the redactor refused - null means withhold, never fall back to the raw one, because an Apple ID on a public issue cannot be un-posted and this page is reached because something already broke. All ten locales' strings for this branch land here, including ones later commits use: they share ten files, and splitting them by hunk would risk a locale going missing for the sake of a tidier history. Co-Authored-By: Claude Opus 5 --- app/build.gradle.kts | 45 +++ .../wander/android/opentagviewer/Shot.java | 58 ++++ .../python/PythonPackagingTest.java | 29 ++ .../error/TheErrorPageIsReportableTest.java | 317 ++++++++++++++++++ .../error/WalkingThroughTheErrorPageTest.java | 188 +++++++++++ app/src/main/AndroidManifest.xml | 6 + .../opentagviewer/python/AppDependencies.java | 20 ++ .../python/ChaquopyLogRedactor.java | 44 +++ .../opentagviewer/python/LogRedactor.java | 48 +++ .../ui/error/ErrorReportActivity.java | 279 +++++++++++++++ .../opentagviewer/ui/error/IssueReport.java | 42 +++ .../opentagviewer/util/LogCollectorUtil.java | 53 +++ .../main/res/layout/activity_error_report.xml | 188 +++++++++++ app/src/main/res/values-de/strings.xml | 23 ++ app/src/main/res/values-en/strings.xml | 23 ++ app/src/main/res/values-fr/strings.xml | 23 ++ app/src/main/res/values-ja/strings.xml | 23 ++ app/src/main/res/values-ko/strings.xml | 23 ++ app/src/main/res/values-nl/strings.xml | 23 ++ app/src/main/res/values-ru/strings.xml | 23 ++ app/src/main/res/values-zh-rCN/strings.xml | 23 ++ app/src/main/res/values-zh-rTW/strings.xml | 23 ++ app/src/main/res/values/strings.xml | 23 ++ .../TheIssueLinkPointsSomewhereRealTest.java | 69 ++++ .../util/DescribingTheBuildInALogTest.java | 62 ++++ 25 files changed, 1678 insertions(+) create mode 100644 app/src/androidTest/java/dev/wander/android/opentagviewer/Shot.java create mode 100644 app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/TheErrorPageIsReportableTest.java create mode 100644 app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/WalkingThroughTheErrorPageTest.java create mode 100644 app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyLogRedactor.java create mode 100644 app/src/main/java/dev/wander/android/opentagviewer/python/LogRedactor.java create mode 100644 app/src/main/java/dev/wander/android/opentagviewer/ui/error/ErrorReportActivity.java create mode 100644 app/src/main/java/dev/wander/android/opentagviewer/ui/error/IssueReport.java create mode 100644 app/src/main/res/layout/activity_error_report.xml create mode 100644 app/src/test/java/dev/wander/android/opentagviewer/ui/error/TheIssueLinkPointsSomewhereRealTest.java create mode 100644 app/src/test/java/dev/wander/android/opentagviewer/util/DescribingTheBuildInALogTest.java diff --git a/app/build.gradle.kts b/app/build.gradle.kts index ffb7bf4a..3d7568a3 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -68,6 +68,26 @@ if (requestedAbis != null) { } } +/** + * The short commit this is being built from, or null when that cannot be known. + * + * **For log headers only.** It must never reach `versionName`: the app stamps + * `via: OpenTagViewer.android:` into every bundle it exports, and rule 9 is that + * nothing patches a version at build time, because two artifacts built from one commit would + * then disagree about what produced them. A build description is a different question from a + * product version, and only the first one wants a commit in it. + * + * Null rather than a guess when git is absent or this is not a checkout - a source zip off a + * release tag has no commit to name, and inventing one is worse than saying nothing. + */ +val gitCommit: String? by lazy { + runCatching { + providers.exec { + commandLine("git", "rev-parse", "--short", "HEAD") + }.standardOutput.asText.get().trim().ifEmpty { null } + }.getOrNull() +} + android { namespace = "dev.wander.android.opentagviewer" compileSdk = 35 @@ -79,6 +99,11 @@ android { versionCode = 3 versionName = "1.0.5" + // Null unless a build type sets it - see the debug block. A release is built from a tag + // and its versionName is exactly right, so there is nothing a commit would add; the + // field exists in both variants so code reading it compiles in both. + buildConfigField("String", "BUILD_COMMIT", "null") + testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" // **Do not add `timeout_msec` here.** It works - a hanging test fails at the cap with @@ -188,6 +213,17 @@ android { applicationIdSuffix = ".debug" versionNameSuffix = "-debug" + // **`-debug` says this is not a release; it does not say which build.** + // `versionName` is a committed literal, so every commit after 1.0.5 reports 1.0.5 + // perfectly confidently - and `build-debug.yml` publishes a debug APK artifact, so + // somebody can be running a build whose version string is months stale. The commit + // is the only thing that identifies such a build, which is exactly the distinction + // the exporter's `describe_build()` draws between a frozen download and a checkout. + buildConfigField( + "String", + "BUILD_COMMIT", + gitCommit?.let { "\"$it\"" } ?: "null") + // Distinct launcher name, otherwise a debug install sits next to a real one // with an identical icon and label and there is no way to tell them apart. manifestPlaceholders["appLabel"] = "OpenTagViewer (debug)" @@ -422,6 +458,15 @@ chaquopy { // desktop rather than reimplemented because what is displayed is what gets // agreed to, and two renderers would eventually show two different documents. "exporter/terms.py", + // Strips personal identifiers out of a log before anybody sends it. Pure stdlib - + // re, Counter, dataclass - and nothing else in exporter/. + // + // Shared for the same reason as terms.py, and more sharply: the wizard's Save + // logs button already runs these rules, and a second set in Java would mean two + // answers to "is my Apple ID in this file". The rules are patterns, so they need + // adding to as new identifiers turn up, and the one that gets forgotten is the + // copy nobody is looking at. + "exporter/redact.py", ) // The package's own test suite is not part of the app. It imports pytest, which is // not in the APK, so it is dead weight that would fail if anything ever touched it. diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/Shot.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/Shot.java new file mode 100644 index 00000000..7ec1b44d --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/Shot.java @@ -0,0 +1,58 @@ +package dev.wander.android.opentagviewer; + +import android.graphics.Bitmap; +import android.util.Log; + +import androidx.test.platform.app.InstrumentationRegistry; + +import java.io.File; +import java.io.FileOutputStream; + +/** + * A picture of whatever is on the screen, dialogs included. + * + *

Whole-screen, unlike the {@code view.draw(canvas)} pattern used elsewhere. That one + * needs the view to be in the activity's own hierarchy, and a dialog is not - it lives in its + * own window, so drawing the activity produces the page behind it with a hole where the dialog + * should be. {@code UiAutomation} photographs the compositor's output instead, which is what a + * person actually sees. + * + *

Writes into the directory AGP passes as {@code additionalTestOutputDir} and does nothing + * when there isn't one, so it is free in an ordinary run. Names are + * {@code -.png} so {@code .claude/skills/device-screenshots/sheet.py} groups + * them. + * + *

As ever: a screenshot is not an assertion. It explains why a failure looks wrong; it cannot + * fail. Assert the thing that matters as well. + */ +public final class Shot { + + private Shot() {} + + private static final String TAG = "Shot"; + + public static void ofTheScreen(final String name) { + final String dir = InstrumentationRegistry.getArguments() + .getString("additionalTestOutputDir"); + if (dir == null) { + return; + } + + try { + final Bitmap bitmap = + InstrumentationRegistry.getInstrumentation().getUiAutomation().takeScreenshot(); + if (bitmap == null) { + Log.w(TAG, "the screen could not be photographed for " + name); + return; + } + try (FileOutputStream out = + new FileOutputStream(new File(new File(dir), name + ".png"))) { + bitmap.compress(Bitmap.CompressFormat.PNG, 100, out); + } + bitmap.recycle(); + } catch (final Exception e) { + // A screenshot explains a failure; it is never the reason for one. + Log.w(TAG, "could not write " + name, e); + } + } +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java index 0f0a500f..b87e1d12 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java @@ -2,6 +2,7 @@ import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; @@ -165,6 +166,34 @@ public void theicloudPipelineIsPackagedAndImports() { } } + /** + * The log redactor imports, and still strips something. + * + *

The one whitelisted module whose absence would be silent and harmful. Everything + * else here fails loudly when it is missing - no sign-in, no import. This one is reached at + * the moment somebody is about to send their log to a public issue, and a caller that + * shrugged off the import error would hand over an unredacted one. So the app refuses to + * produce a log it could not clean, and this is what says the refusal will not be the normal + * case. + * + *

Asserted by running it rather than by importing it: {@code redact} is patterns and a + * {@code Counter}, so an import that resolved against something empty would still be an + * import. The rules themselves are the exporter's to test - {@code python/test/test_redact.py}, + * a different CI job - and duplicating them here would be the second copy this arrangement + * exists to avoid. + */ + @Test + public void theredactorIsPackagedAndRedacts() { + final PyObject redact = Python.getInstance().getModule("exporter.redact"); + assertNotNull("exporter.redact must be importable in the APK", redact); + + final PyObject result = redact.callAttr("redact", "signed in as someone@example.com"); + final String cleaned = result.asList().get(0).toString(); + + assertFalse("the address survived redaction: " + cleaned, + cleaned.contains("someone@example.com")); + } + /** * And it still knows who the exporter is, so the desktop side is unchanged. * diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/TheErrorPageIsReportableTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/TheErrorPageIsReportableTest.java new file mode 100644 index 00000000..ab49ae4e --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/TheErrorPageIsReportableTest.java @@ -0,0 +1,317 @@ +package dev.wander.android.opentagviewer.ui.error; + +import static androidx.test.espresso.Espresso.onView; +import static androidx.test.espresso.action.ViewActions.click; +import static androidx.test.espresso.action.ViewActions.scrollTo; +import static androidx.test.espresso.assertion.ViewAssertions.matches; +import static androidx.test.espresso.intent.Intents.intended; +import static androidx.test.espresso.intent.Intents.intending; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasAction; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasData; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasExtra; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasType; +import static androidx.test.espresso.matcher.RootMatchers.isDialog; +import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; +import static androidx.test.espresso.matcher.ViewMatchers.withId; +import static androidx.test.espresso.matcher.ViewMatchers.withText; +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.startsWith; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.hamcrest.MatcherAssert.assertThat; + +import android.app.Activity; +import android.content.ClipData; +import android.content.ClipboardManager; +import android.app.Instrumentation.ActivityResult; +import android.content.Intent; + +import androidx.lifecycle.Lifecycle.State; +import androidx.test.core.app.ActivityScenario; +import androidx.test.espresso.intent.Intents; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.LargeTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import dev.wander.android.opentagviewer.Eventually; +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.LogRedactor; + +/** + * The page somebody lands on when the app cannot say what went wrong. + * + *

Its whole job is to make a report possible, so what it hands over is the thing to test. + * Two questions decide whether it works: does the link carry the template that puts the questions + * in front of the reporter, and can an unredacted log ever leave the app. The second is the + * one with a permanent cost - an Apple ID posted to a public issue cannot be un-posted - and it + * only happens on the path where redaction fails, which is exactly the path a real device will not + * take on demand. + * + *

Hence a fake redactor. {@code AppDependencies.replaceLogRedactor} produces both the working + * case and the broken one; the broken one is not reachable otherwise, because it means Chaquopy + * failing to start. + */ +@LargeTest +@RunWith(AndroidJUnit4.class) +public class TheErrorPageIsReportableTest { + + private static final String A_CAUSE = "KeychainSessionError: No keychain keys are held"; + + /** Something a real logcat would carry and a report must not. */ + private static final String SOMETHING_PERSONAL = "someone@example.com"; + + /** Only the redactor's output carries this, so finding it proves which text went. + * Asserting the absence of the personal string alone would pass on an empty payload. */ + private static final String REDACTED_MARKER = " [cleaned by the redactor]"; + + private ActivityScenario scenario; + + @Before + public void catchTheIntents() { + Intents.init(); + + // **Only what leaves the app, named explicitly.** + // + // "anything that is not ACTION_MAIN" reads as the same thing and is not: the intent that + // launches the activity under test matches it too, so ActivityScenario.launch was + // answered by the stub, the activity never started, and the scenario waited for a RESUMED + // state that could not arrive. A hang rather than a failure, at 0 of 8 tests, with + // nothing in the output naming the cause. + // **Every intent the page can fire, or the real thing launches.** `intending` stubs + // only what it is named, and this class went on stubbing ACTION_CHOOSER after the share + // sheet became a document picker - so a real picker opened over the app and stayed there + // for whatever ran next. + intending(anyOf( + hasAction(Intent.ACTION_VIEW), + hasAction(Intent.ACTION_CREATE_DOCUMENT))) + .respondWith(new ActivityResult(Activity.RESULT_CANCELED, null)); + } + + @After + public void putTheRealOnesBack() { + if (this.scenario != null) { + this.scenario.close(); + } + Intents.release(); + AppDependencies.reset(); + } + + private void open() { + this.scenario = ActivityScenario.launch(ErrorReportActivity.intentFor( + getInstrumentation().getTargetContext(), A_CAUSE)); + } + + /** + * The report button opens the form, with the template that asks the questions. + * + *

Not {@code /issues/new}. GitHub applies a template's labels and questions from its front + * matter; a bare form gives the reporter a blank box and the maintainer an unlabelled issue. + * And GitHub does not error on a wrong template name - it silently serves the blank one - so + * nothing but an assertion notices. + */ + @Test + public void thereportButtonOpensTheTemplatedForm() { + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted(log, "nothing")); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_button)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_button)).perform(click()); + + intended(allOf( + hasAction(Intent.ACTION_VIEW), + hasData(hasToString(IssueReport.NEW_APP_BUG)))); + } + + /** + * And the cause is on screen verbatim, in the words the failure arrived in. + * + *

It is evidence, not prose: a maintainer searches for it, and a reporter pastes it. A + * translated or prettified cause is one nobody can match against a stack trace. + */ + @Test + public void thefailureIsShownAsItArrived() { + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted(log, "nothing")); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_cause)) + .check(matches(withText(A_CAUSE)))); + } + + /** + * Copying puts the redacted text on the clipboard, and not the raw log. + * + *

The fake stands in for {@code exporter.redact}, whose own rules are the exporter's to + * test. What is asserted here is the wiring - and it is asserted on the payload, + * which is the whole point. This test previously checked that a share sheet had opened and + * called itself "the shared log is the redacted one"; it would have stayed green while + * handing over an unredacted log, which is the one outcome that cannot be taken back. + */ + @Test + public void thecopiedLogIsTheRedactedOne() { + AppDependencies.replaceLogRedactor(log -> + new LogRedactor.Redacted( + log.replace(SOMETHING_PERSONAL, "") + REDACTED_MARKER, + "1 email address")); + this.open(); + + this.chooseFromTheLogMenu(R.string.error_report_log_copy); + + final CharSequence copied = theClipboard(); + assertNotNull("nothing was copied", copied); + assertThat(copied.toString(), containsString(REDACTED_MARKER)); + assertThat("the raw log reached the clipboard", + copied.toString(), not(containsString(SOMETHING_PERSONAL))); + } + + /** + * Saving asks the system for somewhere to put it, rather than choosing for the user. + * + *

A file the user picked the location of is one the browser's file picker can find again + * when they go to attach it, which is the entire reason this option exists. A cache file + * handed to a share sheet is not, and Drive and Files do not even appear as targets for one. + */ + @Test + public void thesaveOptionOpensTheDocumentPicker() { + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted(log, "nothing")); + this.open(); + + this.chooseFromTheLogMenu(R.string.error_report_log_save); + + intended(allOf( + hasAction(Intent.ACTION_CREATE_DOCUMENT), + hasType("text/plain"), + hasExtra(Intent.EXTRA_TITLE, "opentagviewer-log.txt"))); + } + + /** + * And there is a way off this page that is on the page. + * + *

The action bar is hidden here, so before Close existed the system back gesture was the + * only exit - no arrow, no X. Fine for anybody who knows that and a dead end for anybody who + * does not, on a screen somebody reaches at the moment they are already stuck. + */ + @Test + public void thereisAWayOutThatIsNotTheBackGesture() { + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted(log, "nothing")); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_close)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_close)).perform(scrollTo(), click()); + + Eventually.check(() -> assertEquals( + State.DESTROYED, this.scenario.getState())); + } + + /** + * Opens the two-way choice and picks one of them. + * + *

The wait between the two is not padding. A dialog animates in, and + * `animationsDisabled` does not stop it - AGP zeroes the window and transition scales and + * leaves `animator_duration_scale` alone, so clicking the instant the builder returns hits a + * row that is still scaling up and less than 90% visible. Espresso calls that a + * PerformException, which reads like the view being wrong rather than early. It passed on a + * fast device and failed on the managed one, which is the usual way round. + */ + private void chooseFromTheLogMenu(final int option) { + Eventually.check(() -> onView(withId(R.id.error_report_share_log)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_share_log)).perform(scrollTo(), click()); + + final String label = getInstrumentation().getTargetContext().getString(option); + Eventually.check(() -> onView(withText(label)).inRoot(isDialog()) + .check(matches(isDisplayed()))); + onView(withText(label)).inRoot(isDialog()).perform(click()); + } + + /** What is on the clipboard, read on the main thread as the framework requires. */ + private static CharSequence theClipboard() { + final CharSequence[] held = new CharSequence[1]; + getInstrumentation().runOnMainSync(() -> { + final ClipboardManager clipboard = getInstrumentation().getTargetContext() + .getSystemService(ClipboardManager.class); + final ClipData clip = clipboard == null ? null : clipboard.getPrimaryClip(); + held[0] = clip == null || clip.getItemCount() == 0 + ? null + : clip.getItemAt(0).getText(); + }); + return held[0]; + } + + /** + * And it says what came out, rather than asking to be trusted. + */ + @Test + public void itsaysWhatTheRedactorRemoved() { + AppDependencies.replaceLogRedactor(log -> + new LogRedactor.Redacted(log, "1 email address, 2 serial numbers")); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_log_note)) + .check(matches(withText(containsString("1 email address, 2 serial numbers"))))); + } + + /** + * A redactor that cannot run means no log at all - never the raw one. + * + *

The case this class exists for. This page is reached because something broke, and + * "Chaquopy did not start" is a candidate - so the redactor failing is not hypothetical, it is + * correlated with being here. Falling back to the unredacted log would put somebody's Apple ID + * on a public issue at the exact moment they are least inclined to read it first, and it + * cannot be taken back. + * + *

So the button is absent, and the page says why rather than leaving a dead control. + */ + @Test + public void arefusedRedactionOffersNoLogAtAll() { + AppDependencies.replaceLogRedactor(log -> null); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_log_note)) + .check(matches(isDisplayed()))); + + onView(withId(R.id.error_report_share_log)).check(matches(not(isDisplayed()))); + onView(withId(R.id.error_report_log_note)).check(matches( + withText(getInstrumentation().getTargetContext() + .getString(R.string.error_report_log_unavailable)))); + } + + /** + * And reporting still works without one, because a report with no log still helps. + */ + @Test + public void reportingIsStillOfferedWithoutALog() { + AppDependencies.replaceLogRedactor(log -> null); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_button)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_button)).perform(click()); + + intended(hasAction(Intent.ACTION_VIEW)); + } + + private static org.hamcrest.Matcher hasToString(final String expected) { + return new org.hamcrest.TypeSafeMatcher<>() { + @Override + protected boolean matchesSafely(final android.net.Uri uri) { + return expected.equals(uri.toString()); + } + + @Override + public void describeTo(final org.hamcrest.Description description) { + description.appendText("a Uri of " + expected); + } + }; + } +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/WalkingThroughTheErrorPageTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/WalkingThroughTheErrorPageTest.java new file mode 100644 index 00000000..08998171 --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/WalkingThroughTheErrorPageTest.java @@ -0,0 +1,188 @@ +package dev.wander.android.opentagviewer.ui.error; + +import static androidx.test.espresso.Espresso.onView; +import static androidx.test.espresso.action.ViewActions.click; +import static androidx.test.espresso.action.ViewActions.scrollTo; +import static androidx.test.espresso.assertion.ViewAssertions.matches; +import static androidx.test.espresso.intent.Intents.intended; +import static androidx.test.espresso.intent.Intents.intending; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasAction; +import static androidx.test.espresso.matcher.RootMatchers.isDialog; +import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; +import static androidx.test.espresso.matcher.ViewMatchers.withId; +import static androidx.test.espresso.matcher.ViewMatchers.withText; +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; + +import android.app.Activity; +import android.app.Instrumentation.ActivityResult; +import android.content.Intent; + +import androidx.test.core.app.ActivityScenario; +import androidx.test.espresso.intent.Intents; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.LargeTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import dev.wander.android.opentagviewer.Eventually; +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.Shot; +import dev.wander.android.opentagviewer.TestPace; +import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.LogRedactor; + +/** + * The error page as a person meets it, at a pace a person can follow. + * + *

Its sibling {@code TheErrorPageIsReportableTest} asserts; this one is for watching. + * Six separate assertions each open and close the screen, so run in slow motion they are six + * flickers rather than a journey. This walks the whole thing once, paced with {@link TestPace}, so + * {@code slowMotion=2000} shows what somebody actually sees. + * + *

It still asserts as it goes - a demo that could pass while showing the wrong screen is + * decoration - but the assertions are the ones a viewer is looking at anyway, and the two states + * it covers are the ones that matter: a log that can be shared, and a log that cannot. + * + *

See {@code AGENTS.md} under "Showing a UI test to a person" for how to run it on a device + * with a window. + */ +@LargeTest +@RunWith(AndroidJUnit4.class) +public class WalkingThroughTheErrorPageTest { + + private static final String A_CAUSE = + "KeychainSessionError: No keychain keys are held, so nothing can be decrypted."; + + /** What a real logcat would carry and a public issue must not. */ + private static final String AN_EMAIL = "someone@example.com"; + + private ActivityScenario scenario; + + @Before + public void catchWhatLeavesTheApp() { + Intents.init(); + // **Only what leaves the app, named explicitly.** + // + // "anything that is not ACTION_MAIN" reads as the same thing and is not: the intent that + // launches the activity under test matches it too, so ActivityScenario.launch was + // answered by the stub, the activity never started, and the scenario waited for a RESUMED + // state that could not arrive. A hang rather than a failure, at 0 of 8 tests, with + // nothing in the output naming the cause. + // CANCELED for the picker: RESULT_OK with no data would have the page treat a + // cancelled save as a successful one, which is the opposite of what a stub should model. + intending(hasAction(Intent.ACTION_VIEW)) + .respondWith(new ActivityResult(Activity.RESULT_OK, null)); + intending(hasAction(Intent.ACTION_CREATE_DOCUMENT)) + .respondWith(new ActivityResult(Activity.RESULT_CANCELED, null)); + } + + @After + public void putTheRealOnesBack() { + if (this.scenario != null) { + this.scenario.close(); + } + Intents.release(); + AppDependencies.reset(); + } + + /** + * The whole page, in the order somebody reads it. + * + *

Arrive on a failure nobody can act on, see what it was, see which build and which bundle + * it happened to, learn that the log is cleaned and what came out of it, share it, then open + * the report form. + */ + @Test + public void awholeReportFromAFailureNobodyCanAct0n() { + // Stands in for exporter.redact: takes the address out, and says it did. + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted( + log.replace(AN_EMAIL, ""), "1 email address, 2 device names")); + + this.scenario = ActivityScenario.launch(ErrorReportActivity.intentFor( + getInstrumentation().getTargetContext(), A_CAUSE)); + + // 1. It says plainly that this is a bug rather than something to retry. + Eventually.check(() -> onView(withId(R.id.error_report_title)) + .check(matches(isDisplayed()))); + TestPace.afterAStep(); + + // 2. And what actually failed, verbatim - the thing a maintainer searches for. + onView(withId(R.id.error_report_cause)).check(matches(withText(A_CAUSE))); + TestPace.afterAStep(); + + // 3. The two questions the report form opens with, already answered on screen. This is + // what stops somebody opening their export zip to read a version line out of it. + onView(withId(R.id.error_report_build)) + .check(matches(withText(containsString("OpenTagViewer app")))); + onView(withId(R.id.error_report_imported_from)).check(matches(isDisplayed())); + TestPace.afterAStep(); + + // 4. The log is offered only once it has been through the redactor, and the page says + // what came out rather than asking to be trusted. + Eventually.check(() -> onView(withId(R.id.error_report_share_log)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_log_note)) + .check(matches(withText(containsString("1 email address, 2 device names")))); + Shot.ofTheScreen("the_error_page-log_can_be_shared"); + TestPace.afterAStep(); + + // 5. And it asks which of the two things is wanted, because they are not the same + // thing: pasting into the form's log box, or producing a file to attach. This was a + // share sheet, which served neither - with text and no stream, Drive and Files do not + // appear as targets at all. + onView(withId(R.id.error_report_share_log)).perform(scrollTo(), click()); + onView(withText(getInstrumentation().getTargetContext() + .getString(R.string.error_report_log_save))) + .inRoot(isDialog()).check(matches(isDisplayed())); + Shot.ofTheScreen("the_error_page-how_do_you_want_the_log"); + TestPace.afterAStep(); + + // 6. Saving goes to the document picker, so the file lands where the user chose - which + // is the only place a browser's file picker can find it again. + onView(withText(getInstrumentation().getTargetContext() + .getString(R.string.error_report_log_save))).inRoot(isDialog()).perform(click()); + intended(hasAction(Intent.ACTION_CREATE_DOCUMENT)); + TestPace.afterAStep(); + + // 7. And the report button lands on the form with the questions already in it. + onView(withId(R.id.error_report_button)).perform(scrollTo(), click()); + intended(hasAction(Intent.ACTION_VIEW)); + TestPace.afterAStep(); + } + + /** + * And the same page when the log cannot be cleaned. + * + *

Worth watching rather than only asserting, because the correct behaviour is an + * absence - no share button - and an absence is the kind of thing that looks like a + * layout bug until you know it is deliberate. The page says why, and reporting still works. + */ + @Test + public void andwhatItLooksLikeWhenTheLogCannotBeCleaned() { + AppDependencies.replaceLogRedactor(log -> null); + + this.scenario = ActivityScenario.launch(ErrorReportActivity.intentFor( + getInstrumentation().getTargetContext(), A_CAUSE)); + + Eventually.check(() -> onView(withId(R.id.error_report_log_note)) + .check(matches(withText(getInstrumentation().getTargetContext() + .getString(R.string.error_report_log_unavailable))))); + TestPace.afterAStep(); + + // No button, rather than a button that hands over an unredacted log. + onView(withId(R.id.error_report_share_log)).check(matches(not(isDisplayed()))); + Shot.ofTheScreen("the_error_page-log_cannot_be_cleaned"); + TestPace.afterAStep(); + + // Reporting without a log still helps, so it is still offered. + onView(withId(R.id.error_report_button)).perform(scrollTo(), click()); + intended(hasAction(Intent.ACTION_VIEW)); + TestPace.afterAStep(); + } +} diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 76b9d8c0..255ad158 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -67,6 +67,12 @@ android:foregroundServiceType="location"> + + + Here for the usual reason and one sharper one: the screen that offers a log is the error + * page, which exists because something already broke. A test of it has to be able to + * produce a working redactor and one that cannot run, and the second is the case that decides + * whether an unredacted log can escape. + */ + private static LogRedactor logRedactor = new ChaquopyLogRedactor(); + /** * Turns coordinates into something a person recognises. * @@ -149,6 +159,10 @@ public static HardwareDescriber hardwareDescriber() { return hardwareDescriber; } + public static LogRedactor logRedactor() { + return logRedactor; + } + public static AnisetteServerTesterService serverTester(final CronetEngine engine) { return serverTesterFactory.apply(engine); } @@ -173,6 +187,11 @@ public static void replaceHardwareDescriber(final HardwareDescriber replacement) hardwareDescriber = replacement; } + @VisibleForTesting + public static void replaceLogRedactor(final LogRedactor replacement) { + logRedactor = replacement; + } + @VisibleForTesting public static void replaceAnisette(final Function replacement) { anisetteFactory = (context, settings, hasSession) -> replacement.apply(settings); @@ -185,6 +204,7 @@ public static void reset() { anisetteFactory = LocalAnisette::new; serverTesterFactory = AnisetteServerTesterService::new; hardwareDescriber = new ChaquopyHardwareDescriber(); + logRedactor = new ChaquopyLogRedactor(); icloudFactory = AppDependencies::openRealICloud; geocoderFactory = (context, locale) -> AddressLookup.through(new Geocoder(context, locale)); diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyLogRedactor.java b/app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyLogRedactor.java new file mode 100644 index 00000000..e2fbce00 --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyLogRedactor.java @@ -0,0 +1,44 @@ +package dev.wander.android.opentagviewer.python; + +import android.util.Log; + +import com.chaquo.python.PyObject; +import com.chaquo.python.Python; + +/** + * {@link LogRedactor} over {@code exporter.redact}, the same module the desktop wizard's + * Save logs button runs. + * + *

Blocking, and needs a started interpreter. Never call it on the main thread. + */ +public class ChaquopyLogRedactor implements LogRedactor { + private static final String TAG = ChaquopyLogRedactor.class.getSimpleName(); + + private static final String MODULE = "exporter.redact"; + + @Override + public Redacted redact(final String log) { + if (log == null) { + return null; + } + + try { + final PyObject module = Python.getInstance().getModule(MODULE); + + // redact() hands back (text, Counter); summarise() turns the second into a sentence. + final PyObject result = module.callAttr("redact", log); + final PyObject cleaned = result.asList().get(0); + final PyObject counts = result.asList().get(1); + + return new Redacted( + cleaned.toString(), + module.callAttr("summarise", counts).toString()); + } catch (final Exception e) { + // **Null, not the log.** The caller is about to hand this to somebody who will attach + // it to a public issue. A redactor that could not run is a reason to withhold the + // file, never a reason to send the unredacted one - see LogRedactor#redact. + Log.w(TAG, "Could not redact the log, so it will not be offered", e); + return null; + } + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/LogRedactor.java b/app/src/main/java/dev/wander/android/opentagviewer/python/LogRedactor.java new file mode 100644 index 00000000..b9aca2f5 --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/LogRedactor.java @@ -0,0 +1,48 @@ +package dev.wander.android.opentagviewer.python; + +import lombok.AllArgsConstructor; +import lombok.Getter; + +/** + * Takes the personal identifiers out of a log before anybody sends it somewhere public. + * + *

The rules live in Python and are shared with the desktop exporter - + * {@code exporter/redact.py}, whitelisted into the APK. Not ported to Java on purpose: the wizard's + * Save logs button already runs them, they are patterns that need adding to as new identifiers turn + * up, and two sets would mean two answers to "is my Apple ID in this file" with only one of them + * being maintained. + * + *

Behind an interface for the usual reason - the real one needs a running interpreter, so a + * screen that called it directly could not be tested without one. + */ +public interface LogRedactor { + + /** A cleaned log, and one line saying what came out of it. */ + @AllArgsConstructor + @Getter + class Redacted { + private final String text; + + /** + * What was removed, in words - "3 email addresses, 1 serial number". + * + *

Shown to the person about to send the file. It is the difference between trusting a + * claim that something was cleaned and being told what was found, and it costs nothing: + * the redactor counts as it goes. + */ + private final String summary; + } + + /** + * @return the redacted log, or null if it could not be redacted. + * + *

Null means do not send this. The caller's job is to withhold the log, not to fall + * back to the raw one - this runs at the moment somebody is about to attach a file to a public + * issue, and the failure mode of guessing wrong is their Apple ID on the internet + * permanently. Refusing is recoverable; the alternative is not. + * + *

It can genuinely fail: the error page that offers this exists because something + * broke, and "Python did not start" is one of the things that might have. + */ + Redacted redact(String log); +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/error/ErrorReportActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/error/ErrorReportActivity.java new file mode 100644 index 00000000..4cd7c65a --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/error/ErrorReportActivity.java @@ -0,0 +1,279 @@ +package dev.wander.android.opentagviewer.ui.error; + +import static android.view.View.GONE; +import static android.widget.Toast.LENGTH_LONG; + +import android.content.ClipData; +import android.content.ClipboardManager; +import android.content.Context; +import android.content.Intent; +import android.net.Uri; +import android.os.Build; +import android.os.Bundle; +import android.util.Log; +import android.widget.Button; +import android.widget.Toast; +import android.widget.TextView; + +import androidx.activity.result.ActivityResultLauncher; +import androidx.activity.result.contract.ActivityResultContracts; +import androidx.appcompat.app.AppCompatActivity; + +import com.google.android.material.dialog.MaterialAlertDialogBuilder; + +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.Writer; +import java.nio.charset.StandardCharsets; + +import dev.wander.android.opentagviewer.BuildConfig; +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.db.room.OpenTagViewerDatabase; +import dev.wander.android.opentagviewer.db.room.entity.Import; +import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.LogRedactor; +import dev.wander.android.opentagviewer.util.LogCollectorUtil; +import dev.wander.android.opentagviewer.util.android.WebLink; +import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers; +import io.reactivex.rxjava3.core.Observable; +import io.reactivex.rxjava3.schedulers.Schedulers; + +/** + * "This one is a bug" - the screen for a failure nobody here can fix. + * + *

Shown only when the app cannot name the cause. An {@code UnhandledProtocolError} from + * Python, or anything that reaches {@code REASON_UNKNOWN}. Never for a rejected passcode, an + * account with no tags, a network that is down, or a session wanting a verification code - each of + * those has a screen that says what to do, and this one would be worse than the advice they + * already give. A page that turns up for ordinary mistakes is one people learn to dismiss, and + * then it is worth nothing on the day it is right. + * + *

It carries the three things a report needs and a person otherwise has to hunt for: which + * build this is, which exporter made their bundle, and the failure verbatim. The last was the + * reason somebody would open an export zip - a file holding their tags' private keys - to read a + * version line out of it. + */ +public class ErrorReportActivity extends AppCompatActivity { + private static final String TAG = ErrorReportActivity.class.getSimpleName(); + + /** What went wrong, in the words the failure arrived in. Never translated - it is evidence. */ + public static final String EXTRA_CAUSE = "cause"; + + /** + * Which explanation to show above the cause. + * + *

Because "something came back that this app cannot read" is false for half the callers. + * It is exactly right for a protocol failure and wrong for a bundle that would not parse - + * nothing came back from anywhere, somebody chose a file. A page that misdescribes what + * happened is worse than a generic one, because the reader corrects for it and stops trusting + * the rest. + */ + public static final String EXTRA_BODY = "body"; + + /** The protocol case: Apple sent something the library does not understand. */ + public static Intent intentFor(final Context context, final String cause) { + return intentFor(context, cause, R.string.error_report_body); + } + + public static Intent intentFor( + final Context context, final String cause, final int bodyRes) { + return new Intent(context, ErrorReportActivity.class) + .putExtra(EXTRA_CAUSE, cause) + .putExtra(EXTRA_BODY, bodyRes); + } + + /** The redacted log, held once prepared so the share button is instant and cannot re-fail. */ + private LogRedactor.Redacted log; + + @Override + protected void onCreate(final Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + this.setContentView(R.layout.activity_error_report); + + if (this.getSupportActionBar() != null) { + this.getSupportActionBar().hide(); + } + + this.findViewById(R.id.error_report_build).setText( + "OpenTagViewer app " + LogCollectorUtil.describeBuild( + BuildConfig.VERSION_NAME, BuildConfig.BUILD_COMMIT)); + + this.findViewById(R.id.error_report_cause) + .setText(this.getIntent().getStringExtra(EXTRA_CAUSE)); + + this.findViewById(R.id.error_report_body).setText(this.getIntent() + .getIntExtra(EXTRA_BODY, R.string.error_report_body)); + + this.findViewById(R.id.error_report_button).setOnClickListener( + v -> WebLink.open(this, IssueReport.NEW_APP_BUG)); + + // finish() rather than a navigate-up: this page is always arrived at from somewhere, and + // that somewhere is where closing it should land - the map, mid-import, wherever it was. + this.findViewById(R.id.error_report_close).setOnClickListener(v -> this.finish()); + + // Nothing to share until the log has been through the redactor, and that is a Python call + // on a screen that exists because something already broke. + this.findViewById(R.id.error_report_share_log).setVisibility(GONE); + + this.prepareTheEvidence(); + } + + /** + * Reads the provenance and the log, off the main thread, and only then offers the log. + * + *

The share button is hidden until there is something safe to share. Redaction runs + * through Chaquopy, and this screen is reached because something failed - "Python did + * not start" being one of the candidates. A button that appeared regardless and then handed + * over a raw logcat would put somebody's Apple ID on a public issue at the moment they are + * least inclined to read it first. + */ + private void prepareTheEvidence() { + final TextView importedFrom = this.findViewById(R.id.error_report_imported_from); + final TextView note = this.findViewById(R.id.error_report_log_note); + final Button share = this.findViewById(R.id.error_report_share_log); + + var async = Observable.fromCallable(() -> { + final Import last = OpenTagViewerDatabase + .getInstance(this.getApplicationContext()).importDao().getMostRecent(); + + final String via = last == null ? null : last.exportedVia; + final String raw = LogCollectorUtil.getLastLogsWithHeader( + BuildConfig.VERSION_NAME, BuildConfig.BUILD_COMMIT, via); + + return new Evidence(via, AppDependencies.logRedactor().redact(raw)); + }) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe( + evidence -> { + importedFrom.setText(evidence.via == null + ? this.getString(R.string.imported_from_nothing) + : this.getString(R.string.imported_from_x, evidence.via)); + + if (evidence.log == null) { + note.setText(R.string.error_report_log_unavailable); + return; + } + + this.log = evidence.log; + note.setText(this.getString( + R.string.error_report_log_cleaned, evidence.log.getSummary())); + share.setVisibility(android.view.View.VISIBLE); + share.setOnClickListener(v -> this.offerTheLog()); + }, + error -> { + Log.w(TAG, "Could not prepare the log for reporting", error); + note.setText(R.string.error_report_log_unavailable); + }); + } + + /** + * Asks which of the two things somebody actually wants, because they are not the same thing. + * + *

It shipped as a share sheet, and a share sheet serves neither well. With text and + * no stream, Drive and Files do not appear as targets at all - so the file half of the sheet + * was simply absent, on a button whose main purpose is producing a file to attach. And the + * copy half depended on whichever clipboard target the phone happened to have. + * + *

Two named choices instead. Attaching a file to a GitHub issue on a phone goes through + * the browser's file picker, which reads storage - so a file has to exist somewhere the user + * chose, which is what the document picker is for. Pasting into the form's + * {@code render: shell} box just wants the clipboard. + */ + private void offerTheLog() { + new MaterialAlertDialogBuilder(this) + .setTitle(R.string.error_report_log_how) + .setItems( + new CharSequence[] { + this.getString(R.string.error_report_log_copy), + this.getString(R.string.error_report_log_save)}, + (dialog, which) -> { + if (which == 0) { + this.copyTheLog(); + } else { + this.saveTheLog(); + } + }) + .setNegativeButton(R.string.cancel, null) + .show(); + } + + /** Straight to the clipboard, for pasting into the form's log box. */ + private void copyTheLog() { + final ClipboardManager clipboard = this.getSystemService(ClipboardManager.class); + if (clipboard == null) { + Toast.makeText(this, R.string.failed_to_export_log_file, LENGTH_LONG).show(); + return; + } + + clipboard.setPrimaryClip(ClipData.newPlainText("OpenTagViewer log", this.log.getText())); + + // **Android 13 shows its own confirmation, and a toast on top of it reads as a bug.** + // Below that there is nothing at all, and silence after a tap is indistinguishable from + // a dead button. + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) { + Toast.makeText(this, R.string.error_report_log_copied, LENGTH_LONG).show(); + } + } + + /** + * Somewhere the user picks, through the document picker. + * + *

The same route as Settings' own Export Logs button, deliberately: a file the user chose + * the location of is one the browser's file picker can find again, which a cache file handed + * over by a share sheet is not. + */ + private void saveTheLog() { + this.saveLogLauncher.launch(new Intent(Intent.ACTION_CREATE_DOCUMENT) + .addCategory(Intent.CATEGORY_OPENABLE) + .setType("text/plain") + .putExtra(Intent.EXTRA_TITLE, "opentagviewer-log.txt")); + } + + /** + * Writes the redacted log where the picker said, off the main thread. + * + *

Registered as a field rather than made at click time: registering has to happen before + * the activity is started, so a launcher created inside a click listener throws. + */ + private final ActivityResultLauncher saveLogLauncher = this.registerForActivityResult( + new ActivityResultContracts.StartActivityForResult(), + result -> { + final Uri target = result.getData() == null ? null : result.getData().getData(); + if (result.getResultCode() != RESULT_OK || target == null || this.log == null) { + return; // cancelled, which is not a failure and needs no message + } + + var async = Observable.fromCallable(() -> { + try (OutputStream out = this.getContentResolver() + .openOutputStream(target); + Writer writer = new OutputStreamWriter( + out, StandardCharsets.UTF_8)) { + writer.write(this.log.getText()); + } + return target; + }) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe( + written -> Toast.makeText(this, + R.string.log_file_has_been_exported_successfully, + LENGTH_LONG).show(), + error -> { + Log.w(TAG, "Could not write the log out", error); + Toast.makeText(this, R.string.failed_to_export_log_file, + LENGTH_LONG).show(); + }); + }); + + /** What the background read produced, so the UI thread does one hand-off rather than two. */ + private static final class Evidence { + private final String via; + private final LogRedactor.Redacted log; + + private Evidence(final String via, final LogRedactor.Redacted log) { + this.via = via; + this.log = log; + } + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/error/IssueReport.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/error/IssueReport.java new file mode 100644 index 00000000..7cb1075d --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/error/IssueReport.java @@ -0,0 +1,42 @@ +package dev.wander.android.opentagviewer.ui.error; + +import lombok.AccessLevel; +import lombok.NoArgsConstructor; + +/** + * Where the app sends somebody who has hit a bug. + * + *

One constant, because nothing tests a link. The desktop exporter keeps its own as + * {@code GITHUB_ISSUES_LINK} in {@code exporter/version.py} for the same reason: a URL inlined at + * two call sites goes stale at one of them, and the symptom is a worse bug report months later + * with nothing to connect it to the change. + */ +@NoArgsConstructor(access = AccessLevel.PRIVATE) +public final class IssueReport { + + /** + * The template file {@link #NEW_APP_BUG} names, relative to {@code .github/ISSUE_TEMPLATE/}. + * + *

Named separately so a test can check it exists. GitHub does not error on an + * unknown {@code ?template=} - it quietly drops the reporter on a blank issue with none of the + * questions and none of the labels. So renaming the file breaks this with no error anywhere, + * and the only symptom is worse reports, indefinitely. + */ + public static final String TEMPLATE = "app-bug.yml"; + /** + * The issue form for app problems. + * + *

{@code ?template=} and not {@code ?labels=}. Labels in a URL are applied only for + * somebody with permission to label the repository, which a person reporting a bug is not - + * the template's own front matter applies them whoever files. The template is also what puts + * the questions in front of the reporter at all. + * + *

An unauthenticated visitor is redirected to a sign-in page and returned here afterwards, + * so the query survives the round trip. There is no way to file anonymously and GitHub has no + * social sign-in, which is why the screen says an account is needed rather than letting + * somebody discover it after writing everything out. + */ + public static final String NEW_APP_BUG = + "https://github.com/parawanderer/OpenTagViewer/issues/new?template=" + TEMPLATE; + +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/LogCollectorUtil.java b/app/src/main/java/dev/wander/android/opentagviewer/util/LogCollectorUtil.java index a97f6ad8..b1f67fd0 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/LogCollectorUtil.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/LogCollectorUtil.java @@ -29,4 +29,57 @@ public static String getLastLogs() { throw new RuntimeException(e); } } + + /** + * The log, with a few lines at each end saying what produced it. + * + *

Because the questions a bug report opens with are all answerable here. The issue + * template asks for the app version and for which exporter wrote the bundle, and both were + * things the log already knew and never said - so every report either guessed, or went and + * opened an export zip full of private keys to read one line out of it. + * + *

At both ends, which is not belt and braces. Five hundred lines is more than most + * people paste: somebody who has found the interesting part copies the tail around it, and a + * header is exactly the part that gets left behind. Repeating it costs three lines and means + * either end of an excerpt still says what it came from. + * + *

Kept to what identifies the build and the data, and nothing about the person: no account, + * no tag names, no identifiers. A header that leaked would be worse than none, because it + * arrives above content people have been told to read before posting, in the position they + * skim past as boilerplate. + * + * @param appVersion what the app calls itself - {@code BuildConfig.VERSION_NAME}. + * @param importedVia the {@code via:} of the most recent import, or null if nothing has been + * imported: an account-connected install genuinely has no bundle behind it, + * and saying so is an answer rather than a gap. + */ + /** + * What to call this build in a log, so a report says which one produced it. + * + *

{@code versionName} alone is not the answer on a checkout. It is a committed + * literal, so every commit after a release reports the old version perfectly confidently - + * and {@code build-debug.yml} publishes a debug APK artifact, so somebody can be running a + * build whose version string is months stale. {@code BUILD_COMMIT} is set for debug builds + * only and is what identifies those. + * + *

The same three cases the exporter's {@code describe_build()} distinguishes, for the same + * reason: a release is exactly what its version says, a checkout is its commit, and anything + * without one falls back to the version rather than inventing something. + */ + public static String describeBuild(final String appVersion, final String buildCommit) { + return buildCommit == null ? appVersion : appVersion + " (" + buildCommit + ")"; + } + + public static String getLastLogsWithHeader( + final String appVersion, final String buildCommit, final String importedVia) { + final String what = "OpenTagViewer app " + describeBuild(appVersion, buildCommit) + + " | tags imported from: " + + (importedVia == null ? "nothing - no bundle imported" : importedVia); + + return what + "\n" + + "The last " + NUM_LINES_UP + " lines of this device's log follow, " + + "unfiltered by the app.\n\n" + + getLastLogs() + + "\n" + what + "\n"; + } } diff --git a/app/src/main/res/layout/activity_error_report.xml b/app/src/main/res/layout/activity_error_report.xml new file mode 100644 index 00000000..dcad2eaa --- /dev/null +++ b/app/src/main/res/layout/activity_error_report.xml @@ -0,0 +1,188 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + +