Add settings page to hide unwanted effects from the picker#5585
Add settings page to hide unwanted effects from the picker#55851saac-k wants to merge 2 commits into
Conversation
|
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}
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
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.
@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 + usermod effects, we are landing around 280 or more effects already. |
softhack007
added
the
needs_rework
@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 |
@1saac-k maybe the better solution is to create a list of favourite effects, instead of explicitly marking effects as "hidden". This would allow for a simpler UX, for example users could "star" favourite effects (directly in the list), and then switch between "all" and "favourites" with a button above or below the effect list. Maybe we could also pre-sort the list so that starred effects come first (optional), which would help HomeAssistant users, too. |
|
The RAM: [====== ] 56.8% (used 46536 bytes from 81920 bytes)
Error: The program size (894155 bytes) is greater than maximum allowed (892912 bytes)
*** [checkprogsize] Explicit exit, status 1
Flash: [==========] 100.1% (used 894155 bytes from 892912 bytes)compared to 17.0.0 nightly: RAM: [====== ] 56.8% (used 46504 bytes from 81920 bytes)
Flash: [==========] 99.8% (used 891379 bytes from 892912 bytes) |
|
@softhack007 Thank you for the quick and thorough review.
Yes, once the effect id is expanded to 16 bits, we can easily manage more than 256 effects by extending the bitmask array. If we want to support 65,536 effects—setting aside the issue of insufficient ROM to store the effects—we would just need to grow the current bitmask array from 8 to 2048. In practice, I expect there will be a count limit (e.g., 1K, 2K, ...), so the bitmask array shouldn't grow very large.
I apologize for violating WLED's contribution rules, and from now on I'll only add additional commits.
Using favourite instead of hidden also works fine for my use case, and I can accept that approach.
The reason I implemented it with two lists was that I had reordering in mind, but I agree that setting favourites directly from the effect picker list—rather than from a separate settings page—provides a simpler UX. I think that's a good idea. I'll give this approach a try.
I think this would be difficult to implement with the current
Once the design (hide vs. favourite, etc.) is finalized, I'll rework the code to reduce its size and see if it can fit on esp01_1m. Question: what happens if I can't reduce it enough? Would it be acceptable to disable the hide/favourite feature on esp01_1m, or something along those lines? (I'm curious about the scope of esp01_1m support.) |
Yes putting the feature between |
It is customizable.
|
|
I'll hard veto any approach using a bitfield. If we use a numeric ID space at all after expanding past 255, it'll be sparsely populated with different regions allocated to different components. Bitfields are a no-go. Instead, use a list or vector where the list itself can be dynamically allocated, so the memory cost of people not using this feature is kept as small as possible (ie. a single pointer). To be honst, I'm mildly philosophically opposed to spending our code and memory resources on hiding features from integrations. I could accept a "UI preferences" feature that kept UI-relevant-only state in a separate file, but requiring a permanent runtime memory cost on all users to support this seems wasteful to me. |
4 participants
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