Add settings page to hide unwanted effects from the picker#5585
Open
Add settings page to hide unwanted effects from the picker#5585
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
WalkthroughAdds per-effect hiding: a 256-bit hidden mask, config persistence, exclusion from mode selection, optional API hiding, a new Effect Visibility settings page, and build integration for the page. ChangesEffect Visibility / Hiding
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Server
participant Config
participant Bitmask as hiddenFxMask
Browser->>Server: GET /settings/fx (loads PAGE_settings_fx)
Browser->>Server: GET /json/effects?all=1
Browser->>Server: GET /json/cfg
Server-->>Browser: effects list + cfg (includes fx.hidden)
Browser->>Browser: render Visible/Hidden lists
Browser->>Server: POST /settings (FXH=ids...)
Server->>Bitmask: reset and setFxHidden(ids)
Server->>Config: serializeConfig() writes fx.hidden
Server-->>Browser: save response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1saac-k
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/wled.h (1)
433-436: ⚡ Quick winEnforce Solid protection directly in
setFxHidden().
setFxHidden()currently allowsid == 0, so the “Solid can’t be hidden” rule depends on every caller getting it right. Guarding it here makes the invariant global and future-proof.Suggested hardening
inline bool isFxHidden(uint8_t id) { return (hiddenFxMask[id >> 5] >> (id & 31)) & 1u; } -inline void setFxHidden(uint8_t id, bool hide){ if (hide) hiddenFxMask[id >> 5] |= (1u << (id & 31)); - else hiddenFxMask[id >> 5] &= ~(1u << (id & 31)); } +inline void setFxHidden(uint8_t id, bool hide){ + if (id == 0) return; // Solid must always stay visible + if (hide) hiddenFxMask[id >> 5] |= (1u << (id & 31)); + else hiddenFxMask[id >> 5] &= ~(1u << (id & 31)); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/wled.h` around lines 433 - 436, setFxHidden currently permits hiding effect id==0 ("Solid"), leaving protection to callers; modify setFxHidden(uint8_t id, bool hide) to enforce Solid protection inside the function by ignoring any attempt to hide id==0 (e.g., if id==0 treat hide as false or return early). Use the existing symbols (setFxHidden, hiddenFxMask, isFxHidden) to locate the logic and ensure the bitmask is only modified for ids != 0 so the invariant "Solid can't be hidden" is enforced globally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/data/settings_fx.htm`:
- Around line 84-93: The fx items are rendered as non-focusable divs so keyboard
users can't activate toggleSel; make each element with class 'fxitem' focusable
and keyboard-operable by adding tabindex="0", an appropriate ARIA role (e.g.,
role="button") and key handlers that call toggleSel on Enter/Space, and ensure
the existing click handler remains; update the same pattern used at the other
blocks (lines that create li with class 'fxitem' and set li.onclick) so they
also set li.tabIndex = 0, li.setAttribute('role','button') and add a keydown
listener that invokes toggleSel(li) when event.key is "Enter" or " " (Space);
also ensure the 'solid' item remains non-interactive (no tabindex/key handler)
and retains its title.
---
Nitpick comments:
In `@wled00/wled.h`:
- Around line 433-436: setFxHidden currently permits hiding effect id==0
("Solid"), leaving protection to callers; modify setFxHidden(uint8_t id, bool
hide) to enforce Solid protection inside the function by ignoring any attempt to
hide id==0 (e.g., if id==0 treat hide as false or return early). Use the
existing symbols (setFxHidden, hiddenFxMask, isFxHidden) to locate the logic and
ensure the bitmask is only modified for ids != 0 so the invariant "Solid can't
be hidden" is enforced globally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03f3b360-956c-4e5f-a0e3-33a08ca9a6a4
📒 Files selected for processing (11)
tools/cdata.jswled00/FX_fcn.cppwled00/cfg.cppwled00/const.hwled00/data/settings.htmwled00/data/settings_fx.htmwled00/fcn_declare.hwled00/json.cppwled00/set.cppwled00/wled.hwled00/wled_server.cpp
Comment on lines
+84
to
+93
| const li = d.createElement('div'); | ||
| li.className = 'fxitem'; | ||
| li.dataset.id = e.id; | ||
| li.textContent = e.name; | ||
| if (e.id === 0) { | ||
| li.classList.add('solid'); | ||
| li.title = 'Solid cannot be hidden'; | ||
| } else { | ||
| li.onclick = () => toggleSel(li); | ||
| } |
Contributor
There was a problem hiding this comment.
Make effect selection keyboard-operable (currently mouse-only).
Line 84+ renders selectable effects as non-focusable <div> elements, so keyboard users can’t complete hide/show operations.
♿ Suggested fix
function render() {
const visList = gId('visList');
const hidList = gId('hidList');
+ visList.setAttribute('role', 'listbox');
+ visList.setAttribute('aria-multiselectable', 'true');
+ hidList.setAttribute('role', 'listbox');
+ hidList.setAttribute('aria-multiselectable', 'true');
visList.innerHTML = '';
hidList.innerHTML = '';
@@
const li = d.createElement('div');
li.className = 'fxitem';
+ li.setAttribute('role', 'option');
+ li.setAttribute('aria-selected', 'false');
li.dataset.id = e.id;
li.textContent = e.name;
if (e.id === 0) {
li.classList.add('solid');
+ li.tabIndex = -1;
li.title = 'Solid cannot be hidden';
} else {
+ li.tabIndex = 0;
li.onclick = () => toggleSel(li);
+ li.onkeydown = (ev) => {
+ if (ev.key === 'Enter' || ev.key === ' ') {
+ ev.preventDefault();
+ toggleSel(li);
+ }
+ };
}
@@
function toggleSel(li) {
- li.classList.toggle('sel');
+ const selected = li.classList.toggle('sel');
+ li.setAttribute('aria-selected', selected ? 'true' : 'false');
updateBtns();
}Also applies to: 102-112, 163-177
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wled00/data/settings_fx.htm` around lines 84 - 93, The fx items are rendered
as non-focusable divs so keyboard users can't activate toggleSel; make each
element with class 'fxitem' focusable and keyboard-operable by adding
tabindex="0", an appropriate ARIA role (e.g., role="button") and key handlers
that call toggleSel on Enter/Space, and ensure the existing click handler
remains; update the same pattern used at the other blocks (lines that create li
with class 'fxitem' and set li.onclick) so they also set li.tabIndex = 0,
li.setAttribute('role','button') and add a keydown listener that invokes
toggleSel(li) when event.key is "Enter" or " " (Space); also ensure the 'solid'
item remains non-interactive (no tabindex/key handler) and retains its title.
The guard in handleSettingsSet was `subPage > 10`, which silently rejected SUBPAGE_PINS (11) and any future subPages. SUBPAGE_PINS happens to be a GET-only page so the mismatch never surfaced; referencing SUBPAGE_LAST keeps the guard in sync as new subPages are added.
Lets users hide individual effects so they don't clutter the effect picker. Hidden state persists in cfg.fx.hidden and takes effect immediately. Effect IDs are not reshuffled, so existing presets, playlists, and external automations keep parsing without errors. References to a hidden effect fall through to the next visible effect until it's un-hidden. Implementation reuses the existing RSVD placeholder convention (already used for build-disabled and deprecated effect slots): a 32-byte bitmap (uint32_t hiddenFxMask[8]) is consulted at the same sites that already handle RSVD: - serializeModeNames emits "RSVD" when isFxHidden(id) - Segment::setMode skips hidden effects alongside true reserved slots Existing clients (web UI effect picker, HA, python-wled, etc.) thus treat user-hidden effects exactly like RSVD without any changes. /json/effects is byte-identical to prior firmware when nothing is hidden; once any effects are hidden, those ids surface as "RSVD" through the existing filter path. _modeData is never modified, so original names remain recoverable. A new opt-in /json/effects?all=1 query returns real names for user-hidden effects so the un-hide UI can show what's currently hidden. Solid (id 0) is protected -- never user-hidable. UI: new /settings/fx page with stacked Visible/Hidden lists, multi-select, Hide / Show move buttons, and per-list All/None batch selection.
Member
@1saac-k (early feedback) a mask with 256 bits will probably not be future - proof. We are discussing to allow more than 254 effects, and one approach is to widen the 8bit effect ID to 16bit. Maybe re-think your proposal so it's not limited to 256 effects. Usermods can add effects, and with all 1D and 2D effects we have + usermod effects, we are landing around 280 or more effects already. |
softhack007
added
the
needs_rework
Member
@1saac-k please do NOT force-push while you have a PR open. We will squash-and-merge your PR, so just add commits when you have something new. https://github.com/wled/WLED?tab=contributing-ov-file#during-review |
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.
Background
This PR resolves #1262. I felt the same itch as a user myself: of the 220+ effects, I only regularly use a dozen or so, while the rest just add noise to the picker. The ability to trim the visible effect list per user is also useful for external integrations — particularly automation systems like Home Assistant that surface WLED effects as a dropdown.
Alternatives discussed in #1262
The issue was opened back in 2020, and many workarounds and alternatives have been proposed in the comments since. Summarizing:
/json/fxdataalready exposes some capability metadata, which clients like HA could use to filter. However (a) every external integration would have to ship a metadata-aware patch, and (b) it can't express arbitrary per-user preferences ("I just don't like any strobe-style effect").This PR aims to fill the two gaps that the alternatives above each only partially address: (a) the firmware itself being able to remove unwanted effects from the picker, and (b) a single mechanism that applies uniformly to external integrations (HA, python-wled, etc.).
Approaches considered
Reframing the discussion above from an implementation perspective, the candidates I considered:
index.htmapply a user-specified list when drawing the effect picker. Small change footprint, but the JSON API still exposes everything, so HA and other integrations don't benefit. State sharing across browsers on the same device is also awkward (localStorage limits)._modeData(actual deletion) — The most direct option, but every subsequent ID shifts down, breaking every preset / playlist / automation that references an ID. Not realistic compatibility-wise._modeData[id] = _data_RESERVED(destructive RSVD swap) — Reuses the existing RSVD slot convention. The externally observable behavior is clean, but the original name is lost, which makes building an unhide UI structurally impossible.uint32_t hiddenFxMask[8](32 bytes) as the source of truth and add a one-lineisFxHidden(id)check at the two sites that already test for RSVD (serializeModeNames,Segment::setMode)._modeDatais never modified.Other changes
This PR contains two commits:
subPage guard fix — Replaces the hardcoded
subPage > 10guard inhandleSettingsSetwith aSUBPAGE_LASTreference. I found it while working on this feature, but it has actually been latent sinceSUBPAGE_PINS=11was added (SUBPAGE_PINS is GET-only, so it never tripped the form-POST guard and the bug went unnoticed). It stands on its own as a cleanup, so I split it into a separate commit.Per-effect hide feature — Bitmap, runtime checks,
/settings/fxpage,/json/effects?all=1.Limitations and possible follow-ups
Help link target not yet documented
The
?button on the settings page links tofeatures/effects/#effect-visibilityon kno.wled.ge, which doesn't exist yet.A structural limitation:
/json/effectsresponses still contain RSVDWorth flagging separately from this PR, an existing API constraint.
/json/effectsreturns a flat array of effect names where the array index is the effect ID — because the API used to set an effect (/json/state'sseg.fx) is a numeric ID. So if the server were to drop RSVD slots from the response, clients could no longer derive the ID from the index. As long as the index = ID convention holds, RSVD slots have to remain in the response.WLED's own
index.htmfilters out the RSVD name on the client side, so users never see it. Other clients vary. In particular, Home Assistant's WLED integration and the [python-wled](https://github.com/frenck/python-wled) wrapper it uses do no such filtering, so "RSVD" entries leak into HA's effect dropdown — a UX defect.There are three possible directions to fix this:
/json/effects-v2that returns[{id, name}, ...]. Breaks the index dependency, so RSVD can be omitted safely.As a first step, I've opened an issue against python-wled: frenck/python-wled#2053.
Effect reordering
This PR only addresses hide, not reorder, but the two-list (Visible / Hidden) layout of the settings page was chosen with reorder in mind as a future addition. Reorder runs into the same
/json/effectsindex = ID constraint — it can't be exposed cleanly server-side until the API moves to id-name pairs as described above.Screenshots
New settings page. Open to feedback on consolidating it into an existing page (e.g., User Interface).
Select All, then deselect 4 effects.
Hide the selected effects.
Effect picker after save.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior