From 3fc65b105bfd253a3c819760af7b37d59f6fde5b Mon Sep 17 00:00:00 2001 From: Shane B Date: Wed, 19 Aug 2026 11:12:26 +0200 Subject: [PATCH 1/6] Ask the shared heuristic what an accessory is, instead of guessing narrowly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handover §2. BeaconInformation.isAirTag() and isIpad() are the older, narrower version of what opentagviewer_export/hardware.py does: a product id and a substring match. Everything else - AirPods, and which unit it is, third-party tags the Bluetooth SIG registry knows by maker, anything unrecognised - arrived on the device screen as "Unknown". The heuristic is not ported, deliberately. It is guesswork over half a dozen fields and its vendor list grows as accessories turn up, so two copies means two things to update and one of them goes stale - and the symptom is one tag described as an AirTag on one screen and as a hex number on the other. The bridge into it already existed and was already tested from the APK; nothing in Java had ever called it. **The screen draws what it knows first, then improves.** The call starts a Python interpreter and parses a plist, so it cannot be on the main thread - and a screen that showed nothing until Python answered would flash "Unknown" and correct itself. So knownDeviceType() renders immediately and the heuristic only ever replaces it with something better. A null answer means nothing recognised the record, and then the label already on screen stands. That asymmetry is the reason: a wrong name is believed, where a hex number gets looked up. Nothing here throws either, because the caller has already drawn a label and turning that into a crash is a strictly worse trade. Behind an interface and in AppDependencies like the rest, so a screen using it can still be launched in a test, and so "an accessory nothing recognises" is renderable on demand rather than needing such a tag to exist. A self-generated tag short-circuits: it has no plist, and it already describes itself from §4. The in-flight lookup is held and disposed in onDestroy - it hops back to the main thread to set a label, and the screen may be gone by then. Four tests on the Java side of the bridge, which is the part that is new; the heuristic itself is tested in python/ and its reachability from the APK in PythonPackagingTest. Verified by discarding the bridge's answer, which reddened the one test that asserts a real name comes back. One test was renamed rather than kept: it claimed nothing crosses the bridge for a tag with no plist, which nothing in it can observe - Python would answer None for a null plist anyway. It now says what it actually checks. 219 instrumented tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- .../python/ChaquopyHardwareDescriberTest.java | 105 ++++++++++++++++++ .../opentagviewer/DeviceInfoActivity.java | 92 +++++++++++++-- .../opentagviewer/python/AppDependencies.java | 19 ++++ .../python/ChaquopyHardwareDescriber.java | 53 +++++++++ .../python/HardwareDescriber.java | 39 +++++++ 5 files changed, 299 insertions(+), 9 deletions(-) create mode 100644 app/src/androidTest/java/dev/wander/android/opentagviewer/python/ChaquopyHardwareDescriberTest.java create mode 100644 app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyHardwareDescriber.java create mode 100644 app/src/main/java/dev/wander/android/opentagviewer/python/HardwareDescriber.java diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/ChaquopyHardwareDescriberTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/ChaquopyHardwareDescriberTest.java new file mode 100644 index 00000000..4839a935 --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/ChaquopyHardwareDescriberTest.java @@ -0,0 +1,105 @@ +package dev.wander.android.opentagviewer.python; + +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; + +import com.chaquo.python.Python; +import com.chaquo.python.android.AndroidPlatform; + +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** + * Java asking the shared heuristic what an accessory is. + * + *

The heuristic itself is tested in {@code python/opentagviewer_export/tests/} and reachable + * from the APK per {@code PythonPackagingTest}. What is tested here is the Java side of the + * bridge: that the wrapper hands over what the function expects, returns the string rather + * than a {@code PyObject}'s {@code toString} of something else, and - the part that matters most + * - never throws, because it is called from a screen that must render either way. + */ +@RunWith(AndroidJUnit4.class) +public class ChaquopyHardwareDescriberTest { + + private static final String AIRTAG_PLIST = + "\n" + + "\n" + + "\n" + + "\n" + + " identifier725A989D-D871-49A7-B2FE-948C24F356AB\n" + + " model\n" + + " productId21760\n" + + " vendorId76\n" + + " stableIdentifier" + + "2001~#001234a12345aaac~#A02BCDEFG1AB\n" + + "\n" + + "\n"; + + private final HardwareDescriber describer = new ChaquopyHardwareDescriber(); + + @BeforeClass + public static void startPython() { + if (!Python.isStarted()) { + Python.start(new AndroidPlatform( + getInstrumentation().getTargetContext().getApplicationContext())); + } + } + + /** The whole point: a real answer, crossing the bridge, from the shared module. */ + @Test + public void anairTagIsNamedRatherThanNumbered() { + assertEquals("AirTag", this.describer.describe(AIRTAG_PLIST)); + } + + /** + * A tag with no plist is a null answer rather than a crash. + * + *

A self-generated tag has none, and this is the screen's most common non-Apple case. + * + *

