Reorder Preset IDs #3929
Reorder Preset IDs #3929
Conversation
There was a problem hiding this comment.
Thanks for the contribution but I do have some reservations.
First off, this needs to be based on 0_15 branch, not main.
Next, I do not see any API (JSON or otherwise) accompanying this "groundowrk", I would require at least JSON API to be present.
The other thing is inefficiency of reading entire preset into temporary buffer (which is inadequate BTW). As the presets should not change during rearrangement there is no need to read them into memory instead only ID should be rewritten inside presets.json (if the ID will not fit then make a copy using existing behaviour).
| // Function to find the preset ID by its name | ||
| int findPresetIdByName(const char* targetName) { | ||
| String currentName; | ||
| byte maxPresetId = 251; |
There was a problem hiding this comment.
Maximum allowable preset ID is 250, not 251.
|
|
||
| // Check if the IDs are the same or invalid | ||
| if (oldId == newId) { | ||
| //Serial.println(F("Old ID and New ID are the same. No changes required.")); |
There was a problem hiding this comment.
I would require using DEBUG_PRINT macros or remove these comments.
|
|
||
| // Determine the highest ID to dictate array size | ||
| int highestId = max(oldId, newId); | ||
| StaticJsonDocument<4096> *tempPresets = new StaticJsonDocument<4096>[highestId + 2]; |
There was a problem hiding this comment.
Why StaticJsonDocument? For dynamic allocations you should use PSRAMDynamicJsonDocument.
The other thing is the limit to 4k. That's not enough. At minimum it should be the size of JSON_BUFFER_SIZE. Do not forget, you can store PixelMagic images inside preset.
| String presetName; | ||
| if (!getPresetName(i, presetName)) { | ||
| //Serial.println("Error reading preset name for ID: " + String(i)); | ||
| presetName="ERROR READING NAME"; |
There was a problem hiding this comment.
Not all presets have name. Some do not use name at all.
| tempPresets[newId] = tempPresets[highestId+1]; | ||
|
|
||
| // Save all modified presets back to the filesystem | ||
| for (int i = 1; i <= highestId; i++) { |
There was a problem hiding this comment.
That is very inefficient and prone to flash corruption we are fighting since 0.11.
| @@ -81,7 +171,8 @@ static void doSaveState() { | |||
|
|
|||
| bool getPresetName(byte index, String& name) | |||
| { | |||
| if (!requestJSONBufferLock(9)) return false; | |||
| //had to remove this so the rearange presets would work | |||
| //if (!requestJSONBufferLock(9)) return false; | |||
There was a problem hiding this comment.
This is a showstopper, unfortunately.
As ESP32 & S3 are dual core, concurrent use of JSON buffer needs to be prohibited. That's the purpose of requestJSONBufferLock(). If lock cannot be acquired you cannot be sure that pDoc points to a valid JSON buffer.
This pull request addresses issue #3773 . It introduces the functionality for sorting presets, laying the groundwork for potential drag-and-drop capabilities in the frontend in the future. For example, if there are three presets with IDs 1 to 3, and we want the preset with ID 3 to become ID 1, the preset with ID 2 will then move to ID 3, and the preset with ID 1 will shift to ID 2.