search

xLightsSequencer / xLights

xLights is a sequencer for Lights. xLights has usb and E1.31 drivers. You can create sequences in this object oriented program. You can create playlists, schedule them, test your hardware, convert between different sequencers.
GNU General Public License v3.0
547 stars 207 forks source link

Custom Model Data Windows Undo Not working as exepcted #4815

Open kevinsaucier opened 2 weeks ago

kevinsaucier commented 2 weeks ago

I was originally going to file this an enhancement, but I just found a bug as well so figured I'd put them together. šŸ˜¬

  1. Bug: When numbering a model, if you add a number to an existing model (when renumbering, for instance) and then Undo, it removes that number from multiple places in the grid. For instance, if I'm renumbering and number 1-17, then Undo, it removes the 17 I just clicked as well as any other 17's from the rest of the grid.

  2. Kind of a bug: When renumbering and hitting undo, one would expect the Undo to put back the previous number. I figured maybe it was a 2 part process and a second Undo would put the previous number back, but that just removes the previous previous number. Repeating undo continues removing numbers without replacing them. This is unexpected behavior and causes you to lose where the numbers were. This is especially annoying when you're renumbering an existing model and intend to use it to remap submodels/states/faces as the pixels have to be in the exact same place.

  3. My enhancement request was that there was no Redo to go forward again after undoing. šŸ˜

To reproduce, simply open a model (I used the EFL Snowglobe), turn on number and click a cell to enter a 1, then Ctrl-Z to undo. Both the 1 you entered as well as the 1 that was already there disappear. I verified on the nightly build from last night.

As always, thanks!

computergeek1507 commented 2 weeks ago

If you intentionally have multiple nodes of the same number, that is kind of an edge case. I thought it was more important to remove duplicates as it's more likely a mistake. It's also not a true undo/redo, it was just intended as a increase/decrease of numbers box without clicking for keyboard people. To write a full backend that tracks state for true undo/redo seemed unnecessary

kevinsaucier commented 2 weeks ago

Understood on the full backend for undo/redo, that was a wish. :)

As for having multiple nodes be an edge case, I'd respectfully disagree. Other than the one time a vendor (hopefully) creates the base model for a file, I'd think the vast majority of the time, it's end users either fixing or modifying an existing model, which almost certainly will result in duplicate nodes temporarily. As it currently stands, I'd argue that the 'Undo' functions does more harm than good by removing 'nodes' that have nothing to do with what you're actually working on with no indication that it happened. If the intent was mostly to decrease the numbers box, I think it makes more sense to just remove the part of the function that is actually 'undoing' the entry of the number as it's not really functioning as one would expect Undo to do. JMHO.