Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add undo command for removing layers in legend #1218

Closed
wants to merge 1 commit into from

Conversation

etiennesky
Copy link
Contributor

No description provided.

@etiennesky
Copy link
Contributor Author

this PR adds basic support for undo/redo of removing layers in the legend. It also restores layer(s) back to parent but does not restore in same order . I didn't want to mess with that, because undo/redo for moving layers is another topic, and once working could be applied here.

@etiennesky
Copy link
Contributor Author

undo/redo are added to legend context menu for now, I didn't mess with the undo widget

@wonder-sk
Copy link
Member

Hi Etienne

unfortunately I do not think this is the way forward:

  1. it does not really remove the layers from the project (only removes the legend entries). I haven't checked, but it is possible that the removed layers will be even stored in the project file
  2. the undo stack should not be legend-specific - rather it should be a project-wide undo stack, so it works for various other operations with project (where legend does not need to be involved at all). What if someone removes a layer by different means? (e.g. remove from map layer registry)
  3. with undo stack it is necessary to ensure that all similar operations go through the undo stack. It is very strange from user's point of view that only remove operation is supported

@etiennesky
Copy link
Contributor Author

Hi Martin,

  1. I tried to do it as simple as possible and retain the legend item attributes, so keeping the legend item around does that. how would you keep the layers and their properties (symbology, etc) around if you remove them completely? I didn't check though if the layers are still in project
  2. unless I'm mistaken, currently undo/redo commands are bound to a specific layer and the undo stack changes when the active layer changes. I don't see how this can work with a project-wide undo stack - or perhaps it would be separate from that? So you want to support all remove operations, even those done programatically? If you remove the layers from the map registry, the only way is to do it by a command, no gui right?
  3. this is just a prototype intended to add other layer-related actions (like move layers) in the future. again, it would be hard to merge ALL undo operations as there are many undo stacks floating around (one for each layer).

I understand proper support of undo/redo is going to be a challenge, feel free to use some of these ideas in the future.

@wonder-sk
Copy link
Member

To answer your questions:

  1. you would need to keep track of the layer properties - for example use layer's read/write XML methods to retain this information and store that in undo commands. This way you can properly add or remove a layer at any time. The undo command for add layer and remove layer should be ideally the same class, just with reversed functionality. I have some more thoughts from my other projects regarding good practices for implementation of undo commands, but it will need a bit longer discussion
  2. this is an issue we need to tackle somehow with our UX team - what would be the best solution in case they are more active undo stacks. I guess it would be good to have just one pair of undo/redo buttons with some logic for determining which undo stack should be active (project-wide stack or layer-based stack), for example by default it would be project-wide stack and only if the layer is in editing mode it would show layer-based stack - but with the option to switch to the project-wide stack. There are several other options of course.
  3. oh I didn't get it was just a prototype. Also, from my experience, when adding undo/redo support, all related operations have to be added, otherwise the undo stack may get inconsistent and lead to errors,

@etiennesky
Copy link
Contributor Author

I'm closing this PR as it doesn't do much with the new layersTree API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants