-
-
Notifications
You must be signed in to change notification settings - Fork 998
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
Fix return value of undo() and redo() methods. #545
Conversation
@burhanrashid52 Feel free to modify changes. Also, wasn't sure about versioning. |
@@ -75,7 +75,7 @@ internal class GraphicManager( | |||
mViewState.redoViewsCount - 1 | |||
) | |||
if (redoView is DrawingView) { | |||
return redoView.redo() | |||
return redoView.redo() && (mViewState.redoViewsCount != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redo still doesn't work as expected. Steps to reproduce:
- Add a brush drawing
- Add a text
- Call
undo()
twice - Calling
redo()
returnsfalse
. Should returntrue
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wasky I changed the condition and also changed the redo logic a bit. Consider a scenario when the user does a redo action and draws another thing on top of it. I think we should clear redo history in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I agree that the logic should be changed
- I tested the change and can confirm that
undo()
andredo()
return proper values - I tested the change and can confirm that the new clearing redo logic works fine
- But I'm not very familiar with this part of the code, so it would be great if @burhanrashid52
did a review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it's LGTM as well, also the CI UI test is passing so I am assuming everything is working fine. Thanks for review and testing it out @wasky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I'm not very familiar with this part of the code in the lib.
Thanks, @VardanTitan for the PR now it is live at 3.0.2 and also @wasky for the review. Let me know what you think! Thanks. |
Fixes #460