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 custom undo system #118

Merged
merged 32 commits into from
Feb 2, 2022
Merged

🎉 Add custom undo system #118

merged 32 commits into from
Feb 2, 2022

Conversation

FabienRoger
Copy link
Contributor

Fix the bug #27 by creating a custom undo system.

@FabienRoger FabienRoger changed the title Fix the bug #27 🪲 Fix the bug #27 Dec 15, 2021
Copy link
Member

@MathisFederico MathisFederico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please name back scene.scene_history to scene.history as it is useless to precise scene again, for both the submodule name and the OCBScene attribute.

You can create a submodule pyeditor with pyeditor.py and history.py inside.
You can then import submodules classes in pyeditor.__init__.py for easy access.

@FabienRoger FabienRoger linked an issue Dec 23, 2021 that may be closed by this pull request
@FabienRoger FabienRoger self-assigned this Dec 23, 2021
Copy link
Member

@MathisFederico MathisFederico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be updated with the new code structure (opencodeblocks/core doesn't exist anymore)

@FabienRoger
Copy link
Contributor Author

I rebased the changes of this old bug fix manually because the code had changed too much.

@MathisFederico
Copy link
Member

MathisFederico commented Jan 9, 2022

The Undo operation removes last editor changes even when the editor isn't focused.

Steps to reproduce:

Create two code blocks
image
Modify an editor
image
Change the graph
image
Ctrl+Z : Only 1 time !
image
It moved back the graph AND removed the text, but it should only have moved back the graph.

Building an integration test for this (and other) expected behaviors might be a good idea for future maintenance.

@FabienRoger
Copy link
Contributor Author

The undo system I introduced is not the cause of this bug. The current dev branch displays the same behaviour, and markdown blocks, which don't have a custom editor, too.
I don't know what the expected behaviour is for this type of system. Should every text edit be considered as a change and set as a checkpoint to the whole scene?
I think this issue deserves a separate issue/branch.

@FabienRoger
Copy link
Contributor Author

I don't know why the tests failed.

@MathisFederico
Copy link
Member

MathisFederico commented Jan 10, 2022

The current dev branch displays the same behavior, and markdown blocks, which don't have a custom editor, too.

That's because text editor had no history before !

I don't know what the expected behavior is for this type of system. Should every text edit be considered as a change and set as a checkpoint to the whole scene? I think this issue deserves a separate issue/branch.

Text editing should had a checkpoint to the whole scene, but not the reciprocal ! When not in any editor, the ctrl+z should undo the previous action, whatever it was (even text editing). When in an editor, ctrl+z should undo the last action in that editor only.

I don't know why the tests failed.

You probably changed how the history is handled and broke a test, if you don't understand why it failed I will look into it and comment it better !

@FabienRoger
Copy link
Contributor Author

FabienRoger commented Jan 10, 2022

That's because text editor had no history before !

Both markdown blocks and code blocks had the default Qt history.

You probably changed how the history is handled and broke a test

The test which doesn't pass is the test I added. It works on my machine. It doesn't work on github.

@MathisFederico
Copy link
Member

The test which doesn't pass is the test I added. It works on my machine. It doesn't work on github.

Locally the test wasn't called, it fails now on my machine.
image

Also I don't like this error msg, hopefully it will disapear when you fix your test:
image

@MathisFederico MathisFederico changed the title 🪲 Fix the bug #27 🎉 Add custom undo system Jan 14, 2022
@MathisFederico
Copy link
Member

I fixed some issues, but for now this is not working because undo in the main history causes the scene to "rebuild" codeblocks making them lose their local editor history.

This falls into a larger problem mentioned in #119.
I think we need to branch this and refactor the way we undo/redo the main scene.
Using serialization was easy, but is a clear loss of memory and makes us lose track of blocks between undo/redo ...

@MathisFederico
Copy link
Member

Fixes #241

@MathisFederico
Copy link
Member

Tests were commented out because we really need to merge this (they work locally).
Noted in #244

@MathisFederico MathisFederico merged commit 88473c7 into dev Feb 2, 2022
@MathisFederico MathisFederico deleted the bugfix/historylost branch February 2, 2022 06:38
@MathisFederico MathisFederico linked an issue Feb 2, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants