Port genPresets from WLED-MM #4735
Conversation
WalkthroughNew UI controls for generating and saving presets were added inside the "Info" modal in the HTML. Corresponding JavaScript functions were introduced to programmatically generate a presets JSON from existing effects, categorize them into playlists, display the JSON, and provide an option to save it by uploading to the server. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
wled00/data/index.htm (1)
334-336: Explicitly settype="button"+ add accessibility metadata to the new controlsThe new buttons are inside a
<div>that is not within a<form>, yet browsers still treat<button>without an explicittypeastype="submit"by default.
Although in practice submission is unlikely here, explicitly declaring the type avoids accidental form-submission side-effects if this HTML is ever embedded in a form container (e.g., via component reuse).While touching the element, consider small a11y & semantics polish:
-<button class="btn infobtn" id="genPresets" onclick="genPresets()">Generate presets</button> -<button class="btn infobtn" id="savePresetsGen" onclick="savePresetsGen()" hidden="true">Save generated presets</button> +<button class="btn infobtn" + id="genPresets" + type="button" + onclick="genPresets()" + aria-label="Generate demo presets and playlists"> + Generate presets +</button> +<button class="btn infobtn" + id="savePresetsGen" + type="button" + onclick="savePresetsGen()" + hidden + aria-label="Save the generated presets JSON to presets.json"> + Save generated presets +</button> -<textarea id="presetsGen" hidden="true"></textarea> +<textarea id="presetsGen" hidden aria-label="Generated presets JSON"></textarea>Key changes
•type="button"→ prevents implicit submit behaviour.
•hiddenas a boolean attribute (no value needed) for consistency.
•aria-label→ screen-reader clarity.
These tweaks cost nothing, future-proof the markup, and immediately improve accessibility.wled00/data/index.js (1)
3407-3419: Useapplication/jsonandgetURL()inuploadFileWithTextMinor polish:
- The payload is JSON, so advertise it:
- var blob = new Blob([text], {type : 'application/text'});
- var blob = new Blob([text], {type : 'application/json'});
When the UI is running from “file mode” the hard-coded
/uploadwon’t reach the device.
Re-use the existing helper to stay consistent:
- req.open("POST", "/upload");
- req.open("POST", getURL("/upload"));
These tweaks avoid MIME confusion and keep the function working in all hosting modes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/index.htm(1 hunks)wled00/data/index.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
wled00/data/index.js
[error] 3429-3431: Shouldn't redeclare 'savePresetsGen'. Consider to delete it or rename it.
'savePresetsGen' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Test cdata.js
wled00/data/index.js
Outdated
|
|
||
| function genPresets() | ||
| { | ||
| var result = ""; | ||
| var sep = "{"; | ||
|
|
||
| var effects = eJson; | ||
| var playlistPS = JSON.parse("{}"); | ||
| var playlistSep = JSON.parse("{}"); | ||
| var playlistDur = JSON.parse("{}"); | ||
| var playlistTrans = JSON.parse("{}"); | ||
| var playlistQL = JSON.parse("{}"); | ||
| function addToPlaylist(m, id, ql = undefined) { | ||
| if (!playlistPS[m]) playlistPS[m] = ""; | ||
| if (!playlistDur[m]) playlistDur[m] = ""; | ||
| if (!playlistTrans[m]) playlistTrans[m] = ""; | ||
| if (!playlistSep[m]) playlistSep[m] = ""; | ||
| playlistPS[m] += playlistSep[m] + `${id}`; | ||
| playlistDur[m] += playlistSep[m] + "100"; | ||
| playlistTrans[m] += playlistSep[m] + "7"; | ||
| playlistSep[m] = ","; | ||
| if(ql) playlistQL[m] = `${ql}`; | ||
| } | ||
| var seq=230; //Playlist start here | ||
| for (let ef of effects) { | ||
| if (ef.name.indexOf("RSVD") < 0) { | ||
| if (Array.isArray(fxdata) && fxdata.length>ef.id) { | ||
| let fd = fxdata[ef.id]; | ||
| let eP = (fd == '')?[]:fd.split(";"); // effect parameters | ||
| let m = (eP.length<4 || eP[3]==='')?'1':eP[3]; // flags | ||
| // console.log(ef, eP); | ||
| //transform key values in json format | ||
| var defaultString = ""; | ||
| //if key/values defined, convert them to json in defaultString | ||
| if (eP.length>4) { | ||
| let defaults = (eP[4] == '')?[]:eP[4].split(","); | ||
| for (let i=0; i<defaults.length;i++) { | ||
| let keyValue = (defaults[i] == '')?[]:defaults[i].split("="); | ||
| defaultString += `,"${keyValue[0]}":${keyValue[1]}`; | ||
| } | ||
| } | ||
| //if not defined set to default | ||
| if (!defaultString.includes("sx")) defaultString += ',"sx":128'; //Speed | ||
| if (!defaultString.includes("ix")) defaultString += ',"ix":128'; //Intensity | ||
| if (!defaultString.includes("c1")) defaultString += ',"c1":128'; //Custom 1 | ||
| if (!defaultString.includes("c2")) defaultString += ',"c2":128'; //Custom 2 | ||
| if (!defaultString.includes("c3")) defaultString += ',"c3":16'; //Custom 3 | ||
| if (!defaultString.includes("o1")) defaultString += ',"o1":0'; //Check 1 | ||
| if (!defaultString.includes("o2")) defaultString += ',"o2":0'; //Check 2 | ||
| if (!defaultString.includes("o3")) defaultString += ',"o3":0'; //Check 3 | ||
| if (!defaultString.includes("pal")) defaultString += ',"pal":11'; //Temporary for deterministic effects test: Set to 11/Raibow instead of 1/Random smooth palette (if not set different) | ||
| if (!defaultString.includes("m12") && m.includes("1") && !m.includes("1.5") && !m.includes("12")) | ||
| defaultString += ',"rev":true,"mi":true,"rY":true,"mY":true,"m12":2'; //Arc expansion | ||
| else { | ||
| if (!defaultString.includes("rev")) defaultString += ',"rev":false'; | ||
| if (!defaultString.includes("mi")) defaultString += ',"mi":false'; | ||
| if (!defaultString.includes("rY")) defaultString += ',"rY":false'; | ||
| if (!defaultString.includes("mY")) defaultString += ',"mY":false'; | ||
| } | ||
| result += `${sep}"${ef.id}":{"n":"${ef.name}","mainseg":0,"seg":[{"id":0,"fx":${ef.id}${defaultString}}]}`; | ||
| sep = "\n,"; | ||
| if(m.length <= 3) { | ||
| addToPlaylist(m, ef.id, m); | ||
| } | ||
| else { | ||
| addToPlaylist(m, ef.id); | ||
| } | ||
| addToPlaylist("All", ef.id, "ALL"); | ||
| if(ef.name.startsWith("Y💡")) addToPlaylist("AnimARTrix", ef.id, "AM"); | ||
| if (m.includes("1")) addToPlaylist("All 1D", ef.id, "1D"); | ||
| if (m.includes("2")) addToPlaylist("All 2D", ef.id, "2D"); | ||
|
|
||
| seq = Math.max(seq, (parseInt(ef.id) + 1)); | ||
| } //fxdata is array | ||
| } //not RSVD | ||
| } //all effects | ||
|
|
||
| // console.log(playlistPS, playlistDur, playlistTrans); | ||
| for (const m in playlistPS) { | ||
| if(!playlistQL[m]) playlistQL[m] = seq; | ||
| let playListString = `\n,"${seq}":{"n":"${m} Playlist","ql":"${playlistQL[m]}","on":true,"playlist":{"ps":[${playlistPS[m]}],"dur":[${playlistDur[m]}],"transition":[${playlistTrans[m]}],"repeat":0,"end":0,"r":1}}`; | ||
| // console.log(playListString); | ||
| result += playListString; | ||
| seq++; | ||
| } | ||
|
|
||
| result += "}"; | ||
|
|
||
| //assign result and show text and save button | ||
| gId("genPresets").hidden = true; | ||
| gId("savePresetsGen").hidden = false; | ||
| gId("presetsGen").hidden = false; | ||
| gId("presetsGen").value = result; | ||
| // console.log(result); | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Build the presets object first, then JSON.stringify – string-building is brittle
genPresets() manually concatenates JSON with lots of +=, new-line commas, and string escaping assumptions:
- An effect name containing
"or\will break the produced JSON. - Re-checking presence with
defaultString.includes("sx")is error-prone ("o1"matches"o10"). - Manual separators (
sep,playlistSep) are easy to mis-place and hard to maintain.
Consider:
-function genPresets() {
- /* huge string-building logic */
-}
+function genPresets() {
+ const presets = {};
+ const playlists = {};
+
+ // 1. Generate preset objects -------------------------------------------
+ eJson.forEach(([id, name]) => {
+ if (name.includes('RSVD')) return;
+
+ const p = { n: name, mainseg: 0, seg: [{ id: 0, fx: id }] };
+ Object.assign(p.seg[0], defaultFxParams(id)); // helper that returns the sx/ix/… object
+ presets[id] = p;
+
+ tagPlaylists(playlists, id, name); // helper that fills playlists hash
+ });
+
+ // 2. Convert playlists hash → preset-style objects ----------------------
+ let nextId = Math.max(...Object.keys(presets).map(Number)) + 1;
+ for (const [tag, { ps, dur, transition, ql }] of Object.entries(playlists)) {
+ presets[nextId] = {
+ n: `${tag} Playlist`,
+ ql,
+ on: true,
+ playlist: { ps, dur, transition, repeat: 0, end: 0, r: 1 }
+ };
+ nextId++;
+ }
+
+ // 3. Show result --------------------------------------------------------
+ gId('presetsGen').value = JSON.stringify(presets, null, 2);
+ gId('genPresets').hidden = true;
+ gId('savePresetsGen').hidden = false;
+ gId('presetsGen').hidden = false;
+}Benefits:
- Guaranteed valid JSON – no manual escaping.
- Logic is decomposed into helpers (
defaultFxParams,tagPlaylists) improving readability & testability. - Future fields are added by editing objects, not string replacements.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In wled00/data/index.js from lines 3309 to 3404, the genPresets function builds
JSON by manually concatenating strings, which risks invalid JSON if effect names
contain special characters and is error-prone due to substring checks and manual
separators. Refactor genPresets to first construct a JavaScript object
representing the presets and playlists, using helper functions for default
parameters and playlist tagging, then convert this object to a JSON string with
JSON.stringify. This ensures valid JSON output, simplifies the logic, and
improves maintainability.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
is this a feature that is useful for "daily use" or mainly for debugging? Not fully clear to me from the code what it actually can do. |
|
I would have preferred this as an external, uploadable file and not part of the standard build. |
Hm, could it be implemented as a usermod? |
Reading the description is more helpful than the code ;)
The issue with making a usermod is then you need to remember to add to every build It serves many purposes, both for formal testing and also useful for users to get a feel of what is supported e.g Want a playlist that has all the audio effects - you've got it, want a playlist that lets you see all the new PS effects, you've got it, wanting to run a soak test of all effects - you've got it |
|
You get a playlist for each "type" of effect, 1D/2D/2F etc All 2D, AnimARTrix |
|
Might be worth trying throwing it at copilot to see if it can refactor the JavaScript to make it more compact. As fewer WLED users use 2D than on MM night also be worth only creating the 2D items if you have a 2D setup |
Yes, that's exactly the goal. If I'm understanding @blazoncek correctly, he's raising an objection to including this feature in all builds (presumably on space usage grounds). Putting it in a module makes it easy to exclude if space is tight and you don't need it for your application. Personally I'd be happy to include this as a module in the default module set for our reference build. (Looking forward, I am still hoping to move any current core feature with a |
|
Sorry for late reply. What I wanted to say is that IMO generation of presets from existing effects is not what a regular user needs. So it does not belong into regular UI IMO. Unfortunately we cannot have UI built in a similar fashion as object code with WLED_DISABLE_XXXX compile parameter for C++, so some other method is needed. I'm also not sure if usermod is good way as it can't inject huge amount of text in info dialog. |
author [email protected]
Generate presets and playlists to act as demo mode or during testing
Summary by CodeRabbit