gabm / Satty

Satty - Modern Screenshot Annotation. A tool inspired by Swappy and Flameshot.
Mozilla Public License 2.0
389 stars 18 forks source link

fix(bug): reset marker counter after undo action #9

Closed noornee closed 8 months ago

noornee commented 8 months ago

This solves issue #8 I would appreciate your feedback and any recommendations for improving this solution.

video showing bug fix

satty-marker-undo-bug_fix.webm

gabm commented 8 months ago

thank you so much for giving this a try.. but I'm afraid that this won't work on all circumstances..

a simple counter example is

Your solution is to create a global marker tool that is being used to track the number count - but using a local tool instance to do the "tool work"... that local instance works as before - except that it takes the number from the global instance...

Currently the architecture is, that tools create "drawables". Once the editing via the tool is finished, a drawable gets committed to the drawable stack in the sketchboard. Undo pops from the drawable stack and pushes to the redo stack, redo pops from the redo stack and pushes to the drawable stack.

By design, the sketchboard doesn't know anything about the workings of tools and drawables. That also means it doesn't know that popping a drawable may lead to side-effects such as decrementing a number. That should stay that way i think..

Without thinking it through, i believe the best approach would be to let the tool know that something has been reverted by introducing an additional event.. this way the tool can then take proper action or choose to ignore it..

noornee commented 8 months ago

Thank you so much for your response and valuable feedback!

I totally understand the concern you've raised about the limitations of this solution. I only went with using a global variable cos that was the only think i could think of at the time.

The fact that the sketchboard shouldn't be aware of tool or drawable specific details is a reasonable design principle andd I agree with the approach of introducing an additional event to notify the tool when something is reverted

I would have loved to do this myself but I'm still in the process of learning the language and am not yet confident enough to implement the proposed changes myself ;-;

However, I'm eager to contribute to the project and assist in any way I can.

gabm commented 8 months ago

Thank you for your efforts. As explained in the issue, the fix is here: https://github.com/gabm/satty/commit/74880878c42fb7ef8fefbf2e5dcd50ccc556e91c

I am sorry to reject your PR, but it has the mentioned functional shortcummings as it stands. I still want to encourage you to continue to contribute, pls reach out to me on Matrix chat: @gabm:matrix.org - I can give you guidance next time!

noornee commented 8 months ago

Thank you for your efforts. As explained in the issue, the fix is here: 7488087

niceeeee! :star_struck:

I am sorry to reject your PR, but it has the mentioned functional shortcummings as it stands. I still want to encourage you to continue to contribute, pls reach out to me on Matrix chat: @gabm:matrix.org - I can give you guidance next time!

There's no problemo :) Thank you! I would totally contribute whenever i see an issue i could help with andd Thanks for providing your matrix id, i would reach out whenever i need guidance! (i dont have a matrix account atm but it's about time i create one)