Inline warning-confirm directives (;warning, ;warning_color, ;warning_icon, ;accept, ;hold_seconds) - #310
Open
rashevskyv wants to merge 24 commits into
Open
Conversation
…PACKAGE_PATH to handleCommandHold
Phase 1 (overlay-only). Adds three INI-level directives that, when set on a list item, expand a multi-line warning banner + a hold-A Accept button directly under the item instead of executing immediately. Holding A on Accept to completion runs the original action through the existing handleCommandHold pipeline; pressing B (or activating another warning-armed item) collapses the expansion. Multi-line text uses single-line syntax with backslash-n escape sequences (e.g. ;warning=Line 1\nLine 2). Triple-backtick fence syntax is deferred to a Phase 2 libultrahand parser change. Toggle items support direction-specific texts via ;warning_on= / ;warning_off=; ;warning= alone applies to both directions. Toggle visual state and config.ini value are flipped only after Accept-hold completion, and reverted on cancel. Signed-off-by: Devin <devin@cognition.ai> Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…erride
* Crash on Accept-hold completion: collapse() was clearing
runningInterpreter and lastSelectedListItem inside onComplete, racing
the interpreter thread and producing a use-after-free in the next
frame's handleInterpreterCompletion. Split the helper into two:
- collapseUI(): UI-only removal; nulls lastSelectedListItem if it
still points at the (about-to-be-deleted) Accept item.
- collapse(): UI removal + full hold-pipeline reset, used for
B-cancel and single-active-rule resets.
onComplete now calls collapseUI(); B-cancel keeps using collapse().
* Warning glyph: the bundled font lacks U+26A0 so it rendered as a red
crossed square. Replaced with a custom-drawn filled yellow triangle
+ dark exclamation mark, painted via drawLine + drawRect inside the
banner's CustomDrawer.
* New ;accept=TEXT directive lets package authors override the default
'Hold A to confirm' Accept-button label. Same parsing pipeline as
;warning= (quotes stripped, \n / \t escapes decoded).
* Banner content is indented (left margin 20 px instead of 12 px) and
the Accept item label is prefixed with two spaces so both elements
visually nest under the source item. Removes the gap between banner
and Accept by leaving them as immediately adjacent list rows.
* Banner content fades in over ~150 ms via per-channel alpha scaling
computed from armTicksToNs(now - expandStartTick).
Signed-off-by: Devin <devin@cognition.ai>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Prior fix split collapse into collapseUI() (UI removal) and collapse() (full reset) and called collapseUI() from the Accept-hold onComplete. Removing the Accept item there races with the running interpreter and the next-frame handleInterpreterCompletion(), which still tries to update the Accept item with CHECKMARK/CROSSMARK. The result was a Data Abort at offset 0x28 (m_width on a freed Element). Defer the UI removal: onComplete now sets g_pendingCollapse instead of mutating the list. Each handleInput() call site that consumes lastRunningInterpreter calls WarningConfirm::consumeDeferredCollapse() AFTER the completion update has been applied, so the Accept ListItem is removed from m_items only once it is guaranteed to be unused by the rest of the pipeline.
Crash log v3 (LR=0xbebebebebebebebe poison) showed the next handleInput
frame deref'd the freed Accept ListItem via tsl::Gui::m_focusedElement.
List::removePendingItems() decrements its internal m_focusedIndex when an
item is erased, but it does NOT touch the Gui-level raw m_focusedElement
pointer. After our deferred-collapse path deleted Accept, that pointer
went dangling and the very next frame's
for (Element* p = currentFocus; p; p = p->getParent())
handled = p->onClick(...) || p->handleInput(...);
loop crashed inside p->onClick().
Fix: in collapseUI(), BEFORE queueing the banner+Accept removal, move
focus back to the source item via tsl::Gui::requestFocus + List::
setFocusedIndex. After the move, m_focusedElement points at the
still-live source item, m_focusedIndex points at its index, and
List::removePendingItems' subsequent decrement keeps both consistent
once Accept and banner are erased.
Also: actually wire the previously-added requestDeferredCollapse() into
the Accept-hold onComplete callback (the prior commit added the helper
but kept onComplete calling collapseUI() synchronously, which still
raced with handleInterpreterCompletion's CHECKMARK / CROSSMARK update).
With this commit, onComplete only marks the deferred flag, and
consumeDeferredCollapse() at the 5 interpreter-completion sites runs
the actual removal AFTER the completion-update pass.
UX feedback: cursor should land directly on the Accept item once the banner is expanded, so the user does not have to scroll past the source item and the banner to reach it. This also fixes the secondary 'last item misbehaves' report — when the source is the last item, banner+ Accept get appended past the visible viewport; setFocusedIndex on Accept routes through List::updateScrollOffset which centers the viewport on Accept, so it always becomes visible after expand. Direct requestFocus() in expand() is a no-op because List::requestFocus() returns nullptr while m_itemsToAdd is non-empty (banner+Accept have not yet been moved into m_items by the next List::draw -> addPendingItems pass). Defer the transfer: - expand() ends by calling requestFocusToAccept() which sets g_pendingFocusToAccept = true. - PackageMenu / MainMenu handleInput() call consumePendingFocusToAccept() at the very top. On the first frame after expand, Accept is now in m_items, so getIndexInList() returns a valid idx; we then call list->setFocusedIndex(idx) + gui->requestFocus(acceptItem, None, false) and clear the flag. If Accept is somehow still pending, we just wait for the following frame.
…rride UX polish based on user feedback after first stable test of the inline warning-confirm panel: - Collapse no longer snaps off in a single frame. Both B-cancel and the post-Accept-hold deferred dismissal now route through beginCollapseAnim(); the banner CustomDrawer's alpha multiplies the existing fade-in by a 220 ms fade-out, and tickCollapseAnim() runs once per handleInput frame to drop banner+Accept after the fade. Single-active-rule replacement still uses an immediate collapse so the new banner does not visually overlap with a fading old one. - Banner content indent bumped from 20 px to 36 px, and the Accept label prefix widened from two to four spaces, so banner+Accept clearly nest under their originating list item. - New `;hold_seconds=N` per-item directive (float). Parsed alongside `;hold=` in case 'h', threaded through WarningConfirm::expand() as a trailing parameter, captured by the Accept item's click listener, and consumed by processHold() via a static holdDurationMsOverride that overrides ult::holdDurationMs for that hold only. Reset to 0 on release / completion / cancel so unrelated holds keep the global default.
Forward-declaration in the WarningConfirm namespace at the top of main.cpp already supplies the default 'animate = true', so the definition further down must take a bare 'bool animate'. GCC errors out under -fpermissive when both sites carry the default.
- New WarningAcceptListItem ListItem subclass. Its draw() overlays a
4px-wide vertical accent strip at the banner's indent on top of the
base ListItem render, using g_accentColor and the same expand/collapse
alpha curve so banner+Accept appear as a single connected panel.
- Two new directives:
;warning_color=#RRGGBB (or RRGGBB) — accent bar + icon tint.
Defaults to theme yellow.
;warning_icon=triangle|info|error|none — icon shape next to text.
Defaults to triangle.
Parsed in case 'w' before warning_off/warning_on/warning (longest
prefix first), threaded through WarningConfirm::expand() as trailing
parameters, and captured in the banner CustomDrawer lambda.
- Icon rendering refactored to a switch on IconKind:
Triangle (existing) — filled isosceles + dark '!'.
Info — filled circle + dark 'i' (dot + short stem).
Error — filled circle + dark crossed 'X'.
None — skip glyph entirely; text starts right after accent strip.
Banner indent constant (36 px) and accent width (4 px) factored to
namespace constants so the banner and the Accept overlay use the
exact same X coordinate, preventing visible seams.
- New behaviour: pressing A on the source list-item while its warning is already expanded now collapses the banner+Accept (instead of trying to re-expand them). Implemented for both default-mode click listeners (uses isActiveFor(listItem) before calling expand) and toggle-mode stateChangedListener (also reverts the toggle's visual flip that the press just triggered). Adds WarningConfirm::isActiveFor(item) which checks the cached g_sourceItem pointer. - WarningAcceptListItem now stores its accent color as a member instead of reading the namespace-level g_accentColor every frame. expand() copies parseHexColor(accentHex,...) directly into m_accentColor before inserting the item into the list. This avoids the (theoretical) race where the global is overwritten by a subsequent expand() before the Accept's draw() runs. - One-shot ult::logMessage in expand() that records the raw accentHex and iconName strings to /switch/.packages/log.txt, so we can verify what the per-item parser is actually feeding into expand() if colors don't appear to change in a build.
Temporary diagnostic commit. All three are intentionally garish and
will be removed once the underlying issue is understood.
1. parseHexColor fallback inside expand() returns bright cyan instead
of theme warningTextColor. If banners now show cyan instead of
red/yellow, the new code path is executing and parseHexColor is
simply receiving an empty accentHex.
2. WarningAcceptListItem::draw() now also overlays a 6 px tall bright
magenta rectangle along the top of the Accept row. If this is
visible, the override IS being called by the List render loop. If
the magenta rect is NOT visible, the vtable / build is stale.
3. logMessage() (which routes to per-package log.txt and can be
confusing) is replaced by direct fopen+fprintf to a fixed path
sdmc:/config/wc-debug.log. Lines logged:
[WC] parse 'w' commandName='<full command>'
[WC] matched warning_color='<value>'
[WC] expand accentHex='<hex>' iconName='<name>' keyName='<key>'
The user will rebuild, install and reproduce, then ship back the log
file. From that we can tell exactly which of three failure modes is
hitting (no parse / parse no match / parse matches but render ignores).
The previous diagnostic commit confirmed (via screenshot) that:
- ;warning_color= parsing is wired through correctly.
- WarningAcceptListItem::draw() override is invoked by the List.
- Accent strip on Accept renders at the same X as the banner.
Strip both probes back out and tighten the rendering:
- parseHexColor fallback restored to tsl::warningTextColor.
- Magenta probe rectangle on Accept removed; the Accept strip is now
drawn at full row height (getY() .. getY()+getHeight()) so it visually
joins the banner's strip with no gap.
- Banner strip likewise extended to full row height (y .. y+h) for the
same seamless join.
- ult::logMessage and fopen("sdmc:/config/wc-debug.log") debug writes
removed; expand() and case 'w' parsing are silent again.
ListItem::layout() in libtesla bumps its own X by +3 relative to its parent List (tesla.hpp:7390 'this->setBoundaries(this->getX() + 3, ...)'), while CustomDrawer leaves getX() at the raw List X. As a result, Accept's accent strip was rendering 3 px to the right of the banner's accent strip — visible in screenshots as a horizontal jog of roughly one strip-width at the banner/Accept boundary. Compensate explicitly inside WarningAcceptListItem::draw() so the two strips line up at the same screen X. Documented as a constant LIST_ITEM_X_OFFSET so the magic number is searchable if the libtesla internals change later.
User report: collapse looks jumpy because Accept's bright focus ring stays at full opacity while banner+Accept content fades out, and then the highlight pops onto the source item the moment the items are removed. Sometimes the visible jump is small (focus already centered on source), sometimes large (focus stayed on Accept and snaps back to source). Fix: clear the m_focused flag on Accept (and defensively on banner) the instant the collapse animation starts. Gui's m_focusedElement pointer still references Accept (so onClick / handleInput routing is unchanged), but Element::frame() now skips both drawFocusBackground() and drawHighlight() because m_focused is false. Visual result: the banner+Accept simply dissolve, with no focus ring visible during the 220 ms fade. Once the animation finishes, the existing collapseUI() path calls gui->requestFocus(g_sourceItem, ...), which sets the source's m_focused = true and triggers the standard click-animation reset. libtesla draws the focus background and highlight using its built-in pulse animation, so the highlight smoothly fades back in on the original row. Also add an isCollapsing() predicate and swallow all input in PackageMenu::handleInput and MainMenu::handleInput while the animation is mid-flight. Without this, the player could press A again during the fade, retrigger the hold (Accept's click listener is still wired), and crash when the listener completes after Accept has been removed. The 220 ms lock is brief enough that B-cancel / navigation feel unaffected in practice.
User reported (with screenshot) that the accent strip paints over the source item's blue focus halo where they meet. Root cause: libtesla's drawBorderedRoundedRect renders the bottom edge of the focus halo at startY + adjustedHeight (= sourceY + sourceH + 1), with thickness = 5 px, so 5 px of the focused source's halo bleed DOWN into the banner row. Items render in list order, so the banner is drawn AFTER the source -- meaning the strip is on top of the source's halo at that overlap. Same logic applies to the bottom of Accept if the user navigates focus to the item directly below. Skip the first 6 px (5 thickness + 1 offset) of the banner strip and the last 6 px of the Accept strip. The strips still meet at the banner/Accept boundary with no gap (only the outer ends are inset), and any focus halo that bleeds into those insets now sits on top where the user expects it.
…nified libtesla's ListItem::draw paints a 1 px gray separator line at topBound (tesla.hpp:7350) whenever the previous item's bottomBound didn't match. The banner sits between the source ListItem and Accept, and because banner is a CustomDrawer (not a ListItem), the 'lastBottomBound' static stays at the source's bottom -- so when Accept renders next, its top separator gets drawn, producing the visible divider between the warning text and Accept that the user circled. Overwrite that line with tsl::defaultBackgroundColor right after the base ListItem::draw call and before the accent strip is rendered. Now the strip is drawn over the (erased) pixel, so it remains continuous, and the banner + Accept read as one cohesive warning panel with no horizontal divider between them.
…tore
User reported that after the fade-out completes, the blue focus ring
'pops' onto the source item -- which itself may still be sliding to
its final scroll position because List had to recompute m_offset and
m_listHeight after removing banner+Accept. The visible jump of the
ring while the row is still moving reads as chaotic, especially when
the collapsed warning was near the bottom of a long list with the
scrollbar engaged.
Add a brief post-collapse 'settle' window (250 ms):
1. collapseUI() finishes its existing work (move Gui focus to source,
remove banner+Accept).
2. Before clearing globals, capture the source as g_settleTarget,
record g_settleStartTick, and call g_settleTarget->setFocused(false)
so the source's m_focused is OFF the instant items are removed.
Element::frame() now skips drawFocusBackground/drawHighlight, so
no ring is visible during the layout reflow.
3. tickSettle() polls every frame from PackageMenu / MainMenu
handleInput. Once SETTLE_ANIM_NS (250 ms) elapses it checks that
Gui's focused element is still the settle target (user might have
navigated away or started a new warning) and only then calls
setFocused(true). setFocused() resets m_clickAnimationProgress,
so libtesla's built-in pulse + drawHighlight animates the ring in
smoothly rather than popping.
4. expand() clears any in-flight settle state right after the
single-active-rule collapse, so a new warning's Accept focus is
never fighting an old settle that would otherwise flip the
previous source's m_focused=true behind us.
Input remains responsive during settle (only the highlight is hidden),
because tickSettle is robust against the user navigating away or
opening a new warning during the window.
Contributor
Author
|
Owner
|
this seems to change quite a bit of stuff. before incorporating any changes, i always pay close consideration to the compilation size changes to ensure 4mb support and that no packages break from our tight buffer sizes. id have to do a deeper review of this before considering it. the other pr will take me some time to review too. any reason you are using the very old version of nx-ovlloader? |
Owner
|
all your changes will have to be considered after the 2.4.2 release. ive changed some stuff in comparison to 2.4.1, so its helpful to have a stable point before changing this much more other stuff. but I will definitely review everything |
Contributor
Author
I accidentally kept using the WerWolv version. I'm currently in the process of porting Kefir to Ultrahand, so I missed a few things. Thanks for flagging this — I probably wouldn't have caught it on my own for a while |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an inline warning/confirmation UI for package / main menu items. When a list item carries a
;warning=,;warning_on=or;warning_off=directive, pressing A does NOT immediately execute. Instead a multi-line warning banner is spliced into the same list directly under the item, followed by a hold-A "Accept" button. The original command runs only after Accept is held to completion. Collapse happens on B, on a second A-press on the source row, on activating another warning-armed item, or automatically after the Accept-hold finishes — always with a smooth fade-out and a 250 ms post-collapse settle window so the source row's focus highlight does not flash while libtesla recomputes scroll layout.All work lives in
Ultrahand-Overlay/source/main.cpp; the onlylibultrahanddependency isult::holdDurationMs, which is added by the companion PR #309's libultrahand dependency (PR #16).Stacking notes
This PR is stacked on top of PR #309 (
Toggle hold improvements). Until #309 merges, this PR's diff will include the same toggle/hold changes — they are not duplicate work, they are the same commits that PR #309 carries. Once #309 lands onmain, this PR's diff collapses to just the warning-confirm code. Companion submodule pointer here matches PR #309's expectation (rashevskyv/libultrahandat the SHA that #309 also pins; once libultrahand PR #16 merges, the pointer will be flipped to the mergedppkantorski/libultrahand:mainSHA).Directives
;warning=TEXT;warning_on=TEXT;warning=if absent);warning_off=TEXT;warning=if absent);warning_color=#RRGGBB;warning*directive#RRGGBBorRRGGBB; falls back to theme yellow on parse error;warning_icon=NAME;warning*directivetriangle(default),info,error,none. Aliases:warning,i,x/danger,off;accept=LABEL;warning*directiveAccept);hold_seconds=N;warning*directive1.5); falls back to the globalult::holdDurationMswhen absentMulti-line text uses single-line syntax with backslash-n escapes:
Visual & UX polish
m_offset/ recomputesm_listHeight/ runs its scroll animation. ThensetFocused(true)resets libtesla'sm_clickAnimationProgressso the highlight ring fades in smoothly. Robust against the player navigating away or opening a new warning during the window.Implementation overview
WARNING_*,ACCEPT_PATTERN,HOLD_SECONDS_PATTERN, parsed in thecase 'w':/case 'a':/case 'h':arms of the directive switch indrawCommandsMenu()(used by bothSelectionOverlayandPackageMenu). Longest-match-first ordering so;warning_color=/;warning_icon=/;warning_off=/;warning_on=are parsed before plain;warning=.unescapeWarningText()converts\n,\t,\\into real bytes.namespace WarningConfirmholds runtime state and helpers (expand/collapse/collapseUI/requestDeferredCollapse/consumeDeferredCollapse/tickCollapseAnim/tickSettle/requestFocusToAccept/consumePendingFocusToAccept).WarningAcceptListItem(subclass oftsl::elm::ListItem) paints its share of the accent strip and erases the stock top separator.;hold=trueitems;holdDurationMsOverrideis read byprocessHold()for the;hold_seconds=case before falling back toult::holdDurationMs.SelectionOverlay::handleInput,PackageMenu::handleInput, andMainMenu::handleInput.libultrahandchanges beyond what PR Toggle hold improvements: configurable hold duration, hold-cancel parity, helper hardening #309 / PR Temporary Revert "Development for v1.3.1" #16 already add.Example test package
The following
package.iniexercises every directive and every combinationused during development (default-mode + toggle, with / without
;hold=true,colour, icon, accept label, custom hold duration, last-item-in-scrolled-list,
single-active replacement):
Review & Testing Checklist for Human
git fetch origin git checkout devin/inline-warning-confirm git submodule update --init --recursive make clean && make;warning=: pressing A spawns banner + Accept with cursor on Accept; A-hold runs the action; banner + Accept fade away after completion. B before completion collapses smoothly.;warning_on=/;warning_off=: clicking the toggle does NOT flip its visual state until Accept-hold finishes; on B-cancel the toggle does not flip;config.inionly updates on successful hold.;warning_color=#FF3030,;warning_icon=error,;accept=Yes,;hold_seconds=0.5,;hold_seconds=3all work as expected.Notes
\nescape syntax is sufficient for current use.