| Patch for part 2 (the chest assets) now attached: fix-chest-asset.patch.
The description says no patch is attached deliberately, because renaming an
asset touches the catalog and I had not verified the rendering. That part is
now checked, so here is the rename. Part 1 (ArrowHistoryView) still has no
patch and still needs the product decision described above.
What the patch does
-------------------
Renames the imageset so the German pair matches its open counterpart, which is
the spelling OIMcurrency.swift:74 already asks for (`chest: "DEU"`):
OIM.xcassets/OIM-buttons/ChestClosedDE.imageset
-> OIM.xcassets/OIM-buttons/ChestClosedDEU.imageset
It is a pure `git mv` of the imageset directory: two files moved, no content
changed.
Why renaming closed->DEU rather than open->DE
---------------------------------------------
`chest: "DEU"` is what the source declares, so this is the rename that needs no
code change. Renaming ChestOpenDEU to ChestOpenDE would additionally require
editing OIMcurrency.swift, and "DEU" is consistent with the three-letter form
used by the other entries.
Checks performed
----------------
* No literal reference to the old name anywhere:
grep -rn "ChestClosedDE" --include=*.swift --include=*.json .
Image names are built by concatenation in OIMactionButtons.swift:10-11
let chestOpen = "ChestOpen"
let chestClosed = "ChestClosed"
and suffixed with the `chest` string, so nothing hardcodes the full name.
* The SVGs inside were already named consistently (ChestDEclosed.svg /
ChestDEopen.svg), so only the imageset directory needed renaming.
* Asset catalog compiles: the GNU_Taler target builds and Assets.car is
produced with the renamed imageset.
Not verified
------------
The rendering itself is still unverified, because OIMdollars is unreachable at
runtime: it is absent from `OIMcurrencies` (OIMcurrency.swift:13) and
`oimCurrency()` never returns it (OIMcurrency.swift:15-24). So this fixes the
naming inconsistency and removes the latent missing-image, but no build can yet
display the German chest pair to confirm both images resolve. That is why
there is no screenshot with this note.
Branch: dev/phoenix-bot/fix-chest-asset (commit 2a15b4ea), on top of the
current iOS wallet master.
Added by an agent (bot-cg) on behalf of Christian Grothoff. |
|
| Patch verified against the compiled asset catalog, which is the thing that
actually decides whether an image resolves at runtime.
My earlier note said the rendering could not be verified because OIMdollars is
unreachable, and left it there. That was the wrong place to stop: what the
defect is about is whether "ChestClosed" + chest and "ChestOpen" + chest both
resolve for chest == "DEU", and Assets.car answers that without needing the
view on screen.
Method: build the GNU_Taler target from each branch and read the catalog with
`xcrun assetutil --info .../GNU_Taler.app/Assets.car`.
Without the patch (current master):
ChestClosed ChestClosedCHF ChestClosedCdI ChestClosedDE
ChestClosedEU ChestClosedEUR ChestClosedSLE
ChestOpen ChestOpenCHF ChestOpenCdI ChestOpenDEU
ChestOpenEU ChestOpenEUR ChestOpenSLE
ChestClosedDEU present = False <-- what OIMcurrency.swift:74 asks for
ChestOpenDEU present = True
ChestClosedDE present = True
With fix-chest-asset.patch:
ChestClosedDEU present = True
ChestOpenDEU present = True
ChestClosedDE present = False
So before the patch the German pair cannot both resolve under any single
spelling, and after it `chest: "DEU"` resolves both. No other Chest* name
changed, and nothing refers to the old name by a literal (the names are built
by concatenation in OIMactionButtons.swift:10-11).
To re-run:
git checkout dev/phoenix-bot/fuzzbang && python3 -m taler_fuzz build
xcrun assetutil --info "$app/Assets.car" | grep -o '"Name" : "Chest[^"]*"' | sort -u
git checkout dev/phoenix-bot/fix-chest-asset && python3 -m taler_fuzz build
xcrun assetutil --info "$app/Assets.car" | grep -o '"Name" : "Chest[^"]*"' | sort -u
Still unverified, and unverifiable without a product change: that the two
images *look* like a matching pair, since no build can display OIMdollars
today. That is a question for whoever wires USD up.
Part 1 of this issue, ArrowHistoryView, still has no patch: it needs the
product decision described in the report.
Added by an agent (bot-cg) on behalf of Christian Grothoff. |
|
| The pair has now been seen, not just counted in the catalog. Screenshots
attached.
My previous note said the rendering was "unverifiable without a product
change". That was true but was the wrong place to stop, because the product
change needed is three lines and does not have to be merged to take a picture.
What was done
-------------
On a throwaway branch on top of dev/phoenix-bot/fix-chest-asset, marked
do-not-merge, `oimCurrency()` was made to map the demo currency to
`OIMdollars`, and `OIMdollars` was added to `OIMcurrencies` -- the two places
(OIMcurrency.swift:13 and 15-24) that make it unreachable today. Nothing else
was touched, and nothing from that branch is proposed for merge: it exists so
that a human can look at the chest.
Then the app was driven into the OIM presentation: fund the wallet with demo
cash, rotate to landscapeRight (which is what Controller.setOIMmode() gates
on), tap the chest.
python3 -m taler_fuzz --orientation portrait --rotate-at 1 \
--prefer "democash,chestclosed" \
replay --tape "0,0,0,0,0" --shots --label chest
Result
------
Both images resolve and render: a grey chest carrying the black-red-gold
flag, lid closed and then open. No missing-image placeholder on either side.
The accessibility tree names them `ChestClosedDEU` (160x160) and
`ChestOpenDEU`, which are the two the renamed catalog provides and the two
`chest: "DEU"` asks for.
chest-closed-DEU-renders.png the renamed asset
chest-open-DEU-renders.png its counterpart
Both are crops of the full screenshots, rotated upright: the OIM presentation
is landscape content captured in a portrait frame, so the uncropped files are
on their side and hard to read.
So the rename is verified twice over: the compiled Assets.car contains the
pair (previous note), and the pair draws (this one).
Still open, unchanged: part 1 of this issue, ArrowHistoryView, which needs a
product decision rather than a patch. And `OIMdollars` remains unreachable on
master -- making it reachable for real is a separate question from whether its
images exist.
Added by an agent (bot-cg) on behalf of Christian Grothoff. |
|
| Part 1 now has a patch, and it is verified by coverage: ArrowHistoryView goes
from never executing to 85.9%. Attached: fix-arrow-history.patch.
My first note said this half needed a product decision rather than a patch.
That was half right -- the decision is whether the arrow presentation is still
wanted -- but it was the wrong place to stop, because one of the two possible
answers can be implemented without making the other harder.
The patch
---------
The file already has the idiom for choosing a history presentation: `oimChart`,
an @AppStorage toggle in More Settings. The arrow/river choice becomes a
second one beside it, shown only when the chart is off because it means
nothing otherwise:
} else if oimArrows {
ArrowHistoryView(...)
} else {
RiverHistoryView(...)
}
replacing the `#if TALER_WALLET` / `#else` pair. Default false, so every
shipped target draws the river exactly as before and nothing changes until
someone turns it on.
Verification
------------
One replay episode per arm, same tape, `coverage --run <id> --top 400`:
arm OIMtransactions Arrow River
master branch, oimArrows set anyway 438/622 70.4% 0 0.0% 81.8%
with the patch, toggle on 468/664 70.5% 440 85.9% 0.0%
with the patch, toggle off 469/664 70.6% 0 0.0% 81.8%
Three things that settles:
* the arrow view executes -- 440 of its 512 lines -- where before it never
did;
* the default path is unchanged: with the toggle off the river draws exactly
as it does today, so this is not a silent swap of one presentation for the
other;
* the baseline arm had `oimArrows = 1` written into UserDefaults while it
ran and the arrow view still never executed, which is the control: on
master the key does not exist in the binary at all (`strings | grep -c
oimArrows` gives 0 on master, 4 with the patch).
Attached arrow-history-enabled.png is the arrow presentation on screen. For
comparison the default (river) is what the previous screenshots in this issue
and every shipped build show.
Two notes for whoever reproduces it
-----------------------------------
* OIMtransactions does not need the chest. TransactionsListView.swift:139
draws it as an .overlay whenever `controller.oimModeActive`, so the route
is: fund with demo cash, tap the balance row, rotate to landscapeRight.
Going via the open chest does not work -- the money row's only tappable
strip is covered by banknote buttons that swallow the tap.
* The toggle is a UserDefaults key, and the harness reinstalls the app per
episode, which wipes preferences. Set `reset_mode` to "never", install
once, then
`xcrun simctl spawn <udid> defaults write com.taler-systems.talerwallet-1 oimArrows -bool YES`.
The other option is still open
------------------------------
If the arrow presentation is not wanted, deleting ArrowHistoryView.swift and
the branch is a smaller diff than this patch, and this patch does not make
that harder -- it is two lines to remove. I have implemented the option that
preserves the code and makes it testable, because that one is reversible and
deletion is not. The choice is still yours.
Added by an agent (bot-cg) on behalf of Christian Grothoff. |
|
| The default path now has a screenshot too, not only a coverage row.
Attached river-history-default-path.png is the same walk, same tape, same
build, with `oimArrows` off: the blue river drawn down the strip with the
sun/moon counts, which is what every shipped target draws today and what this
patch must not change. Beside the arrow screenshot in the previous note, the
pair shows the two presentations swapping cleanly rather than one replacing
the other.
The coverage rows remain the quantitative half of the same claim --
RiverHistoryView 81.8% with the toggle off and 0.0% with it on, Arrow the
other way round.
Added by an agent (bot-cg) on behalf of Christian Grothoff. |
|
| CORRECTION: the "Checked and NOT a problem" paragraph in this report is wrong.
OIMpayView is a third instance of exactly the defect this issue describes, and
I told you not to re-investigate it. Please disregard that paragraph.
What I wrote
------------
OIMpayView looked like a third instance of the same thing, but it is fine:
its call site at PaymentScan.swift:134 is inside `#if OIM`, not a
condition that no target defines, so it is reachable in GNU_Taler and
Taler_Nightly whenever `controller.oimSheetActive` is set. Mentioning it
so nobody re-investigates.
Why it is wrong
---------------
I checked the `#if OIM` immediately above the call and stopped there. It is
nested inside another one:
PaymentScan.swift:107 #if TALER_NIGHTLY2
PaymentScan.swift:131 #if OIM
PaymentScan.swift:134 OIMpayView(stack: ..., amount: effective)
PaymentScan.swift:138 #endif
PaymentScan.swift:145 #endif
`TALER_NIGHTLY2` is defined by no target -- it is `TALER_NIGHTLY` with a `2`
appended, the same way `OIMx`, `TALDIR2` and `DEBUG2` disable their blocks.
So the inner `#if OIM` is never evaluated and OIMpayView.swift (47 lines) is
compiled into every OIM target and constructed by none of them, exactly like
ArrowHistoryView.
The block takes more with it than OIMpayView: lines 107-145 also contain the
whole "already paid" presentation -- the `Already paid` / `Confirm Payment`
navigation titles, "You already paid for this article.", and the
fulfillment-URL `Link` to the merchant's website. None of that is in any
build. Whether that is deliberate is a question for whoever wrote it; it does
not look deliberate from the outside, because the code inside is complete and
maintained.
A pattern worth a grep
----------------------
Disabling code by mutating the flag name rather than by deleting it or using
`#if false` hides it from the obvious search. Searching for `TALER_NIGHTLY`
finds this block; searching for what the project actually defines does not
flag it. Across the app:
#if PRINT_CHANGES 53 blocks 221 lines (build settings define
xxxPRINT_CHANGES instead)
#if false 8 blocks 158 lines
#if TALER_NIGHTLY2 1 block 39 lines
#if TALDIR2 1 block 12 lines
#if DEBUG2 2 blocks 12 lines
#if OIMx 1 block 5 lines
---------
447 lines compiled into nothing
The `PRINT_CHANGES` ones are deliberate tracing and are fine where they are.
The other five flags are each one character away from a real flag, and each
hides a block that reads as if it were meant to ship.
How to check for new ones
-------------------------
Every `#if` in the app, evaluated against each target's
SWIFT_ACTIVE_COMPILATION_CONDITIONS, by a script on branch
dev/phoenix-bot/fuzzbang under harness/scripts/. Anything that comes out
false for all six target/configuration pairs is in this list.
Sorry for the misdirection -- that paragraph was meant to save you time and
would have cost it instead.
Added by an agent (bot-cg) on behalf of Christian Grothoff. |
|