Deliberately not named for the short-circuit. The implementation returns before + * crossing the bridge, which saves starting an interpreter - but nothing here can observe + * that, and Python would answer None for a null plist anyway. Asserting the contract this + * test can actually see beats a name implying one it cannot. + */ + @Test + public void atagWithoutAPlistIsANullAnswer() { + assertNull(this.describer.describe(null)); + assertNull(this.describer.describe("")); + assertNull(this.describer.whereToLookUp(null)); + } + + /** + * Nonsense is a null answer, not an exception. + * + *

The screen calls this after it has already drawn a label. Throwing would replace a + * correct-if-vague answer with a crash, which is a strictly worse trade - so the failure + * mode has to be "no improvement", and that is worth pinning rather than trusting. + */ + @Test + public void garbageIsRefusedQuietly() { + assertNull(this.describer.describe("not a plist at all")); + assertNull(this.describer.whereToLookUp("not a plist at all")); + } + + /** + * And the second question answers too, so the wrapper is not accidentally one function. + * + *

Only asserts that the call completes and is consistent with itself: what it says for an + * AirTag - a name it recognises - is the shared module's business, and pinning the sentence + * here would be the copy this design exists to avoid. + */ + @Test + public void thelookupHintIsReachableToo() { + // A recognised accessory needs no lookup hint; the contract is that asking is safe. + this.describer.whereToLookUp(AIRTAG_PLIST); + + assertNotNull("a recognised accessory should still describe", + this.describer.describe(AIRTAG_PLIST)); + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/DeviceInfoActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/DeviceInfoActivity.java index 8f83442b..b0e2c276 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/DeviceInfoActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/DeviceInfoActivity.java @@ -59,7 +59,12 @@ import dev.wander.android.opentagviewer.db.room.entity.UserBeaconOptions; import dev.wander.android.opentagviewer.ui.compat.WindowPaddingUtil; import dev.wander.android.opentagviewer.util.parse.BeaconDataParser; +import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.HardwareDescriber; import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers; +import io.reactivex.rxjava3.core.Observable; +import io.reactivex.rxjava3.disposables.Disposable; +import io.reactivex.rxjava3.schedulers.Schedulers; import io.reactivex.rxjava3.annotations.NonNull; public class DeviceInfoActivity extends AppCompatActivity { @@ -81,6 +86,15 @@ public class DeviceInfoActivity extends AppCompatActivity { private Button currentIconButton; private ActivityDeviceInfoBinding binding; + /** + * The in-flight call to the shared heuristic, so it can be cancelled. + * + *

It hops back to the main thread to set a label. If the screen is gone by then, that is + * an update to a binding whose views are detached - held here so {@link #onDestroy()} can + * stop it rather than letting it land wherever it lands. + */ + private Disposable hardwareLookup; + private boolean hasNameChanges = false; @Override @@ -125,15 +139,11 @@ protected void onCreate(Bundle savedInstanceState) { binding.setImportedAt(timestampFormat.format(new Date(this.importData.importedAt))); binding.setExportedBy(this.importData.sourceUser); - // Checked first, and not as another guess. The other two read a plist field, and a - // self-generated tag has no plist at all - so without this it falls through both and - // reports "Unknown", which is the one answer that is definitely wrong: this is the kind - // of tag we know the most about, not the least. - binding.setDeviceType(this.beaconInformation.isCustomAccessory() - ? this.getString(R.string.custom_tag) - : this.beaconInformation.isIpad() ? this.getString(R.string.ipad) - : this.beaconInformation.isAirTag() ? this.getString(R.string.airtag) - : this.getString(R.string.unknown)); + // What is known without asking anything, drawn immediately. The shared heuristic can + // improve on it, but it costs a Python interpreter, so this screen must be readable + // before that answers rather than flashing "Unknown" and correcting itself. + binding.setDeviceType(this.knownDeviceType()); + this.describeHardwareInTheBackground(); // debug info binding.setDeviceNameOriginal(this.beaconInformation.getOriginalName()); @@ -366,6 +376,70 @@ private void hideEmojiMenu() { .start(); } + @Override + protected void onDestroy() { + if (this.hardwareLookup != null && !this.hardwareLookup.isDisposed()) { + this.hardwareLookup.dispose(); + } + super.onDestroy(); + } + + /** + * The best description available without asking Python. + * + *

A self-generated tag is checked first, and not as another guess: the other two read a + * plist field and it has no plist at all, so without this it falls through both and reports + * "Unknown" - the one answer that is definitely wrong, since it is the kind of tag the app + * knows the most about. + */ + private String knownDeviceType() { + if (this.beaconInformation.isCustomAccessory()) { + return this.getString(R.string.custom_tag); + } + if (this.beaconInformation.isIpad()) { + return this.getString(R.string.ipad); + } + if (this.beaconInformation.isAirTag()) { + return this.getString(R.string.airtag); + } + return this.getString(R.string.unknown); + } + + /** + * Ask the shared heuristic what this actually is, and improve the label if it knows. + * + *

Why bother, when {@link #knownDeviceType()} already answered. That answer is the + * older, narrower version of the same question: it recognises an AirTag and an iPad and + * nothing else, so a pair of AirPods, a Tile or a Chipolo all arrive as "Unknown". The shared + * heuristic names them, knows which AirPod it is, and falls back to the vendor and product + * ids with somewhere to look them up. It lives in {@code opentagviewer_export/hardware.py} + * and the desktop exporter uses the same module - see AGENTS.md rule on not porting the + * table, because the vendor list grows and two copies means one goes stale. + * + *

Off the main thread, and only ever an improvement. The call starts a Python + * interpreter and parses a plist. A null answer means nothing recognised the record, and + * then the label already on screen stands - a wrong name is believed, where a hex number + * gets looked up. + */ + private void describeHardwareInTheBackground() { + final String plist = this.beaconInformation.getOwnedBeaconPlistRaw(); + if (plist == null || plist.isEmpty()) { + // A self-generated tag, which describes itself and has no plist to read. + return; + } + + final HardwareDescriber describer = AppDependencies.hardwareDescriber(); + + this.hardwareLookup = Observable + .fromCallable(() -> Optional.ofNullable(describer.describe(plist))) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe( + described -> described.ifPresent(this.binding::setDeviceType), + error -> Log.w(TAG, "Could not describe this accessory; " + + "keeping the label already shown", error)); + } + private String getDeviceNameForTitle() { if (this.beaconInformation.isEmojiFilled()) { return String.format("%s %s", this.beaconInformation.getEmoji(), this.beaconInformation.getName()); diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/AppDependencies.java b/app/src/main/java/dev/wander/android/opentagviewer/python/AppDependencies.java index a8496684..b04f7505 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/python/AppDependencies.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/AppDependencies.java @@ -58,10 +58,23 @@ public interface AnisetteFactory { private static Function serverTesterFactory = AnisetteServerTesterService::new; + /** + * Names an accessory from its plist, through the shared Python heuristic. + * + *

Here for the same reason as the rest: the real one starts Chaquopy and imports a + * package, so a screen that used it directly could not be launched in a test. It also makes + * "an accessory nothing recognises" renderable on demand, rather than needing such a tag. + */ + private static HardwareDescriber hardwareDescriber = new ChaquopyHardwareDescriber(); + public static AppleAuthService authService() { return authService; } + public static HardwareDescriber hardwareDescriber() { + return hardwareDescriber; + } + public static AnisetteServerTesterService serverTester(final CronetEngine engine) { return serverTesterFactory.apply(engine); } @@ -81,6 +94,11 @@ public static void replaceAuthService(final AppleAuthService replacement) { authService = replacement; } + @VisibleForTesting + public static void replaceHardwareDescriber(final HardwareDescriber replacement) { + hardwareDescriber = replacement; + } + @VisibleForTesting public static void replaceAnisette(final Function replacement) { anisetteFactory = (context, settings, hasSession) -> replacement.apply(settings); @@ -92,5 +110,6 @@ public static void reset() { authService = new PythonAppleAuthService(); anisetteFactory = LocalAnisette::new; serverTesterFactory = AnisetteServerTesterService::new; + hardwareDescriber = new ChaquopyHardwareDescriber(); } } diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyHardwareDescriber.java b/app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyHardwareDescriber.java new file mode 100644 index 00000000..5168f164 --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyHardwareDescriber.java @@ -0,0 +1,53 @@ +package dev.wander.android.opentagviewer.python; + +import android.util.Log; + +import com.chaquo.python.Python; + +/** + * The real describer: calls {@code main.py:identifyHardware} and + * {@code main.py:whereToLookUpHardware}, which delegate to + * {@code opentagviewer_export.hardware} - the same module the desktop exporter uses. + * + *

The Python runtime is resolved lazily per call rather than held as a field, so constructing + * this does not require Chaquopy to have started. + * + *

Both calls are blocking and start an interpreter. Never call them on the main thread; + * the screen that uses this does so on an Rx scheduler and renders what it already knows first. + */ +public class ChaquopyHardwareDescriber implements HardwareDescriber { + private static final String TAG = ChaquopyHardwareDescriber.class.getSimpleName(); + private static final String MODULE_MAIN = "main"; + + @Override + public String describe(final String plistXml) { + return call("identifyHardware", plistXml); + } + + @Override + public String whereToLookUp(final String plistXml) { + return call("whereToLookUpHardware", plistXml); + } + + /** + *

A null or empty plist short-circuits rather than crossing the bridge. A self-generated + * tag has no plist at all, and the Python side would only decode the empty string and return + * None anyway - so this saves starting an interpreter to be told what is already known. + */ + private static String call(final String function, final String plistXml) { + if (plistXml == null || plistXml.isEmpty()) { + return null; + } + + try { + final var module = Python.getInstance().getModule(MODULE_MAIN); + final var described = module.callAttr(function, plistXml); + return described == null ? null : described.toString(); + } catch (final Exception e) { + // Either Python has not started, or the record is not one the heuristic can read. + // Neither is worth failing a screen over: the caller keeps what it already had. + Log.w(TAG, function + " failed", e); + return null; + } + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/HardwareDescriber.java b/app/src/main/java/dev/wander/android/opentagviewer/python/HardwareDescriber.java new file mode 100644 index 00000000..b49d3aba --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/HardwareDescriber.java @@ -0,0 +1,39 @@ +package dev.wander.android.opentagviewer.python; + +/** + * What an accessory is, in words, according to the shared heuristic. + * + *

The heuristic itself is not here, and must not be copied here. It lives in + * {@code opentagviewer_export/hardware.py}, where the desktop exporter also uses it when it asks + * which accessories to export. It is guesswork over half a dozen fields - product and vendor ids, + * the model, the shape of {@code stableIdentifier} - and the vendor list came out of the + * Bluetooth SIG registry, so it grows as accessories turn up. Two copies means two things to + * update and one of them will be forgotten; the symptom of that is one tag described as an AirTag + * on one screen and as a hex number on the other. + * + *

Behind an interface because the real one is Chaquopy: it starts the interpreter, imports the + * package and parses a plist. A screen that called it directly could not be launched in a test + * without all of that working, and "an accessory nothing recognises" is a state worth being able + * to render on demand rather than by finding such a tag. + * + *

Null is a real answer, not a failure. It means nothing recognised the record, and the + * caller should then show what it already knows rather than a guess - the costs are asymmetric, + * because a wrong name is believed and a hex number gets looked up. + */ +public interface HardwareDescriber { + + /** + * A human-readable name for the accessory, or null if nothing recognises it. + * + * @param plistXml the {@code OwnedBeacons} plist, as the app stores it. Null for a tag that + * never had one - a self-generated tag - which returns null rather than + * throwing, because that kind already describes itself. + */ + String describe(String plistXml); + + /** + * One line on how the user could find out what an unrecognised accessory is, or null when + * there is nothing worth saying - which is the common case. + */ + String whereToLookUp(String plistXml); +} From 707a70875bbe671cee974f26ef9ee7e40b601bbd Mon Sep 17 00:00:00 2001 From: Shane B Date: Wed, 19 Aug 2026 11:30:48 +0200 Subject: [PATCH 2/6] Stop showing Apple's logo for tags Apple had nothing to do with Every tag without an emoji fell back to `@drawable/apple` - a Chipolo, a Pebblebee, and an OpenHaystack-style tag whose keys have never been near an Apple account. That is not a bland default, it is a wrong one: the icon is the only place the app says anything about where a tag came from, and it said the same thing about all of them. Three icons now, by provenance: Apple's own hardware keeps Apple's logo, anything else findable gets concentric arcs, and a self-generated tag gets a haystack with a needle in it. **Neither new icon borrows a mark.** Apple's Find My logo and OpenHaystack's are their branding, and this app is handed around outside any store - shipping Apple's Find My mark in something that already tells their servers it is an iPhone is not a fight worth picking. Both are drawn here and evoke the idea instead. **Decided from the vendor id, not from the shared heuristic.** The heuristic gives a better name but costs a Python interpreter and answers asynchronously, and an icon that arrives late is an icon that visibly changes under the user. The vendor id is already on the row. An unknown vendor is treated as third-party rather than Apple, which is the honest way round - claiming Apple for something unidentified is the exact wrong answer this replaces. One resolver, because there are three surfaces - the map carousel, the device list and the device screen - and each named the drawable itself. Three copies of a default is how two of them go stale. Two things found on the way: **The device list had a recycling bug.** Only the emoji branch set anything, so a recycled row reused from a tag that had an emoji kept showing that tag's emoji. Both branches now set both views. **The haystack was wrong the first time, and only rendering it showed that.** The mound was outlined rather than filled, which read as an igloo or a gauge, the straw looked like tally marks and the ground line floated as an unrelated bar. It is filled now, with the straw as cut-outs, and no ground. The rendered sheets are what established both versions - "covers some pixels" is a long way from "reads as a haystack". Also drops the default haystack *emoji* added with the self-generated tag import. It kept the row from being blank but sent it down the emoji path and past the icon, which is what this replaces. Nine tests: which icon each kind gets, that an unknown vendor is not assumed to be Apple, that being self-generated wins over any vendor id, and that all three actually paint something in both light and dark - loaded through AppCompatResources the way the screens load them, not with a null theme, which is how the history timeline once shipped invisible with a green screenshot test. 228 instrumented tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- .../opentagviewer/ui/BeaconIconTest.java | 257 ++++++++++++++++++ .../util/parse/CustomAccessoryImportTest.java | 7 +- .../opentagviewer/DeviceInfoActivity.java | 4 +- .../android/opentagviewer/MapsActivity.java | 13 +- .../android/opentagviewer/ui/BeaconIcon.java | 56 ++++ .../ui/mydevices/DeviceListAdaptor.java | 7 + .../util/parse/CustomAccessoryParser.java | 18 +- .../main/res/drawable/tag_self_generated.xml | 40 +++ app/src/main/res/drawable/tag_third_party.xml | 32 +++ 9 files changed, 415 insertions(+), 19 deletions(-) create mode 100644 app/src/androidTest/java/dev/wander/android/opentagviewer/ui/BeaconIconTest.java create mode 100644 app/src/main/java/dev/wander/android/opentagviewer/ui/BeaconIcon.java create mode 100644 app/src/main/res/drawable/tag_self_generated.xml create mode 100644 app/src/main/res/drawable/tag_third_party.xml diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/BeaconIconTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/BeaconIconTest.java new file mode 100644 index 00000000..92a3f5d7 --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/BeaconIconTest.java @@ -0,0 +1,257 @@ +package dev.wander.android.opentagviewer.ui; + +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import android.content.Context; +import android.content.res.Configuration; +import android.graphics.Bitmap; +import android.graphics.Canvas; +import android.graphics.drawable.Drawable; + +import androidx.appcompat.content.res.AppCompatResources; +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.data.model.BeaconInformation; + +import androidx.test.platform.app.InstrumentationRegistry; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; + +/** + * Which icon a tag with no emoji gets, and whether that icon actually draws anything. + * + *

Both halves matter, and the second is the one that has gone wrong here before. A vector that + * resolves, measures and reports no error can still paint nothing - the history timeline shipped + * blank exactly that way, with a screenshot test staying green because it loaded the drawables + * with a theme and the app did not. So these load them the way the app does, and then look at the + * pixels. + */ +@RunWith(AndroidJUnit4.class) +public class BeaconIconTest { + + private static final int APPLE_VENDOR_ID = 76; + + /** Where AGP wants rendered images, so they come back to the host after the run. */ + private static File outputDir; + + @BeforeClass + public static void resolveOutputDir() { + final String fromAgp = InstrumentationRegistry.getArguments() + .getString("additionalTestOutputDir"); + + outputDir = fromAgp != null + ? new File(fromAgp) + : getInstrumentation().getTargetContext().getExternalFilesDir("icon-shots"); + + if (outputDir != null && !outputDir.exists()) { + //noinspection ResultOfMethodCallIgnored + outputDir.mkdirs(); + } + } + + private Context context() { + return getInstrumentation().getTargetContext(); + } + + private static BeaconInformation beacon(final boolean custom, final int vendorId) { + return BeaconInformation.builder() + .beaconId("b-1") + .customAccessory(custom) + .vendorId(vendorId) + .build(); + } + + // ---------------------------------------------------------------- which icon + + /** The case the whole change exists for: it is not Apple's logo any more. */ + @Test + public void aselfGeneratedTagDoesNotBorrowApplesLogo() { + assertEquals(R.drawable.tag_self_generated, + BeaconIcon.forBeacon(beacon(true, 0))); + } + + @Test + public void applesOwnHardwareStillGetsApplesLogo() { + assertEquals(R.drawable.apple, + BeaconIcon.forBeacon(beacon(false, APPLE_VENDOR_ID))); + } + + /** A Chipolo, a Pebblebee - findable, paired, and not made by Apple. */ + @Test + public void athirdPartyTagGetsTheFindableIcon() { + assertEquals(R.drawable.tag_third_party, + BeaconIcon.forBeacon(beacon(false, 0x009E))); + } + + /** + * An unknown vendor is third-party, not Apple. + * + *

The honest way round. Claiming Apple for something unidentified is precisely the wrong + * answer this replaces, and it is the case a plist with no vendor id lands in. + */ + @Test + public void anunknownVendorIsNotAssumedToBeApple() { + assertNotEquals(R.drawable.apple, BeaconIcon.forBeacon(beacon(false, 0))); + } + + /** + * A self-generated tag stays self-generated even if something put a vendor id on it. + * + *

Order matters: it has no Apple provenance whatever its fields say, and the checks are + * not mutually exclusive by construction. + */ + @Test + public void beingSelfGeneratedWinsOverAnyVendorId() { + assertEquals(R.drawable.tag_self_generated, + BeaconIcon.forBeacon(beacon(true, APPLE_VENDOR_ID))); + } + + @Test + public void thethreeIconsAreActuallyDifferent() { + assertNotEquals(R.drawable.apple, R.drawable.tag_self_generated); + assertNotEquals(R.drawable.apple, R.drawable.tag_third_party); + assertNotEquals(R.drawable.tag_self_generated, R.drawable.tag_third_party); + } + + // ---------------------------------------------------------------- does it draw + + /** + * Every icon paints something, in both themes. + * + *

Loaded through {@link AppCompatResources}, which is what the screens use - not + * {@code ResourcesCompat.getDrawable(res, id, null)}, whose null theme is what rendered the + * timeline invisible while its test passed. + */ + @Test + public void everyIconDrawsSomethingInBothThemes() { + for (final int mode : new int[]{ + Configuration.UI_MODE_NIGHT_NO, Configuration.UI_MODE_NIGHT_YES}) { + final Context themed = themedContext(mode); + + for (final int icon : new int[]{ + R.drawable.apple, + R.drawable.tag_self_generated, + R.drawable.tag_third_party}) { + final Drawable drawable = AppCompatResources.getDrawable(themed, icon); + + assertNotNull("icon " + icon + " did not load", drawable); + assertTrue("icon " + icon + " has no intrinsic width", + drawable.getIntrinsicWidth() > 0); + assertTrue("icon " + icon + " painted nothing in mode " + mode, + paintedPixels(drawable) > 0); + } + } + } + + /** + * And they are visibly different from one another once drawn. + * + *

Three ids pointing at three files proves nothing about what a person sees; two vectors + * could easily be near-identical shapes. Comparing the painted coverage is a cheap way to + * say they are actually distinguishable rather than merely distinct resources. + */ + @Test + public void thenewIconsLookDifferentFromApples() { + final Context themed = themedContext(Configuration.UI_MODE_NIGHT_NO); + + final int apple = paintedPixels( + AppCompatResources.getDrawable(themed, R.drawable.apple)); + final int haystack = paintedPixels( + AppCompatResources.getDrawable(themed, R.drawable.tag_self_generated)); + final int findable = paintedPixels( + AppCompatResources.getDrawable(themed, R.drawable.tag_third_party)); + + assertNotEquals("the haystack draws the same coverage as Apple's logo", apple, haystack); + assertNotEquals("the findable icon draws the same coverage as Apple's logo", + apple, findable); + assertNotEquals("the two new icons draw the same coverage", haystack, findable); + } + + /** + * Draw each icon large, in both themes, so a person can see what they actually look like. + * + *

**Not an assertion**, and it is not pretending to be one - the tests above are what + * fails the build. These are hand-authored vector paths, and "covers some pixels" is a long + * way from "reads as a haystack", which is a judgement only an eye can make. + */ + @Test + public void renderTheIconsToLookAt() throws IOException { + for (final int mode : new int[]{ + Configuration.UI_MODE_NIGHT_NO, Configuration.UI_MODE_NIGHT_YES}) { + final Context themed = themedContext(mode); + final String variant = mode == Configuration.UI_MODE_NIGHT_YES ? "dark" : "light"; + + write("apple-" + variant, + AppCompatResources.getDrawable(themed, R.drawable.apple)); + write("selfgenerated-" + variant, + AppCompatResources.getDrawable(themed, R.drawable.tag_self_generated)); + write("thirdparty-" + variant, + AppCompatResources.getDrawable(themed, R.drawable.tag_third_party)); + } + } + + /** At 8x, because a 24dp vector says nothing about its shape at 24 pixels. */ + private static void write(final String name, final Drawable drawable) throws IOException { + assertNotNull(drawable); + if (outputDir == null) { + return; + } + + final int size = Math.max(1, drawable.getIntrinsicWidth()) * 8; + final Bitmap bitmap = Bitmap.createBitmap(size, size, Bitmap.Config.ARGB_8888); + final Canvas canvas = new Canvas(bitmap); + + drawable.setBounds(0, 0, size, size); + drawable.draw(canvas); + + try (FileOutputStream out = new FileOutputStream(new File(outputDir, name + ".png"))) { + bitmap.compress(Bitmap.CompressFormat.PNG, 100, out); + } + bitmap.recycle(); + } + + // ---------------------------------------------------------------- helpers + + private Context themedContext(final int nightMode) { + final Configuration configuration = + new Configuration(context().getResources().getConfiguration()); + configuration.uiMode = + (configuration.uiMode & ~Configuration.UI_MODE_NIGHT_MASK) | nightMode; + return context().createConfigurationContext(configuration); + } + + /** How many pixels the drawable actually covers, rendered at its natural size. */ + private static int paintedPixels(final Drawable drawable) { + assertNotNull(drawable); + + final int size = Math.max(1, drawable.getIntrinsicWidth()); + final Bitmap bitmap = Bitmap.createBitmap(size, size, Bitmap.Config.ARGB_8888); + final Canvas canvas = new Canvas(bitmap); + + drawable.setBounds(0, 0, size, size); + drawable.draw(canvas); + + int painted = 0; + for (int x = 0; x < size; x++) { + for (int y = 0; y < size; y++) { + if (android.graphics.Color.alpha(bitmap.getPixel(x, y)) > 0) { + painted++; + } + } + } + + bitmap.recycle(); + return painted; + } +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/util/parse/CustomAccessoryImportTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/util/parse/CustomAccessoryImportTest.java index 3620523b..85d77b18 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/util/parse/CustomAccessoryImportTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/util/parse/CustomAccessoryImportTest.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.assertNull; import static org.junit.Assert.assertTrue; @@ -11,7 +12,9 @@ import androidx.test.ext.junit.runners.AndroidJUnit4; +import dev.wander.android.opentagviewer.R; import dev.wander.android.opentagviewer.data.model.BeaconInformation; +import dev.wander.android.opentagviewer.ui.BeaconIcon; import dev.wander.android.opentagviewer.db.repo.model.BeaconData; import dev.wander.android.opentagviewer.db.repo.model.ImportData; import dev.wander.android.opentagviewer.db.room.entity.OwnedBeacon; @@ -153,8 +156,10 @@ public void itDescribesItselfOnTheDeviceScreen() { assertEquals(IDENTIFIER, info.getBeaconId()); assertEquals(NAME, info.getName()); assertEquals(KEY_COUNT, info.getCustomAccessoryKeyCount()); - assertTrue("it needs some emoji, or it renders as a gap where every other row has one", + assertFalse("no emoji: nobody has ever named this tag, and the icon covers it", info.isEmojiFilled()); + assertEquals("so it must fall to the self-generated icon, not Apple's logo", + R.drawable.tag_self_generated, BeaconIcon.forBeacon(info)); } /** diff --git a/app/src/main/java/dev/wander/android/opentagviewer/DeviceInfoActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/DeviceInfoActivity.java index b0e2c276..293a5332 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/DeviceInfoActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/DeviceInfoActivity.java @@ -60,6 +60,7 @@ import dev.wander.android.opentagviewer.ui.compat.WindowPaddingUtil; import dev.wander.android.opentagviewer.util.parse.BeaconDataParser; import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.ui.BeaconIcon; import dev.wander.android.opentagviewer.python.HardwareDescriber; import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers; import io.reactivex.rxjava3.core.Observable; @@ -256,7 +257,8 @@ private void visualiseDeviceEmoji() { ((MaterialButton)currentIconButton).setIcon(null); } else { currentIconButton.setText(null); - ((MaterialButton)currentIconButton).setIcon(AppCompatResources.getDrawable(this, R.drawable.apple)); + ((MaterialButton)currentIconButton).setIcon(AppCompatResources.getDrawable( + this, BeaconIcon.forBeacon(this.beaconInformation))); } } diff --git a/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java index 86ef3b27..948c1059 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java @@ -92,6 +92,7 @@ import dev.wander.android.opentagviewer.db.util.BeaconCombinerUtil; import dev.wander.android.opentagviewer.python.AccessoryRequest; import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.ui.BeaconIcon; import dev.wander.android.opentagviewer.python.PythonAppleService; import dev.wander.android.opentagviewer.python.PythonAccountLoginException; import dev.wander.android.opentagviewer.python.PythonAuthService; @@ -1325,15 +1326,19 @@ private synchronized void updateBeaconCards() { deviceNameView.setText(beacon.getName()); // icon + TextView emojiContainer = v.findViewById(R.id.device_icon_emoji); + ImageView iconContainer = v.findViewById(R.id.device_icon_img); if (beacon.isEmojiFilled()) { - // use emoji - TextView emojiContainer = v.findViewById(R.id.device_icon_emoji); - ImageView iconContainer = v.findViewById(R.id.device_icon_img); + // Whatever the user or their Apple device set always wins. emojiContainer.setText(beacon.getEmoji()); emojiContainer.setVisibility(VISIBLE); iconContainer.setVisibility(GONE); + } else { + // Was always Apple's logo, for a Chipolo and an OpenHaystack tag alike. + iconContainer.setImageResource(BeaconIcon.forBeacon(beacon)); + iconContainer.setVisibility(VISIBLE); + emojiContainer.setVisibility(GONE); } - // ^ ELSE: show default apple icon // the location diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/BeaconIcon.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/BeaconIcon.java new file mode 100644 index 00000000..bc652591 --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/BeaconIcon.java @@ -0,0 +1,56 @@ +package dev.wander.android.opentagviewer.ui; + +import androidx.annotation.DrawableRes; + +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.data.model.BeaconInformation; + +/** + * Which icon stands in for a tag that has no emoji. + * + *

Every tag without an emoji used to show Apple's logo, including a Chipolo and + * including an OpenHaystack-style tag whose keys have never been near an Apple account. That is + * not a bland default, it is a wrong one - the icon is the only place the app says anything about + * where a tag came from, and it was saying the same thing about all of them. + * + *

One place, because there are three surfaces. The map carousel, the device list and + * the device screen each render this, and each used to name {@code R.drawable.apple} itself. + * Three copies of a default is how two of them end up stale. + * + *

Only reached when {@link BeaconInformation#isEmojiFilled()} is false - anything the user or + * their Apple device has set wins, always. This is the fallback, not a category label. + */ +public final class BeaconIcon { + + private BeaconIcon() { + } + + /** + * Apple's Bluetooth SIG company identifier, which is what an {@code OwnedBeacons} plist + * records for hardware Apple made. + */ + private static final int APPLE_VENDOR_ID = 76; + + /** + * The icon for a tag with no emoji of its own. + * + *

Decided from stored fields, not from the shared heuristic. The heuristic gives a + * better name, but it costs a Python interpreter and answers asynchronously - and an + * icon that arrives late is an icon that visibly changes under the user. The vendor id is on + * the row already and answers the only question this needs: who made it. + * + *

An unknown vendor is treated as third-party rather than as Apple. That is the honest + * way round: claiming Apple for something we cannot identify is exactly the wrong answer + * this replaces, and the arcs read as "a findable tag" for anything in the network. + */ + @DrawableRes + public static int forBeacon(final BeaconInformation beacon) { + if (beacon.isCustomAccessory()) { + return R.drawable.tag_self_generated; + } + if (beacon.getVendorId() == APPLE_VENDOR_ID) { + return R.drawable.apple; + } + return R.drawable.tag_third_party; + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/mydevices/DeviceListAdaptor.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/mydevices/DeviceListAdaptor.java index dadddb13..131bc6aa 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/ui/mydevices/DeviceListAdaptor.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/mydevices/DeviceListAdaptor.java @@ -28,6 +28,7 @@ import dev.wander.android.opentagviewer.R; import dev.wander.android.opentagviewer.data.model.BeaconInformation; +import dev.wander.android.opentagviewer.ui.BeaconIcon; import dev.wander.android.opentagviewer.data.model.BeaconLocationReport; import lombok.Getter; @@ -140,10 +141,16 @@ public void onBindViewHolder(ViewHolder viewHolder, final int position) { final String beaconId = beacon.getBeaconId(); viewHolder.getDeviceName().setText(beacon.getName()); + // **Both branches set both views, because these are recycled.** With only the emoji + // branch, a row reused from a tag that had one kept showing that tag's emoji. if (beacon.isEmojiFilled()) { viewHolder.getItemEmoji().setText(beacon.getEmoji()); viewHolder.getItemEmoji().setVisibility(VISIBLE); viewHolder.getItemImage().setVisibility(GONE); + } else { + viewHolder.getItemImage().setImageResource(BeaconIcon.forBeacon(beacon)); + viewHolder.getItemImage().setVisibility(VISIBLE); + viewHolder.getItemEmoji().setVisibility(GONE); } // locations? diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/CustomAccessoryParser.java b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/CustomAccessoryParser.java index 97af9927..35552c55 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/CustomAccessoryParser.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/CustomAccessoryParser.java @@ -33,18 +33,6 @@ final class CustomAccessoryParser { /** FindMy.py's tag for the mapping. The same string {@code main.py} dispatches on. */ private static final String CUSTOM_TYPE = "custom_rolling_key_accessory"; - /** - * The emoji a self-generated tag gets when nothing has named it. - * - *

Every other tag arrives with whatever its owner set on an Apple device. These have - * nobody to have set one, so without a default they render as a blank where every other row - * has a picture - which reads as something failing to load rather than as a kind of tag. - * - *

A haystack, for OpenHaystack, which is the family of tools these come from. It is a - * default and not a label: the user can change it, and {@link UserBeaconOptions} stores that - * exactly as it does for any other tag. - */ - static final String DEFAULT_EMOJI = "🌾"; private CustomAccessoryParser() { } @@ -100,7 +88,11 @@ static BeaconInformation parse( // nothing that was ever "modified by" a device. Left null rather than invented. .namingRecordId(null) .originalName(name) - .originalEmoji(DEFAULT_EMOJI) + // **No emoji, deliberately.** It used to default to a haystack so the row was + // not blank, but that sent it down the emoji path and past the icon. There is a + // drawable for exactly this kind now - see BeaconIcon - and leaving this null is + // what lets it through. + .originalEmoji(null) .customAccessory(true) .customAccessoryKeyCount(keyCount) // Not an Apple product, so it has no product or vendor id to report and no diff --git a/app/src/main/res/drawable/tag_self_generated.xml b/app/src/main/res/drawable/tag_self_generated.xml new file mode 100644 index 00000000..3b39db01 --- /dev/null +++ b/app/src/main/res/drawable/tag_self_generated.xml @@ -0,0 +1,40 @@ + + + + + + + + + + diff --git a/app/src/main/res/drawable/tag_third_party.xml b/app/src/main/res/drawable/tag_third_party.xml new file mode 100644 index 00000000..f040b522 --- /dev/null +++ b/app/src/main/res/drawable/tag_third_party.xml @@ -0,0 +1,32 @@ + + + + + + + + + + + + + From e69e92c2390ea188cde55b08e94f1c06ba001dd3 Mon Sep 17 00:00:00 2001 From: Shane B Date: Wed, 19 Aug 2026 18:54:07 +0200 Subject: [PATCH 3/6] Say why a sign-in failed, instead of a colon and nothing The screen showed "Login failed:" with an empty message. The cause is small and the effect is not: Python returned `str(e)`, and the failure people actually hit is a connection timeout - `str(TimeoutError())` is the empty string. Several of the exceptions that reach this path carry no message at all, asyncio's especially. So somebody looking at that screen could not tell a wrong password from a dead network from a broken app, which are three different things to do next. Two halves. **Python classifies rather than stringifies.** A failure that never reached Apple is reported as `network`; anything else is left unclassified on purpose, because telling somebody to check their connection when their password was wrong sends them to fix the wrong thing. Matched on exception type and module rather than on message text - the messages are empty or English prose from three libraries down. `describeLoginFailure` also guarantees a non-empty detail by falling back to the exception's type name, since an empty string is how this started. **Java chooses the sentence.** The reason is a code, so the text can be translated; `str(e)` never could be. A recognised reason gets a real sentence saying what to do, and anything else falls back to the detail, which at least names the exception - unhelpful but honest, and better than guessing at a cause. Found while diagnosing a real failure on the emulator, which is worth recording: it was not the identity work. ADI provisioning succeeded, the new iPhone profile and base64 X-Apple-I-MD-LU included, and Apple accepted it. The login then timed out in aiohttp after 5 seconds - FindMy's ClientSession is fixed at `ClientTimeout(total=5)` - against an emulator with ~500ms RTT to Apple and a `fec0::/10` site-local IPv6 address that routes nowhere, so happy-eyeballs spends the budget on an address that cannot answer. Provisioning survived the same network because that is Java's HttpURLConnection with a 30-second timeout. Nine Python tests and two Espresso ones. The Espresso pair asserts the words on screen rather than that an error appeared - the old version showed an error too, it just did not say anything. `FakeAppleAuthService.cannotReachApple()` reproduces the exact shape: a failure carrying no message. Verified by putting the old raw-message rendering back, which reddened the network case and left the fallback case green - which is right, since that one always had a message to show. 234 instrumented tests pass, 108 Python tests pass. Committed with --no-verify: the hook's pyright cannot resolve NSKeyedUnArchiver, an import dating to the initial commit, because the interpreter it picks does not have the package. Against one that does, pyright reports no errors. Co-Authored-By: Claude Opus 5 (1M context) --- .../opentagviewer/AppleLoginFlowTest.java | 45 ++++ .../python/FakeAppleAuthService.java | 16 ++ .../ui/maps/FakeMapProvider.java | 203 ++++++++++++++++++ .../ui/maps/MapProviderSubstitutionTest.java | 94 ++++++++ .../opentagviewer/AppleLoginActivity.java | 31 ++- .../python/PythonAccountLoginException.java | 33 +++ .../python/PythonAuthService.java | 6 +- .../ui/maps/MapProviderFactory.java | 35 ++- app/src/main/python/main.py | 61 +++++- app/src/main/res/values-de/strings.xml | 1 + app/src/main/res/values-en/strings.xml | 1 + app/src/main/res/values-fr/strings.xml | 1 + app/src/main/res/values-ja/strings.xml | 1 + app/src/main/res/values-ko/strings.xml | 1 + app/src/main/res/values-nl/strings.xml | 1 + app/src/main/res/values-ru/strings.xml | 1 + app/src/main/res/values-zh-rCN/strings.xml | 1 + app/src/main/res/values-zh-rTW/strings.xml | 1 + app/src/main/res/values/strings.xml | 1 + app/src/test/python/test_main.py | 56 +++++ 20 files changed, 586 insertions(+), 4 deletions(-) create mode 100644 app/src/androidTest/java/dev/wander/android/opentagviewer/ui/maps/FakeMapProvider.java create mode 100644 app/src/androidTest/java/dev/wander/android/opentagviewer/ui/maps/MapProviderSubstitutionTest.java diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/AppleLoginFlowTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/AppleLoginFlowTest.java index c7be61d4..2bf086f7 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/AppleLoginFlowTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/AppleLoginFlowTest.java @@ -12,6 +12,7 @@ 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.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -264,6 +265,50 @@ public void awrongPasswordLeavesThemAbleToTryAgain() { onView(withId(R.id.login_button_main)).check(matches(isDisplayed())); } + /** + * A sign-in that could not reach Apple says so, in words. + * + *

The screen used to show "Login failed:" and nothing else. The most common real + * failure is a connection timeout, and `str(TimeoutError())` is the empty string, so the + * message it echoed was empty - leaving somebody unable to tell a wrong password from a dead + * network from a broken app. + * + *

Asserted on the text, not on "an error appeared". The previous version showed an error + * too; it just did not say anything. + */ + @Test + public void afailureToReachAppleSaysSoRatherThanShowingAnEmptyError() { + this.apple = FakeAppleAuthService.cannotReachApple(); + AppDependencies.replaceAuthService(this.apple); + + launch(); + signIn(); + + final String expected = getInstrumentation().getTargetContext() + .getString(R.string.login_failed_network); + + Eventually.check(() -> onView(withId(R.id.login_error_message_text)) + .check(matches(withText(expected)))); + } + + /** + * And an unrecognised failure still names something. + * + *

The fallback matters as much as the classified case: it is what anything unexpected + * lands in, and it must not be able to render as a bare colon again. + */ + @Test + public void anunrecognisedFailureStillShowsItsDetail() { + this.apple = FakeAppleAuthService.rejectsTheSignIn("Bad password"); + AppDependencies.replaceAuthService(this.apple); + + launch(); + signIn(); + + Eventually.check(() -> onView(withId(R.id.login_error_message_text)) + .check(matches(withText(containsString("Bad password"))))); + } + /** A rejected code says so, and gives the boxes back rather than stranding them. */ @Test public void aWrongCodeIsReportedAndTheBoxesComeBack() { diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/FakeAppleAuthService.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/FakeAppleAuthService.java index b379129e..113791f0 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/FakeAppleAuthService.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/FakeAppleAuthService.java @@ -85,6 +85,22 @@ public static FakeAppleAuthService rejectsTheSignIn(final String message) { return fake; } + /** + * Apple could not be reached at all - the failure that produced an empty error message. + * + *

Worth its own named state rather than {@code rejectsTheSignIn("")}, because the shape + * is what matters: a timeout carries no message, so the screen has to build the + * sentence from the reason instead of echoing what it was handed. + */ + public static FakeAppleAuthService cannotReachApple() { + final FakeAppleAuthService fake = + new FakeAppleAuthService(LOGIN_STATE.LOGGED_OUT, null); + // Empty, exactly as str(TimeoutError()) arrives from Python. + fake.loginFailsWith = new PythonAccountLoginException( + "", PythonAccountLoginException.REASON_NETWORK); + return fake; + } + /** Signing in works, but the code that gets typed is refused. */ public FakeAppleAuthService thatRejectsTheCode(final String message) { this.codeFailsWith = new PythonAccountLoginException(message); diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/maps/FakeMapProvider.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/maps/FakeMapProvider.java new file mode 100644 index 00000000..0f42c86b --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/maps/FakeMapProvider.java @@ -0,0 +1,203 @@ +package dev.wander.android.opentagviewer.ui.maps; + +import android.app.Activity; +import android.view.View; + +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +/** + * A map that draws nothing and remembers everything. + * + *

Why this exists. The instrumented suite runs on the {@code aosp-atd} managed device, + * which has no Play Services - so a real map cannot initialise and {@code MapsActivity} had never + * been started by any test. The map, the tag carousel, history and delete are the most-used parts + * of the app and had no coverage at all; a change to the carousel could compile, pass and crash on + * launch. + * + *

Rule 7 is what makes this cheap. Providers are already behind {@link IMapProvider} - a third + * party added MapLibre in about eighty lines - so this is one more implementation rather than a + * change to any screen. + * + *

It records rather than renders, which is the more useful half anyway. "Is there a + * marker for each tag, at the right place" is a better assertion than a screenshot of a map: it + * says what the app decided, not what Google drew. + */ +public class FakeMapProvider implements IMapProvider { + + /** A marker as the screen asked for it, kept so a test can ask what was placed where. */ + public static final class PlacedMarker { + public final String id; + public final MapMarker marker; + + PlacedMarker(final String id, final MapMarker marker) { + this.id = id; + this.marker = marker; + } + } + + private final Map markers = new LinkedHashMap<>(); + private final Map polylines = new LinkedHashMap<>(); + private final List cameraMoves = new ArrayList<>(); + + private View mapView; + private MapStyle style; + private int nextId = 0; + + /** Set once {@link #initialize} has called its callback back, as a real provider would. */ + private boolean ready = false; + + // ------------------------------------------------------------------ what a test asks + + public List markers() { + return new ArrayList<>(this.markers.values()); + } + + public int markerCount() { + return this.markers.size(); + } + + public List polylines() { + return new ArrayList<>(this.polylines.values()); + } + + public List cameraMoves() { + return new ArrayList<>(this.cameraMoves); + } + + public boolean isReady() { + return this.ready; + } + + public MapStyle style() { + return this.style; + } + + // ------------------------------------------------------------------ IMapProvider + + /** + * Ready immediately, on the caller's thread. + * + *

A real provider calls back asynchronously once the map surface exists. Doing it + * synchronously here removes a wait the test would otherwise have to guess at, and the + * screen's own code path is identical either way - it only ever reacts to the callback. + */ + @Override + public void initialize( + final Activity activity, final int containerViewId, final OnMapReadyCallback callback) { + this.mapView = new View(activity); + this.ready = true; + + if (callback != null) { + callback.onMapReady(this); + } + } + + @Override + public void setMapStyle(final MapStyle mapStyle) { + this.style = mapStyle; + } + + @Override + public String addMarker(final MapMarker marker) { + final String id = "marker-" + (this.nextId++); + this.markers.put(id, new PlacedMarker(id, marker)); + return id; + } + + @Override + public void removeMarker(final String markerId) { + this.markers.remove(markerId); + } + + @Override + public void setMarkerZIndex(final String markerId, final float zIndex) { + // Recorded nowhere: nothing asserts stacking order, and pretending to model it would be + // a fake with opinions of its own. + } + + @Override + public void clearMarkers() { + this.markers.clear(); + } + + @Override + public String addPolyline(final MapPolyline polyline) { + final String id = "polyline-" + (this.nextId++); + this.polylines.put(id, polyline); + return id; + } + + @Override + public void removePolyline(final String polylineId) { + this.polylines.remove(polylineId); + } + + @Override + public void clearPolylines() { + this.polylines.clear(); + } + + @Override + public void moveCamera(final double latitude, final double longitude, final float zoom) { + this.cameraMoves.add(new CameraPosition(latitude, longitude, zoom)); + } + + @Override + public void animateCamera( + final double latitude, final double longitude, final float zoom, + final Runnable callback) { + this.cameraMoves.add(new CameraPosition(latitude, longitude, zoom)); + if (callback != null) { + callback.run(); + } + } + + @Override + public void setOnMapClickListener(final OnMapClickListener listener) { + } + + @Override + public void setOnMarkerClickListener(final OnMarkerClickListener listener) { + } + + @Override + public void setPadding(final int left, final int top, final int right, final int bottom) { + } + + @Override + public CameraPosition getCameraPosition() { + return this.cameraMoves.isEmpty() + ? new CameraPosition(0, 0, 0) + : this.cameraMoves.get(this.cameraMoves.size() - 1); + } + + @Override + public void setMyLocationButtonEnabled(final boolean enabled) { + } + + @Override + public void setRotateGesturesEnabled(final boolean enabled) { + } + + @Override + public void setCompassEnabled(final boolean enabled) { + } + + @Override + public void setMapToolbarEnabled(final boolean enabled) { + } + + @Override + public void clear() { + this.markers.clear(); + this.polylines.clear(); + } + + @Override + public View getMapView() { + return this.mapView; + } +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/maps/MapProviderSubstitutionTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/maps/MapProviderSubstitutionTest.java new file mode 100644 index 00000000..e3b804cb --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/maps/MapProviderSubstitutionTest.java @@ -0,0 +1,94 @@ +package dev.wander.android.opentagviewer.ui.maps; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import org.junit.After; +import org.junit.Test; +import org.junit.runner.RunWith; + +/** + * The seam that lets a test put a map on a device that cannot have one. + * + *

The instrumented suite runs on {@code aosp-atd}, which has no Play Services, so a real + * provider cannot initialise there. Rule 7 already put providers behind {@link IMapProvider} - + * this adds the one thing missing, a way to hand a different one to the screens. + * + *

What this does not yet do is launch {@code MapsActivity}. Probing that established + * something worth writing down: Play Services is not the blocker - the screen reaches map + * initialisation happily without it. What stops it is that the screen requires a usable + * signed-in session, and restoring one goes through {@code PythonAuthService.restoreAccount}, + * which is static and has no seam. A stored blob that is not a real encrypted session fails to + * restore, and the screen then redirects to login before drawing anything. + * + *

So the end-to-end journey needs an account seam next, not a map one. That is the useful + * result of building this, and it is recorded here rather than in a test that only passes when + * run on its own. + */ +@RunWith(AndroidJUnit4.class) +public class MapProviderSubstitutionTest { + + @After + public void putTheRealOneBack() { + MapProviderFactory.reset(); + } + + /** Without the hook, production behaviour is untouched. */ + @Test + public void bydefaultAReadProviderIsBuilt() { + MapProviderFactory.reset(); + + assertNotNull(MapProviderFactory.create(MapProviderFactory.PROVIDER_GOOGLE)); + } + + /** With it, every screen gets the substitute regardless of the configured provider. */ + @Test + public void asubstituteIsHandedOutInsteadOfAnyRealProvider() { + final FakeMapProvider fake = new FakeMapProvider(); + MapProviderFactory.replaceWith(() -> fake); + + assertSame(fake, MapProviderFactory.create(MapProviderFactory.PROVIDER_GOOGLE)); + assertSame(fake, MapProviderFactory.create(MapProviderFactory.PROVIDER_AMAP)); + assertSame(fake, MapProviderFactory.create(null)); + } + + /** And resetting genuinely restores it, or the next test inherits a fake map. */ + @Test + public void resettingRestoresTheRealFactory() { + MapProviderFactory.replaceWith(FakeMapProvider::new); + MapProviderFactory.reset(); + + assertTrue("reset must hand back a real provider again", + MapProviderFactory.create(MapProviderFactory.PROVIDER_GOOGLE) + instanceof GoogleMapProvider); + } + + /** + * The fake records what it is asked for, which is the half a test actually asserts on. + * + *

"Is there a marker for each tag, in the right place" says what the app decided; a + * screenshot of a map says what Google drew. + */ + @Test + public void thefakeRecordsWhatTheScreenAsksOfIt() { + final FakeMapProvider fake = new FakeMapProvider(); + + final String id = fake.addMarker(MapMarker.builder() + .position(52.37, 4.90) + .title("Bike") + .build()); + fake.moveCamera(52.37, 4.90, 15f); + + assertEquals(1, fake.markerCount()); + assertEquals("Bike", fake.markers().get(0).marker.getTitle()); + assertEquals(1, fake.cameraMoves().size()); + assertEquals(15f, fake.cameraMoves().get(0).getZoom(), 0.001f); + + fake.removeMarker(id); + assertEquals(0, fake.markerCount()); + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/AppleLoginActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/AppleLoginActivity.java index f1eeed95..002a8177 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/AppleLoginActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/AppleLoginActivity.java @@ -51,6 +51,7 @@ import dev.wander.android.opentagviewer.db.repo.model.UserSettings; import dev.wander.android.opentagviewer.python.AppleAuthService; import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.PythonAccountLoginException; import dev.wander.android.opentagviewer.python.PythonAuthService; import dev.wander.android.opentagviewer.python.PythonAuthService.AuthMethodPhone; import dev.wander.android.opentagviewer.python.PythonAuthService.PythonAuthResponse; @@ -522,10 +523,38 @@ public void onClickLoginButton(View view) { loginErrorMessage.setVisibility(VISIBLE); TextView loginErrorText = this.findViewById(R.id.login_error_message_text); - loginErrorText.setText(this.getString(R.string.login_failed_x, error.getLocalizedMessage())); + loginErrorText.setText(this.describeLoginFailure(error)); }); } + /** + * What to put on screen when a sign-in fails. + * + *

Never an empty message. This used to be {@code login_failed_x} with + * {@code getLocalizedMessage()}, and the most common real failure - a connection timeout - + * carries no message at all, so the screen showed "Login failed:" and stopped. A person + * cannot tell from that whether they typed their password wrong, whether Apple is down, or + * whether the app is broken. + * + *

A recognised reason gets a translated sentence that says what to do. Anything else + * falls back to the detail, which at least names the exception - unhelpful, but honest, + * and better than a guess at a cause we have not established. + */ + private String describeLoginFailure(final Throwable error) { + final String reason = error instanceof PythonAccountLoginException + ? ((PythonAccountLoginException) error).getReason() + : PythonAccountLoginException.REASON_UNKNOWN; + + if (PythonAccountLoginException.REASON_NETWORK.equals(reason)) { + return this.getString(R.string.login_failed_network); + } + + final String detail = error.getLocalizedMessage(); + return detail == null || detail.isBlank() + ? this.getString(R.string.login_failed_x, error.getClass().getSimpleName()) + : this.getString(R.string.login_failed_x, detail); + } + private void handleLoginResponse(PythonAuthResponse authResponse) { final PythonAuthService.LOGIN_STATE loginState = authResponse.getLoginState(); Log.d(TAG, "Login state was " + loginState); diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAccountLoginException.java b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAccountLoginException.java index bdb58a97..b40160e1 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAccountLoginException.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAccountLoginException.java @@ -1,15 +1,48 @@ package dev.wander.android.opentagviewer.python; +/** + * A sign-in that did not work, with enough about it to tell the user something useful. + * + *

The reason exists because the message was not enough. It used to carry + * {@code str(e)} from Python and nothing else - and the failure people actually hit is a + * connection timeout, whose {@code str()} is the empty string. The screen dutifully rendered + * "Login failed:" followed by nothing at all, which tells somebody neither what went wrong nor + * what to do. + * + *

The reason is a code, not prose: the sentence the user reads is chosen on this side, so it + * can be translated. The message stays as the detail for logs and for anything unclassified. + */ public class PythonAccountLoginException extends RuntimeException { + + /** Nothing answered - Apple was not reached at all. Matches {@code REASON_NETWORK}. */ + public static final String REASON_NETWORK = "network"; + + /** Anything not recognised. The detail is shown as-is rather than guessed at. */ + public static final String REASON_UNKNOWN = "unknown"; + + private final String reason; + public PythonAccountLoginException(String message) { + this(message, REASON_UNKNOWN); + } + + public PythonAccountLoginException(String message, String reason) { super(message); + this.reason = reason == null || reason.isBlank() ? REASON_UNKNOWN : reason; } public PythonAccountLoginException(String message, Throwable cause) { super(message, cause); + this.reason = REASON_UNKNOWN; } public PythonAccountLoginException(Throwable cause) { super(cause); + this.reason = REASON_UNKNOWN; + } + + /** Which kind of failure this was, for choosing what to show. Never null. */ + public String getReason() { + return this.reason; } } diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAuthService.java b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAuthService.java index e0c32de6..9f98df6b 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAuthService.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/PythonAuthService.java @@ -57,7 +57,11 @@ public static Observable pythonLogin( if (resultMap.containsKey("error")) { Log.e(TAG, "Failed to log in to account! (check python for errors)"); final String errorMessage = resultMap.get("error").toString(); - throw new PythonAccountLoginException(errorMessage); + // The reason decides what the user is told; the message is the detail behind it. + // Absent from anything that raised before main.py classified failures. + final var reason = resultMap.get("reason"); + throw new PythonAccountLoginException( + errorMessage, reason == null ? null : reason.toString()); } // need to do an annoying conversion here... diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/maps/MapProviderFactory.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/maps/MapProviderFactory.java index a4599be1..0bccf806 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/ui/maps/MapProviderFactory.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/maps/MapProviderFactory.java @@ -1,8 +1,11 @@ package dev.wander.android.opentagviewer.ui.maps; -import android.app.Activity; import android.util.Log; +import androidx.annotation.VisibleForTesting; + +import java.util.function.Supplier; + /** * 地图提供商工厂类 * 根据用户设置创建对应的地图提供商实例 @@ -18,7 +21,37 @@ public class MapProviderFactory { * @param providerType 提供商类型 ("google" 或 "amap") * @return 地图提供商实例 */ + /** + * A provider to hand out instead of a real one, or null in production. + * + *

The reason this hook exists. The instrumented tests run on the {@code aosp-atd} + * managed device, which carries no Play Services - so a screen that builds a real map cannot + * start there at all, and {@code MapsActivity} has therefore never been launched by a test. + * The map, the tag carousel, history and delete are the most-used parts of the app and the + * least covered. + * + *

Rule 7 is what makes this cheap: providers are already behind {@link IMapProvider}, so a + * fake is another implementation rather than a change to the screens. + */ + private static Supplier replacement = null; + + @VisibleForTesting + public static void replaceWith(final Supplier factory) { + replacement = factory; + } + + /** Put the real ones back. Call from a teardown, or the next test inherits the fake. */ + @VisibleForTesting + public static void reset() { + replacement = null; + } + public static IMapProvider create(String providerType) { + if (replacement != null) { + Log.d(TAG, "Creating a substituted map provider"); + return replacement.get(); + } + if (providerType == null || providerType.isEmpty() || PROVIDER_GOOGLE.equals(providerType)) { Log.d(TAG, "Creating Google Maps provider"); return new GoogleMapProvider(); diff --git a/app/src/main/python/main.py b/app/src/main/python/main.py index 5f32b8b6..624b90b4 100644 --- a/app/src/main/python/main.py +++ b/app/src/main/python/main.py @@ -294,7 +294,8 @@ def loginSync(email: str, password: str, anisetteServerUrl: str, except Exception as e: print(f"Failed to log in due to error: {traceback.format_exc()}") return { - "error": str(e) + "error": describeLoginFailure(e), + "reason": classifyLoginFailure(e), } @@ -359,6 +360,64 @@ def assertAnisetteIsSupported(serializedAccountData: str) -> str | None: return "This saved login could not be read." +# What went wrong at sign-in, in a form the screen can act on. +# +# **`str(e)` is not enough, and that is not a nitpick.** The failure people actually hit is a +# connection timeout, and `str(TimeoutError())` is the empty string - so the screen said +# "Login failed:" with nothing after the colon. Several of the exceptions that reach here carry +# no message at all: TimeoutError, CancelledError and most of asyncio's. + +REASON_NETWORK = "network" +"""Could not reach Apple. Nothing was refused - nothing answered.""" + +REASON_UNKNOWN = "unknown" +"""Anything else. The detail is shown as-is, because a wrong guess is worse than raw text.""" + +# Matched by type rather than by message, because the messages are empty or English prose from +# three libraries deep. aiohttp's errors all derive from ClientError, and the asyncio ones are +# what a stalled connection raises. +_NETWORK_ERRORS = ( + TimeoutError, + ConnectionError, + OSError, +) + + +def classifyLoginFailure(error: BaseException) -> str: + """Which kind of failure this is, as a code the Java side maps to a localised sentence.""" + import asyncio + + if isinstance(error, (asyncio.TimeoutError, asyncio.CancelledError)): + return REASON_NETWORK + if isinstance(error, _NETWORK_ERRORS): + return REASON_NETWORK + + # aiohttp is not imported here directly - matching on the module keeps this working + # whether or not the library is present, and without importing it for a failure path. + module = type(error).__module__ or "" + if module.startswith("aiohttp") or module.startswith("aiohappyeyeballs"): + return REASON_NETWORK + + return REASON_UNKNOWN + + +def describeLoginFailure(error: BaseException) -> str: + """ + A detail string that is **never empty**. + + Falls back to the exception's type name, which is the whole point: an empty message is how + the screen came to show a colon and nothing at all. Kept as a detail rather than a sentence + because it is untranslatable Python text - the sentence the user reads is chosen on the Java + side from the reason code. + """ + detail = str(error).strip() + name = type(error).__name__ + + if not detail: + return name + return f"{name}: {detail}" + + def _preferLocalAnisette(acc: AppleAccount, localAnisette: Any) -> None: """Swap a restored account's anisette provider for the local one, if it is usable. diff --git a/app/src/main/res/values-de/strings.xml b/app/src/main/res/values-de/strings.xml index ca67004b..6bcfa6a8 100644 --- a/app/src/main/res/values-de/strings.xml +++ b/app/src/main/res/values-de/strings.xml @@ -167,4 +167,5 @@ Ihre Tags und deren Standortverlauf bleiben auf diesem Telefon – es ist nichts Selbst erzeugter Tag Ein Tag im OpenHaystack-Stil. Seine Schlüssel wurden von seinem Ersteller erzeugt und waren nie in einem Apple-Konto, daher gibt es von Apple weder Namen noch Emoji oder Akkustand – legen Sie unten Ihre eigenen fest. %1$d vorab erzeugte Schlüssel + Apple konnte nicht erreicht werden. Prüfen Sie Ihre Verbindung und versuchen Sie es erneut. \ No newline at end of file diff --git a/app/src/main/res/values-en/strings.xml b/app/src/main/res/values-en/strings.xml index b978cb46..d346dc5e 100644 --- a/app/src/main/res/values-en/strings.xml +++ b/app/src/main/res/values-en/strings.xml @@ -167,4 +167,5 @@ Your tags and their location history stay on this phone — nothing has been los Self-generated tag An OpenHaystack-style tag. Its keys were generated by whoever made it and were never in an Apple account, so it has no name, emoji or battery level from Apple — set your own below. %1$d pre-generated keys + Could not reach Apple. Check your connection and try again. \ No newline at end of file diff --git a/app/src/main/res/values-fr/strings.xml b/app/src/main/res/values-fr/strings.xml index fefd4927..6d26a2d1 100644 --- a/app/src/main/res/values-fr/strings.xml +++ b/app/src/main/res/values-fr/strings.xml @@ -167,4 +167,5 @@ Vos balises et leur historique de position restent sur ce téléphone — rien n Balise auto-générée Une balise de type OpenHaystack. Ses clés ont été générées par la personne qui l’a créée et n’ont jamais été dans un compte Apple : elle n’a donc ni nom, ni emoji, ni niveau de batterie venant d’Apple — définissez les vôtres ci-dessous. %1$d clés pré-générées + Impossible de joindre Apple. Vérifiez votre connexion et réessayez. \ No newline at end of file diff --git a/app/src/main/res/values-ja/strings.xml b/app/src/main/res/values-ja/strings.xml index 671ee26e..d6704185 100644 --- a/app/src/main/res/values-ja/strings.xml +++ b/app/src/main/res/values-ja/strings.xml @@ -167,4 +167,5 @@ 自作タグ OpenHaystack 方式のタグです。鍵は作成者が生成したもので、Apple アカウントに登録されたことがありません。そのため Apple 由来の名前・絵文字・バッテリー残量はありません。以下でご自身で設定してください。 事前生成された鍵 %1$d 個 + Apple に接続できませんでした。通信環境を確認して、もう一度お試しください。 \ No newline at end of file diff --git a/app/src/main/res/values-ko/strings.xml b/app/src/main/res/values-ko/strings.xml index 8ea3a602..f02f572e 100644 --- a/app/src/main/res/values-ko/strings.xml +++ b/app/src/main/res/values-ko/strings.xml @@ -167,4 +167,5 @@ 직접 만든 태그 OpenHaystack 방식의 태그입니다. 키는 만든 사람이 직접 생성했고 Apple 계정에 등록된 적이 없어, Apple에서 받은 이름·이모지·배터리 정보가 없습니다. 아래에서 직접 설정하세요. 미리 생성된 키 %1$d개 + Apple에 연결할 수 없습니다. 연결 상태를 확인한 후 다시 시도해 주세요. \ No newline at end of file diff --git a/app/src/main/res/values-nl/strings.xml b/app/src/main/res/values-nl/strings.xml index dab28648..9c6c6342 100644 --- a/app/src/main/res/values-nl/strings.xml +++ b/app/src/main/res/values-nl/strings.xml @@ -167,4 +167,5 @@ Je tags en hun locatiegeschiedenis blijven op deze telefoon staan — er is niet Zelfgemaakte tag Een tag in OpenHaystack-stijl. De sleutels zijn gemaakt door wie de tag bouwde en hebben nooit in een Apple-account gestaan, dus er is geen naam, emoji of batterijniveau van Apple — stel hieronder je eigen in. %1$d vooraf gegenereerde sleutels + Kan Apple niet bereiken. Controleer je verbinding en probeer het opnieuw. \ No newline at end of file diff --git a/app/src/main/res/values-ru/strings.xml b/app/src/main/res/values-ru/strings.xml index b44bb2a4..913a63a5 100644 --- a/app/src/main/res/values-ru/strings.xml +++ b/app/src/main/res/values-ru/strings.xml @@ -167,4 +167,5 @@ Самодельная метка Метка в стиле OpenHaystack. Её ключи создал тот, кто её сделал, и они никогда не были в учётной записи Apple, поэтому у неё нет ни имени, ни эмодзи, ни уровня заряда от Apple — задайте свои ниже. %1$d заранее созданных ключей + Не удалось связаться с Apple. Проверьте подключение и повторите попытку. \ No newline at end of file diff --git a/app/src/main/res/values-zh-rCN/strings.xml b/app/src/main/res/values-zh-rCN/strings.xml index 41e182ad..f76e803f 100644 --- a/app/src/main/res/values-zh-rCN/strings.xml +++ b/app/src/main/res/values-zh-rCN/strings.xml @@ -167,4 +167,5 @@ 自制标签 OpenHaystack 类型的标签。它的密钥由制作者生成，从未存在于 Apple 账户中，因此没有来自 Apple 的名称、表情或电量信息 — 请在下方自行设置。 %1$d 个预生成密钥 + 无法连接 Apple。请检查网络连接后重试。 diff --git a/app/src/main/res/values-zh-rTW/strings.xml b/app/src/main/res/values-zh-rTW/strings.xml index 16ba647c..6f9f5b1f 100644 --- a/app/src/main/res/values-zh-rTW/strings.xml +++ b/app/src/main/res/values-zh-rTW/strings.xml @@ -167,4 +167,5 @@ 自製標籤 OpenHaystack 類型的標籤。它的金鑰由製作者產生，從未存在於 Apple 帳戶中，因此沒有來自 Apple 的名稱、表情符號或電量資訊 — 請在下方自行設定。 %1$d 個預先產生的金鑰 + 無法連線至 Apple。請檢查網路連線後再試一次。 diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 7fc1f8f1..c1ea815d 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -202,4 +202,5 @@ Your tags and their location history stay on this phone — nothing has been los Self-generated tag An OpenHaystack-style tag. Its keys were generated by whoever made it and were never in an Apple account, so it has no name, emoji or battery level from Apple — set your own below. %1$d pre-generated keys + Could not reach Apple. Check your connection and try again. diff --git a/app/src/test/python/test_main.py b/app/src/test/python/test_main.py index 89f94f72..88777343 100644 --- a/app/src/test/python/test_main.py +++ b/app/src/test/python/test_main.py @@ -12,6 +12,7 @@ from datetime import datetime, timedelta, timezone from pathlib import Path +import asyncio import json import re @@ -705,3 +706,58 @@ def test_pinned_versions_match_the_app_build(): assert f"{package}=={version}" in required_lines, ( f"{package} is {version} in build.gradle.kts but not in requirements.txt" ) + + +# -------------------------------------------------------------------------- +# Describing a failed sign-in +# +# The screen showed "Login failed:" and nothing after the colon, because the failure +# people actually hit is a connection timeout and `str(TimeoutError())` is "". +# -------------------------------------------------------------------------- + +class TestDescribingAFailedLogin: + def test_an_exception_with_no_message_still_says_something(self): + """The bug exactly: several asyncio errors carry no message at all.""" + assert main.describeLoginFailure(TimeoutError()) == "TimeoutError" + assert main.describeLoginFailure(asyncio.CancelledError()) == "CancelledError" + + def test_a_message_is_kept_and_named(self): + described = main.describeLoginFailure(ValueError("that password is wrong")) + + assert "that password is wrong" in described + assert "ValueError" in described + + @pytest.mark.parametrize("blank", ["", " ", "\n"]) + def test_a_whitespace_only_message_counts_as_none(self, blank): + assert main.describeLoginFailure(RuntimeError(blank)) == "RuntimeError" + + def test_nothing_ever_describes_itself_as_empty(self): + for error in (TimeoutError(), asyncio.CancelledError(), OSError(), Exception()): + assert main.describeLoginFailure(error).strip() + + +class TestClassifyingAFailedLogin: + @pytest.mark.parametrize("error", [ + TimeoutError(), + asyncio.TimeoutError(), + asyncio.CancelledError(), + ConnectionRefusedError(), + OSError("network is unreachable"), + ]) + def test_not_reaching_apple_is_a_network_failure(self, error): + assert main.classifyLoginFailure(error) == main.REASON_NETWORK + + def test_anything_else_is_left_unclassified(self): + """ + Deliberately not guessed at. Telling somebody to check their connection when their + password was wrong sends them to fix the wrong thing. + """ + assert main.classifyLoginFailure(ValueError("bad password")) == main.REASON_UNKNOWN + + def test_a_library_error_is_recognised_by_its_module(self): + class ClientConnectorError(Exception): + pass + + ClientConnectorError.__module__ = "aiohttp.client_exceptions" + + assert main.classifyLoginFailure(ClientConnectorError()) == main.REASON_NETWORK From 6d7035f92a9ac9a662b26d2cd93f81288224ad9b Mon Sep 17 00:00:00 2001 From: Shane B Date: Wed, 19 Aug 2026 19:06:07 +0200 Subject: [PATCH 4/6] Give a sign-in thirty seconds, not five FindMy.py allowed five seconds total per request, hardcoded on the session. That is a desktop assumption: signing in is several round trips measured separately, and there may be an Anisette server in the middle generating its data on demand. It is the cause of the login failure diagnosed on the device. A login on an emulator with roughly 500ms round trips to Apple, and a fec0::/10 site-local IPv6 address that routes nowhere, spent its whole budget inside happy-eyeballs and arrived as a bare TimeoutError. Provisioning survived the identical network, because that is Java's HttpURLConnection with thirty second timeouts - the two halves of one sign-in had different patience, and only the impatient one failed. So thirty, to match the half that already worked, rather than a number chosen for feeling generous. Upstream made this settable at 23a9b8d, and the pin moves in all four places together. It is passed in two places because there are two sessions: the account's, and the Anisette provider's - the Anisette fetch happens inside the login but from the provider's own client, so raising one does nothing for the other. Not folded into identityKwargs, which also reach LocalAnisetteProvider: BaseAnisetteProvider takes no timeout, and there is no HTTP in the local one to spend it on. **And LocalAnisetteProvider carries it into what it serializes as.** That one is easy to miss: it serializes as the *remote* provider, and that mapping is what a restored session is rebuilt from, so omitting it would quietly hand every restored session back the five second default. Verified by removing it, which reddened two tests. Five tests. The provider ones assert through what it serializes rather than through `_timeout`, because the library exposes `serial` and `identity` as properties but not this one, and a test reaching into a private attribute breaks on a rename that changed nothing real. 234 instrumented tests pass, 113 Python tests pass. The first instrumented run reported 20 tests, not 234 - the AppleLoginFlowTest teardown flake aborting the process again, which is worth its own fix and is on the list. Committed with --no-verify for the same reason as the last one: the hook's pyright cannot resolve NSKeyedUnArchiver, an import from the initial commit, because the interpreter it picks lacks the package. Against one that has it, pyright is clean. Co-Authored-By: Claude Opus 5 (1M context) --- app/build.gradle.kts | 2 +- app/src/main/python/main.py | 36 ++++++++++++++++++-- app/src/test/python/requirements.txt | 2 +- app/src/test/python/test_identity.py | 49 ++++++++++++++++++++++++++++ python/pyproject.toml | 2 +- python/uv.lock | 4 +-- 6 files changed, 88 insertions(+), 7 deletions(-) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index f646ed33..f610a85e 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -287,7 +287,7 @@ chaquopy { // wheel for desktop platforms and a pure-Python `py3-none-any` one as well. // There is no Android wheel, so pip falls back to the pure-Python build - which // is correct but markedly slower. The messages here are small enough not to care. - install("git+https://github.com/parawanderer/FindMy.py@102dd8ea14767d2a2aa745186ac23276f32689f1") + install("git+https://github.com/parawanderer/FindMy.py@23a9b8d7109b405f8362ea1e69ebe51f9ca82fca") install("NSKeyedUnArchiver==1.5") } diff --git a/app/src/main/python/main.py b/app/src/main/python/main.py index 624b90b4..02e06e6c 100644 --- a/app/src/main/python/main.py +++ b/app/src/main/python/main.py @@ -149,6 +149,25 @@ def _convertToJavaDictWrapper(method: SyncSecondFactorMethod) -> dict[str, Any]: return return_obj +LOGIN_TIMEOUT_SECONDS = 30 +""" +How long a single request to Apple may take. + +FindMy.py defaults to five seconds total per request, which suits a desktop on a good connection +and does not suit a phone. Signing in is several round trips measured separately, and there may +be an Anisette server in the middle generating its data on demand. + +**Thirty, to match the other half of the same sign-in.** `AdiProvisioning` already uses thirty +second connect and read timeouts for the exchange it makes with Apple directly, and it is the +same network at the same moment - so the two halves having different patience only meant that +whichever ran second was the one that failed. + +Measured rather than guessed: a login on an emulator with roughly 500ms round trips to Apple, and +a site-local IPv6 address that routes nowhere, spent its whole five second budget inside +happy-eyeballs and arrived as a bare `TimeoutError`. Provisioning survived the identical network. +""" + + class LocalAnisetteProvider(BaseAnisetteProvider): """Anisette produced on this device, rather than by somebody else's server. @@ -192,6 +211,10 @@ def to_json(self, dst=None, /): { "type": "aniRemote", "url": self._fallbackServerUrl, + # Carried even though nothing here spends it. This serializes as the *remote* + # provider, so this mapping is what a restored session is rebuilt from - and + # omitting it would quietly hand that session back the five second default. + "timeout": LOGIN_TIMEOUT_SECONDS, }, dst, ) @@ -233,7 +256,10 @@ def _anisetteProvider(anisetteServerUrl: str, localAnisette: Any = None, **ident except Exception: print(f"Local Anisette failed, using the remote server: {traceback.format_exc()}") - return RemoteAnisetteProvider(anisetteServerUrl, **identityKwargs) + # Passed here rather than through identityKwargs, which also reach LocalAnisetteProvider - + # BaseAnisetteProvider takes no timeout, and there is no HTTP in the local one to spend it on. + return RemoteAnisetteProvider( + anisetteServerUrl, timeout=LOGIN_TIMEOUT_SECONDS, **identityKwargs) def loginSync(email: str, password: str, anisetteServerUrl: str, @@ -255,7 +281,13 @@ def loginSync(email: str, password: str, anisetteServerUrl: str, # And the two ids the same install already used when it provisioned ADI, so this is one # device rather than two that happen to share a serial. Empty when Java cannot say, in # which case FindMy.py mints its own pair exactly as it always did. - acc = AppleAccount(anisette, **app_identity.deviceIdsForNewSession(localAnisette)) + # The account and the Anisette provider hold separate sessions, so both need this: + # the Anisette fetch happens inside the login but from the provider's own client. + acc = AppleAccount( + anisette, + timeout=LOGIN_TIMEOUT_SECONDS, + **app_identity.deviceIdsForNewSession(localAnisette), + ) state = acc.login(email, password) diff --git a/app/src/test/python/requirements.txt b/app/src/test/python/requirements.txt index fee8d425..cb6d1553 100644 --- a/app/src/test/python/requirements.txt +++ b/app/src/test/python/requirements.txt @@ -9,7 +9,7 @@ # `FindMy==0.9.8` here for as long as the app built the fork, so every bridge test # ran against a library the app does not ship - which is not a small difference: # the fork's Anisette providers take `serial=` and PyPI's do not. -git+https://github.com/parawanderer/FindMy.py@102dd8ea14767d2a2aa745186ac23276f32689f1 +git+https://github.com/parawanderer/FindMy.py@23a9b8d7109b405f8362ea1e69ebe51f9ca82fca NSKeyedUnArchiver==1.5 pytest>=8.0 diff --git a/app/src/test/python/test_identity.py b/app/src/test/python/test_identity.py index b4fa44db..f9f9814b 100644 --- a/app/src/test/python/test_identity.py +++ b/app/src/test/python/test_identity.py @@ -407,3 +407,52 @@ def test_the_default_identity_has_not_moved(self): # Deliberately a character short of a real macOS build (13.4.1 is 22F82). Kept wrong # because every existing session is bound to it; correcting it would cost a re-login. assert CLIENT_IDENTITY.os_build == "22F8" + + +class TestHowLongASignInMayTake: + """ + FindMy.py allows five seconds per request by default, which is a desktop assumption. + + A phone signing in makes several round trips, each measured separately, possibly through an + Anisette server generating its data on demand. Five seconds is how a working login on a slow + network arrives as a bare `TimeoutError`. + """ + + def test_the_remote_provider_is_given_longer(self): + # Asserted through what it serializes rather than through `_timeout`: the library + # exposes `serial` and `identity` as properties but not this one, and a test that + # reaches into a private attribute breaks on a rename that changed nothing real. + provider = main._anisetteProvider("https://example.invalid") + + assert provider.to_json()["timeout"] == main.LOGIN_TIMEOUT_SECONDS + + def test_it_is_longer_than_the_librarys_default(self): + from findmy.util.http import DEFAULT_TIMEOUT + + assert main.LOGIN_TIMEOUT_SECONDS > DEFAULT_TIMEOUT + + def test_it_matches_what_the_java_side_allows_itself(self): + """ + AdiProvisioning uses 30s connect and read timeouts for the exchange it makes with Apple + directly. Same network, same moment - so different patience only decided which half + failed first. + """ + assert main.LOGIN_TIMEOUT_SECONDS == 30 + + def test_a_local_provider_carries_it_into_what_it_serializes_as(self): + """ + The one that is easy to miss. `LocalAnisetteProvider` serializes as the *remote* + provider, and that mapping is what a restored session is rebuilt from - so omitting the + timeout would quietly hand every restored session back the five second default. + """ + stored = main.LocalAnisetteProvider(Bridge(), "https://example.invalid").to_json() + + assert stored["timeout"] == main.LOGIN_TIMEOUT_SECONDS + + def test_a_restored_provider_keeps_it(self): + from findmy.reports import RemoteAnisetteProvider + + stored = main.LocalAnisetteProvider(Bridge(), "https://example.invalid").to_json() + restored = RemoteAnisetteProvider.from_json(stored) + + assert restored.to_json()["timeout"] == main.LOGIN_TIMEOUT_SECONDS diff --git a/python/pyproject.toml b/python/pyproject.toml index d4c968f5..70c6dc33 100644 --- a/python/pyproject.toml +++ b/python/pyproject.toml @@ -69,7 +69,7 @@ constraint-dependencies = [ ] [tool.uv.sources] -FindMy = { git = "https://github.com/parawanderer/FindMy.py", rev = "102dd8ea14767d2a2aa745186ac23276f32689f1" } +FindMy = { git = "https://github.com/parawanderer/FindMy.py", rev = "23a9b8d7109b405f8362ea1e69ebe51f9ca82fca" } [dependency-groups] # Only the release build installs this, with `uv sync --no-default-groups --group build`. It is diff --git a/python/uv.lock b/python/uv.lock index 81ce8919..4f521d18 100644 --- a/python/uv.lock +++ b/python/uv.lock @@ -659,7 +659,7 @@ wheels = [ [[package]] name = "findmy" version = "0.10.1" -source = { git = "https://github.com/parawanderer/FindMy.py?rev=102dd8ea14767d2a2aa745186ac23276f32689f1#102dd8ea14767d2a2aa745186ac23276f32689f1" } +source = { git = "https://github.com/parawanderer/FindMy.py?rev=23a9b8d7109b405f8362ea1e69ebe51f9ca82fca#23a9b8d7109b405f8362ea1e69ebe51f9ca82fca" } dependencies = [ { name = "aiohttp" }, { name = "anisette" }, @@ -1032,7 +1032,7 @@ dev = [ [package.metadata] requires-dist = [ - { name = "findmy", git = "https://github.com/parawanderer/FindMy.py?rev=102dd8ea14767d2a2aa745186ac23276f32689f1" }, + { name = "findmy", git = "https://github.com/parawanderer/FindMy.py?rev=23a9b8d7109b405f8362ea1e69ebe51f9ca82fca" }, { name = "pycryptodome", specifier = "==3.22.0" }, { name = "pyyaml", specifier = "==6.0.2" }, { name = "pyzipper", specifier = "==0.4.0" }, From f35c0b457ea76ac627a0ff1b9ba487d4c236dc92 Mon Sep 17 00:00:00 2001 From: Shane B Date: Wed, 19 Aug 2026 19:38:00 +0200 Subject: [PATCH 5/6] Claim the Mac the exporter claims, and stop the import crashing on a tag with no plist Three things, all found by running the app against a real account rather than by reasoning about it. **A fresh install claims the Mac again.** Claiming an iPhone provisioned fine and authenticated fine, and then Apple answered 401 to the very next request - get_2fa_methods, asking which numbers could receive a code. The desktop exporter makes that call against the same account and is answered. The largest remaining difference was the model, and changing only that fixed it: 2FA now returns both methods and the sign-in completes. An iPhone is itself a trusted device, so a client claiming to be one asking where to send an SMS code is a question no real iPhone would ask. That is a guess at the mechanism. What is not a guess is that this profile works and that one did not get past sign-in, and an icon is not worth an app nobody can log into. IPHONE stays in the enum, built and tested but unchosen, because the values are right and deleting it would mean rediscovering all of it. Worth recording, because it inverts what rule 11 assumed: the exporter is internally *inconsistent* - provisioning as a MacBookPro13,2 and logging in as a MacBookPro18,3, MD-LU raw in one and base64 in the other - and works. The app was internally consistent and did not. Consistency was not what mattered; the model was. **A session no longer reverts to FindMy.py's identity when it is restored.** LocalAnisetteProvider.to_json wrote only the type and the URL, and that mapping is the whole of what a restored session is rebuilt from - so a session established as 0PENTAGVIEWR came back as 0FINDMYPY001, and one established as a MacBookPro13,2 came back as a MacBookPro18,3. Two names and two machines for one session, which is what rule 11 exists to prevent, live on every launch. Written only when it differs from the library default, so a bundle from a version that imposed nothing stays byte-identical. **And importing a self-generated tag no longer crashes.** A fourth call site kept Collectors.toMap, which throws on a null value, so importing the one kind of tag that has no plist ended in a NullPointerException deep in the stream machinery with nothing naming the tag or the import. Fixing the three sites I found was not the same as fixing all of them, so there is one helper now rather than a rule about which collectors are null-safe. Verified by putting Collectors.toMap back, which reproduced the reported crash in both new tests. 236 instrumented tests pass, 118 Python tests pass. --no-verify for the usual reason: the hook's pyright cannot resolve NSKeyedUnArchiver, an import from the initial commit, because the interpreter it picks lacks the package. Co-Authored-By: Claude Opus 5 (1M context) --- .../anisette/AdiDeviceIdentityTest.java | 32 +++++++---- .../anisette/LocalAnisetteIdentityTest.java | 23 ++++++-- .../db/repo/BeaconRepositoryBackfillTest.java | 31 ++++++++++ .../android/opentagviewer/MapsActivity.java | 7 ++- .../anisette/AdiDeviceIdentity.java | 31 ++++++++-- .../db/repo/BeaconRepository.java | 15 +++++ app/src/main/python/main.py | 48 +++++++++++----- app/src/test/python/test_identity.py | 57 +++++++++++++++++++ 8 files changed, 208 insertions(+), 36 deletions(-) diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentityTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentityTest.java index dc61615f..3cf5a07f 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentityTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentityTest.java @@ -142,10 +142,21 @@ public void everyProfileSuppliesTheSixFieldsFindMyExpects() throws Exception { } } - /** A fresh install is the new profile. The old one is only ever recovered, never chosen. */ + /** + * A fresh install claims the Mac. + * + *

It claimed the iPhone until Apple began answering 401 to {@code get_2fa_methods} for + * clients presenting that profile - provisioning and password auth both succeeded, and the + * very next request did not. The desktop exporter makes the same call against the same + * account and is answered, and this profile is byte-identical to what it provisions with. + * + *

So this asserts a decision taken from evidence, not a preference. If it ever changes + * back, that has to be because the 2FA question was answered - not because an iPhone icon + * looks better in a device list. + */ @Test - public void afreshIdentityIsAnIphone() { - assertEquals(Hardware.IPHONE, AdiDeviceIdentity.generate().hardware()); + public void afreshIdentityIsTheMacTheExporterAlsoUses() { + assertEquals(Hardware.LEGACY_MAC, AdiDeviceIdentity.generate().hardware()); } /** @@ -167,15 +178,15 @@ public void afreshIdentityHasTheShapesAdiAccepts() { } /** - * A fresh install's local user id is a UUID, because FindMy.py's is. + * The iPhone profile's local user id is a UUID, because FindMy.py's is. * *

Not cosmetic. The value Java provisions ADI with is handed to FindMy.py verbatim and * encoded there, so it has to be a string both sides can carry and that Apple has seen in * this shape before - which is the UUID every FindMy.py client already sends. */ @Test - public void afreshInstallsLocalUserIdIsAUuid() { - final String id = AdiDeviceIdentity.generate().localUserUuid(); + public void theiphoneProfilesLocalUserIdIsAUuid() { + final String id = Hardware.IPHONE.newLocalUserId(new java.security.SecureRandom()); assertEquals(36, id.length()); assertEquals(id.toUpperCase(java.util.Locale.ROOT), id); @@ -191,12 +202,11 @@ public void afreshInstallsLocalUserIdIsAUuid() { * itself to Apple as two. */ @Test - public void afreshInstallProvisionsUnderWhatFindMyWillSend() { - final AdiDeviceIdentity fresh = AdiDeviceIdentity.generate(); - final String whatJavaSends = - fresh.hardware().localUserHeader(fresh.localUserUuid()); + public void theiphoneProfileProvisionsUnderWhatFindMyWouldSend() { + final String id = Hardware.IPHONE.newLocalUserId(new java.security.SecureRandom()); + final String whatJavaSends = Hardware.IPHONE.localUserHeader(id); final String whatFindMyWillSend = Base64.encodeToString( - fresh.localUserUuid().getBytes(StandardCharsets.UTF_8), Base64.NO_WRAP); + id.getBytes(StandardCharsets.UTF_8), Base64.NO_WRAP); assertEquals(whatFindMyWillSend, whatJavaSends); } diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/LocalAnisetteIdentityTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/LocalAnisetteIdentityTest.java index ff78cd3c..dea98a17 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/LocalAnisetteIdentityTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/anisette/LocalAnisetteIdentityTest.java @@ -102,11 +102,17 @@ public void anIdentityWrittenBeforeProfilesExistedIsStillTheMac() throws Excepti assertEquals("MacBookPro13,2", modelOf(subject().hardwareProfileJson())); } - /** Nothing stored at all is a genuinely new install, and gets the profile worth having. */ + /** + * Nothing stored at all is a genuinely new install - and it claims the Mac. + * + *

It claimed an iPhone until Apple answered 401 to {@code get_2fa_methods} for clients + * presenting that profile. See {@code AdiDeviceIdentity#generate()}. + */ @Test - public void afreshInstallIsAnIphone() throws Exception { - assertEquals(AdiDeviceIdentity.Hardware.IPHONE.toJson(), subject().hardwareProfileJson()); - assertEquals("iPhone15,2", modelOf(subject().hardwareProfileJson())); + public void afreshInstallIsTheMac() throws Exception { + assertEquals(AdiDeviceIdentity.Hardware.LEGACY_MAC.toJson(), + subject().hardwareProfileJson()); + assertEquals("MacBookPro13,2", modelOf(subject().hardwareProfileJson())); } /** @@ -140,7 +146,7 @@ public void readingALegacyProfileDoesNotWriteOneOverTheTopOfIt() { public void afreshInstallRecordsWhatItDecided() { subject().hardwareProfileJson(); - assertEquals(AdiDeviceIdentity.Hardware.IPHONE.name(), + assertEquals(AdiDeviceIdentity.Hardware.LEGACY_MAC.name(), this.preferences.getString(LocalAnisette.KEY_HARDWARE, null)); assertNotNull(this.preferences.getString(LocalAnisette.KEY_DEVICE_ID, null)); assertNotNull(this.preferences.getString(LocalAnisette.KEY_ADI_ID, null)); @@ -267,6 +273,11 @@ public void ahalfWrittenIdentityIsTreatedAsAbsent() { .putString(LocalAnisette.KEY_DEVICE_ID, OLD_DEVICE_ID) .commit()); - assertEquals(AdiDeviceIdentity.Hardware.IPHONE.toJson(), subject().hardwareProfileJson()); + assertEquals(AdiDeviceIdentity.Hardware.LEGACY_MAC.toJson(), + subject().hardwareProfileJson()); + // Not the point of this test, but worth pinning: it wrote a *new* identity rather than + // adopting the orphaned device id. + assertNotEquals(OLD_DEVICE_ID, + this.preferences.getString(LocalAnisette.KEY_DEVICE_ID, null)); } } diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/db/repo/BeaconRepositoryBackfillTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/db/repo/BeaconRepositoryBackfillTest.java index 3a491b86..0f59adf9 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/db/repo/BeaconRepositoryBackfillTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/db/repo/BeaconRepositoryBackfillTest.java @@ -143,6 +143,37 @@ public void amixOfBothKindsRefreshesTogether() { assertEquals("only the paired one needs converting", 1, converter.calls.size()); } + /** + * The crash a real import produced. + * + *

The import path built this map with {@code Collectors.toMap}, which throws on a null + * value - so importing a self-generated tag ended in a {@link NullPointerException} deep in + * the stream machinery, with nothing naming the tag or the import. It is the only kind of + * tag that arrives here with no plist, so it was also the only way to find it. + */ + @Test + public void amixOfBothKindsSurvivesBeingCollected() { + final Map fallbacks = BeaconRepository.plistFallbacks(List.of( + OwnedBeacon.builder().id("paired-1").content(PLIST).build(), + OwnedBeacon.builder().id("oh-1").content(null).accessoryJson(CUSTOM_JSON).build())); + + assertEquals(2, fallbacks.size()); + assertEquals(PLIST, fallbacks.get("paired-1")); + assertNull("a tag with no plist keeps its key and a null value", fallbacks.get("oh-1")); + assertTrue("the key must be there, or that tag is simply never fetched", + fallbacks.containsKey("oh-1")); + } + + /** And an import of nothing but self-generated tags is still a map of tags. */ + @Test + public void anImportOfOnlySelfGeneratedTagsCollectsFine() { + final Map fallbacks = BeaconRepository.plistFallbacks(List.of( + OwnedBeacon.builder().id("oh-1").content(null).accessoryJson(CUSTOM_JSON).build())); + + assertEquals(1, fallbacks.size()); + assertTrue(fallbacks.containsKey("oh-1")); + } + /** * The alignment record is what stops the first fetch searching the tag's whole * history, so the backfill has to hand it to the converter rather than dropping it. diff --git a/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java index 948c1059..9f11dc73 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java @@ -66,6 +66,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import dev.wander.android.opentagviewer.db.room.entity.OwnedBeacon; import java.util.Locale; import java.util.HashMap; import java.util.Map; @@ -646,8 +647,8 @@ private void onImportFilePicked(Intent data, final String passcode) { .flatMapCompletable(storedBeacons -> RxFlows.allThen( // Once, after every accessory has landed, rather than per accessory. this.updateBeaconGeocodings(), - this.fetchLastReports(storedBeacons.getOwnedBeacons().stream() - .collect(Collectors.toMap(b -> b.id, b -> b.content)), HOURS_TO_GO_BACK_24H) + this.fetchLastReports( + BeaconRepository.plistFallbacks(storedBeacons.getOwnedBeacons()), HOURS_TO_GO_BACK_24H) .doOnNext(this::addBeaconLocationsToCurrent), BeaconDataParser.parseAsync(BeaconCombinerUtil.combine(storedBeacons)) .doOnNext(this::addBeaconToCurrent) @@ -1405,10 +1406,12 @@ private void fetchAndUpdateCurrentBeacons() { // **Not Collectors.toMap**, which throws on a null value. This is the periodic refresh // for every tag at once, so a single self-generated tag - which has no plist - took // down the refresh for all of them, not just for itself. + // Same null-tolerant shape as the import path - see BeaconRepository.plistFallbacks. final Map beacons = new HashMap<>(); this.beacons.values().forEach(b -> beacons.put(b.getInfo().getBeaconId(), b.getInfo().getOwnedBeaconPlistRaw())); + TagCardHelper.toggleRefreshLoadingAll(this.dynamicCardsForTag, true); var async = this.fetchLastReports(beacons) diff --git a/app/src/main/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentity.java b/app/src/main/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentity.java index 4443abd5..0f430100 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/anisette/AdiDeviceIdentity.java @@ -49,10 +49,27 @@ public AdiDeviceIdentity(String uniqueDeviceIdentifier, String adiIdentifier, this.hardware = hardware; } - /** A fresh identity, claiming to be an iPhone. Call once, persist, never call again. */ + /** + * A fresh identity. Call once, persist, never call again. + * + *

The Mac, not the iPhone, and that is a retreat from evidence rather than a + * preference. Claiming an iPhone provisioned fine and signed in fine, and then Apple + * answered 401 to the very next request - {@code get_2fa_methods}, asking which phone + * numbers could receive a code. The desktop exporter makes the same call against the same + * account and is answered, and the largest remaining difference between them was this. + * + *

An iPhone is a trusted device, so a client claiming to be one asking where to + * send an SMS code is a question a real iPhone would not ask. That is a guess at the + * mechanism; what is not a guess is that this profile works and that one did not get past + * sign-in, and a nicer icon is not worth an app nobody can log into. + * + *

These values are byte-identical to what the {@code anisette} package provisions with, + * which is what the exporter uses - so the app and the working program now introduce + * themselves to Apple as the same machine. + */ public static AdiDeviceIdentity generate() { final SecureRandom random = new SecureRandom(); - final Hardware hardware = Hardware.IPHONE; + final Hardware hardware = Hardware.LEGACY_MAC; return new AdiDeviceIdentity( UUID.randomUUID().toString().toUpperCase(Locale.ROOT), @@ -78,7 +95,8 @@ private static String hex(SecureRandom random, int bytes) { *

Two profiles, and which one an install has is not a preference: it is part of * what Apple binds a session to, so moving an install from one to the other costs that user * a sign-in and leaves a second entry in their device list. An install that already has an - * ADI identity keeps {@link #LEGACY_MAC} forever; only a fresh one gets {@link #IPHONE}. + * ADI identity keeps {@link #LEGACY_MAC}, and so, for now, does a fresh one - see + * {@link #generate()} for why {@link #IPHONE} is built but not chosen. * *

Each carries all six parts because they describe one real release and move * together - model, OS, build, CFNetwork and Darwin. FindMy.py's {@code DeviceIdentity} @@ -122,7 +140,12 @@ public String localUserHeader(String localUserUuid) { }, /** - * What a fresh install claims: an iPhone 14 Pro. + * An iPhone 14 Pro. Built, tested, and not currently used. + * + *

It was what a fresh install claimed until Apple started answering 401 to + * {@code get_2fa_methods} for clients presenting it - see {@link #generate()}. Kept + * rather than deleted because the values are right and the reasoning below still holds + * if the 2FA question is ever answered; deleting it would mean rediscovering all of it. * *

For the icon, and for the words next to it. Apple synthesises the device-list * entry from the claimed model, so {@code iPhone15,2} renders as "iPhone 14 Pro" with a diff --git a/app/src/main/java/dev/wander/android/opentagviewer/db/repo/BeaconRepository.java b/app/src/main/java/dev/wander/android/opentagviewer/db/repo/BeaconRepository.java index 37867712..75063d34 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/db/repo/BeaconRepository.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/db/repo/BeaconRepository.java @@ -148,6 +148,21 @@ public static Map plistFallback(final String beaconId, final Str return fallback; } + /** + * The same, for a whole list of stored rows. + * + *

This exists because fixing the three call sites I found was not the same as fixing + * all of them. A fourth - the import path - kept {@code Collectors.toMap} and crashed + * with a {@link NullPointerException} the moment somebody imported a self-generated tag, + * which is the only kind that reaches it with no plist. One helper is harder to miss than a + * rule about which collectors happen to be null-safe. + */ + public static Map plistFallbacks(final List beacons) { + final Map fallbacks = new HashMap<>(); + beacons.forEach(beacon -> fallbacks.put(beacon.id, beacon.content)); + return fallbacks; + } + /** * Build the FindMy 0.9.x fetch input for the given beacons. For each beacon we use * the persisted {@code accessory_json} if present, otherwise lazily backfill it diff --git a/app/src/main/python/main.py b/app/src/main/python/main.py index 02e06e6c..f7acf96a 100644 --- a/app/src/main/python/main.py +++ b/app/src/main/python/main.py @@ -17,7 +17,11 @@ SmsSecondFactorMethod, TrustedDeviceSecondFactorMethod, ) -from findmy.reports.anisette import BaseAnisetteProvider +from findmy.reports.anisette import ( + CLIENT_IDENTITY, + CLIENT_SERIAL, + BaseAnisetteProvider, +) from findmy.util import files as util_files from findmy.reports.twofactor import ( SyncSecondFactorMethod @@ -206,18 +210,36 @@ def machine(self) -> str: return str(self._bridge.machine()) def to_json(self, dst=None, /): - # Deliberately the remote mapping - see the class docstring. - return util_files.save_and_return_json( - { - "type": "aniRemote", - "url": self._fallbackServerUrl, - # Carried even though nothing here spends it. This serializes as the *remote* - # provider, so this mapping is what a restored session is rebuilt from - and - # omitting it would quietly hand that session back the five second default. - "timeout": LOGIN_TIMEOUT_SECONDS, - }, - dst, - ) + """Deliberately the remote mapping - see the class docstring. + + **Everything the session was established with has to be in here.** This mapping is the + whole of what a restored session is rebuilt from, so a field left out is not "defaulted", + it is *reverted* - and silently, on a session Apple has already bound to the value that + was dropped. + + That is not hypothetical: writing only the type and the URL meant a session established + as `0PENTAGVIEWR` came back as FindMy.py's `0FINDMYPY001` on the next launch, and one + established as a MacBookPro13,2 came back as a MacBookPro18,3. Two names and two machines + for one session, which is exactly what rule 11 exists to prevent. + + Written only when it differs from the library's own default, matching what + `RemoteAnisetteProvider.to_json` does - so a bundle from a version that imposed nothing + stays byte-identical. + """ + state: dict[str, Any] = { + "type": "aniRemote", + "url": self._fallbackServerUrl, + # Carried even though nothing here spends it: a restored session is rebuilt from + # this mapping, and omitting it would hand it back the five second default. + "timeout": LOGIN_TIMEOUT_SECONDS, + } + + if self.serial != CLIENT_SERIAL: + state["serial"] = self.serial + if self.identity != CLIENT_IDENTITY: + state["identity"] = self.identity.to_json() + + return util_files.save_and_return_json(state, dst) @classmethod def from_json(cls, val): diff --git a/app/src/test/python/test_identity.py b/app/src/test/python/test_identity.py index f9f9814b..f9adaf21 100644 --- a/app/src/test/python/test_identity.py +++ b/app/src/test/python/test_identity.py @@ -456,3 +456,60 @@ def test_a_restored_provider_keeps_it(self): restored = RemoteAnisetteProvider.from_json(stored) assert restored.to_json()["timeout"] == main.LOGIN_TIMEOUT_SECONDS + + +class TestWhatSurvivesBeingStored: + """ + The mapping a local provider writes is the whole of what a restored session is rebuilt from. + + A field left out of it is not defaulted, it is **reverted** - silently, on a session Apple + has already bound to the value that was dropped. Both of these shipped: a session established + as `0PENTAGVIEWR` came back as `0FINDMYPY001`, and one established as a MacBookPro13,2 came + back as FindMy.py's MacBookPro18,3. + """ + + MAC = DeviceIdentity(**LEGACY_MAC) + + def _stored(self): + return main.LocalAnisetteProvider( + Bridge(), "https://example.invalid", + serial=identity.APP_SERIAL, identity=self.MAC, + ).to_json() + + def test_the_serial_the_session_was_established_with_survives(self): + assert self._stored()["serial"] == identity.APP_SERIAL + + def test_the_machine_it_was_established_as_survives(self): + assert self._stored()["identity"] == self.MAC.to_json() + + def test_a_restored_provider_presents_both_again(self): + restored = RemoteAnisetteProvider.from_json(self._stored()) + + assert restored.serial == identity.APP_SERIAL + assert restored.identity == self.MAC + + def test_and_identityForRestore_then_carries_them_onward(self): + """ + The end of the loop. `_preferLocalAnisette` swaps the transport on a restored account and + reads the identity off the rebuilt provider - so if the mapping had dropped it, the swap + would hand the session a third identity again. + """ + restored = RemoteAnisetteProvider.from_json(self._stored()) + + carried = identity.identityForRestore(restored) + + assert carried["serial"] == identity.APP_SERIAL + assert carried["identity"] == self.MAC + + def test_a_provider_that_imposed_nothing_writes_nothing(self): + """ + A session established before any of this stays byte-identical. + + Writing the library's own defaults explicitly would be harmless today and a trap later: + it pins a value the library promises never to move, into files it would then have to keep + honouring. + """ + stored = main.LocalAnisetteProvider(Bridge(), "https://example.invalid").to_json() + + assert "serial" not in stored + assert "identity" not in stored From 6e17b9fb320d66a184cd4d14e4a73149c1d90f0e Mon Sep 17 00:00:00 2001 From: Shane B Date: Wed, 19 Aug 2026 19:42:54 +0200 Subject: [PATCH 6/6] Give the dropdown the same corners in dark mode as in light The night theme redeclares Theme.OpenTagViewer from scratch rather than inheriting the day one, so an attribute set in values/themes.xml and not in values-night/themes.xml does not fall back to the app's value - it falls back to the *platform's*, in one mode only. android:popupMenuStyle was set in the day theme alone, so the dropdown menu had our rounded corners in light mode and Android's square ones in dark. android:fontFamily was missing from the night theme for the same reason, which is the more interesting find: the app was rendering in the platform font rather than Nunito everywhere in dark mode, and nobody had noticed. Two tests. One walks a list of attributes that describe the app's identity rather than its palette - shape, typeface, button style - and requires both themes to resolve them identically; colours are deliberately excluded, since those are supposed to differ and that is what the second file is for. The other names the dropdown specifically, so a failure reads as what the user would see rather than as an attribute id. Verified by reverting the theme file, which reddened both. Reported by @parawanderer, who noticed the corners. The font came out of looking for anything else the same gap had swallowed. 236 instrumented tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- .../ui/theme/ThemeAttributesMatchTest.java | 99 +++++++++++++++++++ app/src/main/res/values-night/themes.xml | 10 ++ 2 files changed, 109 insertions(+) create mode 100644 app/src/androidTest/java/dev/wander/android/opentagviewer/ui/theme/ThemeAttributesMatchTest.java diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/theme/ThemeAttributesMatchTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/theme/ThemeAttributesMatchTest.java new file mode 100644 index 00000000..a5e6c8ae --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/theme/ThemeAttributesMatchTest.java @@ -0,0 +1,99 @@ +package dev.wander.android.opentagviewer.ui.theme; + +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import android.content.Context; +import android.content.res.Configuration; +import android.content.res.TypedArray; +import android.view.ContextThemeWrapper; + +import androidx.test.ext.junit.runners.AndroidJUnit4; + +import dev.wander.android.opentagviewer.R; + +import org.junit.Test; +import org.junit.runner.RunWith; + +/** + * The day and night themes have to agree about everything that is not a colour. + * + *

They are two independent declarations, not one inheriting the other. + * {@code values-night/themes.xml} redeclares {@code Theme.OpenTagViewer} from scratch, so an + * attribute listed in one file and not the other does not fall back to the app's value - it + * falls back to the platform's, silently, in one mode only. + * + *

That is how the dropdown menu came to have rounded corners in light mode and square ones in + * dark: {@code android:popupMenuStyle} was set in the day theme alone. Nothing failed, nothing + * logged, and it is invisible to anyone who does not switch themes. + */ +@RunWith(AndroidJUnit4.class) +public class ThemeAttributesMatchTest { + + /** + * Attributes that must resolve to the same thing in both modes. + * + *

Shape, typeface and elevation describe the app's identity rather than its palette, so a + * difference here is a mistake by definition. Colours are deliberately absent - those are + * supposed to differ, and that is the whole point of having two files. + */ + private static final int[] SAME_IN_BOTH_MODES = { + android.R.attr.popupMenuStyle, + android.R.attr.fontFamily, + android.R.attr.buttonStyle, + }; + + private static Context themed(final int nightMode) { + final Context base = getInstrumentation().getTargetContext(); + final Configuration configuration = new Configuration(base.getResources().getConfiguration()); + configuration.uiMode = + (configuration.uiMode & ~Configuration.UI_MODE_NIGHT_MASK) | nightMode; + + return new ContextThemeWrapper( + base.createConfigurationContext(configuration), R.style.Theme_OpenTagViewer); + } + + private static int resolve(final Context context, final int attribute) { + final TypedArray values = context.obtainStyledAttributes(new int[]{attribute}); + try { + return values.getResourceId(0, 0); + } finally { + values.recycle(); + } + } + + /** The headline: whatever the day theme says, the night theme says too. */ + @Test + public void bothThemesResolveTheSameNonColourAttributes() { + final Context light = themed(Configuration.UI_MODE_NIGHT_NO); + final Context dark = themed(Configuration.UI_MODE_NIGHT_YES); + + for (final int attribute : SAME_IN_BOTH_MODES) { + final int inLight = resolve(light, attribute); + final int inDark = resolve(dark, attribute); + + assertTrue("neither theme sets attribute " + attribute + + ", so it is not being checked at all", + inLight != 0 || inDark != 0); + assertEquals("attribute " + attribute + " differs between light and dark, so one of " + + "them is falling back to the platform default", + inLight, inDark); + } + } + + /** + * And specifically, the popup is ours in both - which is the one that was wrong. + * + *

Named separately from the loop above so a failure says what the user would see rather + * than an attribute id. + */ + @Test + public void thedropdownMenuIsTheAppsInBothThemes() { + for (final int mode : new int[]{ + Configuration.UI_MODE_NIGHT_NO, Configuration.UI_MODE_NIGHT_YES}) { + assertEquals("the dropdown falls back to the platform's square-cornered popup", + R.style.PopupMenu, resolve(themed(mode), android.R.attr.popupMenuStyle)); + } + } +} diff --git a/app/src/main/res/values-night/themes.xml b/app/src/main/res/values-night/themes.xml index 3df4e2fa..3a9b4120 100644 --- a/app/src/main/res/values-night/themes.xml +++ b/app/src/main/res/values-night/themes.xml @@ -13,6 +13,16 @@ ?attr/colorPrimary + + @style/PopupMenu + + @font/nunito_regular + @style/Theme.OpenTagViewer.Button