Only store revisions that include style changes

maplibre / maputnik

An open source visual editor for the 'MapLibre Style Specification'

https://www.maplibre.org/maputnik

MIT License

2.14k stars 402 forks source link

Only store revisions that include style changes #806

Closed kochis closed 9 months ago

kochis commented 1 year ago

Similar to issue https://github.com/maputnik/editor/issues/753, I've run into some odd behavior when trying undo/redo recent changes.

I've been using this change in a local fork, and it's seemed to be running pretty well. The main logic changes are:

Only store a revision if there is a style diff
- so you don't need to "undo" no-op changes
Clear the "redo" revisions once an addition is made
- without this change, things are pushed to the end of the list, even though the currentIndex may have been decremented (I suspect this is the main bug)

codesandbox-ci[bot] commented 1 year ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8a1a5ac42baaca86be4394adc4a9788a6f0cab20:

Sandbox	Source
maputnik	Configuration

nyurik commented 1 year ago

@kochis thx! Could you give some instructions on testing this? I'm slowly getting back into reviewing it

HarelM commented 11 months ago

@kochis I see how this makes sense (assuming style spec diff is trust worthy). We have now brought everything to a state where this can be maintained. There's e2e test that checks undo/redo, but they don't check this scenario, so I think it would be good to have either an e2e test or a unit test around this code. You'll need to migrate this code to use maplibre diff and move it to the relevant typescript file. Hope this is still relevant for you.

HarelM commented 9 months ago

I've tested this code for comments change, which is not defined as a change in terms of the maplibre-style-spec code. I'm not sure if this is a bug there as comments are not considered to be a style change for maplibre-gl. The redo fix is probably correct anyway and needs to be added to the code.

HarelM commented 9 months ago

Closing this PR for now, I think a proper style change mechanism should be used here but I'm not sure the diff of maplibre-style-spec is the right solution.