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

change number seed on undo/redo (proof of concept) #316

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NicoWeio
Copy link

@NicoWeio NicoWeio commented Nov 17, 2022

My suggested behavior is setting the seed to the just-undone number or (just-redone + 1), respectively.

I'm not familiar with this codebase at all, so my approach might be quite ugly – but it works and currently, no better solution (as in “a PR”) seems to exist.

Fixes #308

@DamirPorobic
Copy link
Member

What is the motivation for having this behavior? Do you have scenario where such a behavior is required?

@NicoWeio
Copy link
Author

I think #308 describes it pretty well.
The suggested behavior is not strictly required, but more efficient and IMO more intuitive.
If I press Ctrl+Z, I expect all state to be “reset”.
However, currently, adding a number annotation, undoing, then adding one again, would result in a larger annotation number each time you do it.

@DamirPorobic
Copy link
Member

That makes sense. Still I don't like the if there, would love to see some inheritance helping us out. Could you add some simple factory that returns one of two types of AddCommands, for NumberTools a AddAnnotationNumberCommand and for all other the usual AddCommand. The AddAnnotationNumberCommand inherits from AddCommand and does all the same (by calling the base) + resets the seed. I know it's a bit more code but I think it's the right way to do it.

@NicoWeio
Copy link
Author

I absolutely agree. As I said, this was only a quick draft.

Unfortunately, I'm not that accustomed to C++ and am currently struggling with referencing AddCommand from AddAnnotationNumberCommand. For some reason, all I get is expected class-name before »{« token.

It might take me some weeks or months until I can allocate more time to this, so if you or somebody else would like to, you are more than welcome to take over.

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.

Undoing a Number annotation does not decrease Number Seed
2 participants