Wx Undo system #628
Wx Undo system #628
Conversation
comments the first line
first version of undo/redo manager with stack memory option
class decorator to show calls methods form Windows
This reverts commit 5be06c1.
This reverts commit 63b7118.
1 - Add a "logging" menu in "window" 2-Create and Open a LoggingWindow with a wxHandlerCtrl 3 - Create and TimedRotatingFileHandler to cwd\Logs or via env variable trufont_logpath
1 - Autosize on parent if exists 2 - add OnClose to prevent multiple openingwindow
If log folder in os.getcwd() create it if not exists
Add the '.' in outside part Add class_name in method_decorator for logging
Add a state method Add a callback on_activated Change show methods
debug and loggingrotate option add them as paramaters of Application
Add a name for each object (__init__) Test this and state method
Set an undoredomgr for each new glyph opened When changes to a new glyph (via tab) activate the undoredomgr of the current glyph If debug is set, this can be view on menu item "Edit" , line after the redo
Add an undoredomgr with each glyph Replace call to ctor Glyph with TruGlyph
On _alignHCenter & _alignVCenter
log msg to understand organisation of window ant toolbars
Add Class Action in undoredo mgr Use it in fontWindow and propertiesView
Use it for debug wxWidget on mac osx 10.14
UndoRedoMgr -> UndoManager UndoRedoGlyph -> UndoableGlyph So filenames are changed too undoredomgr.py -> undoManager.py undoredoglyph.py -> undoableGlyph.py
|
(The title of this PR could be a bit more descriptive.) Briefly recapping the Blackfoundry approach: There are decorators to wrap UI methods, so the UI code needs to deal with undo/redo as little as possible. Layer objects are (partially) snapshotted for restoring later. For every action, there is a snapshot made before the action, and one after the action. The layer snapshots are partial: for example, if only anchors will be changed, the snapshot will only contain anchors. This factorisation is nice, as it nicely separates UI code from undo code. However, the decorators are quite complex, and a little hard to read/understand sometimes. The amount of logging is a little excessive, and often distracts from the actual code. It seems a little wasteful that two snapshots are needed (one before, one after the change), but that may be an unavoidable consequence of the fact that the snapshots are partial. I don't really have a solution to propose, but a sequence of outline edits will result in twice the amount of snapshot data than is strictly necessary. undoManager.py contains a lot of layer-specific code that should probable live in a different module. In trufont/tfont#18, the tfont layer object got some undo logic with the I tried to see if the Blackfoundry approach would be compatible with a more data oriented undo strategy, where changes are recorded instead of snapshots of edited objects. The undo manager infrastructure is definitely capable, as its Action objects are agnostic: they just store callables that can perform anything. That said, the majority of the code in this PR is dealing with specifics such as layer objects. |
|
Observation: this "sparse snapshot" approach is in some ways equivalent to the change-based approach suggested by Raph in #614, just at a courser granularity: the units here are layer.paths, layer.anchors, layer.components, layer.guidelines. |
|
@adrientetar what is needed to accept/reject this PR? :) |
|
I think it would help to review this if things were split up into reviewable smaller chunks. This PR sadly looks like a stream-of-consciousness commit log... One way to do this is:
I also recommend working on functionality in branches that is not named the same as the target branch. So you could branch off Regarding the command line things, maybe keep that to a separate PR, as it doesn't have anything to do with undo support. Edit: here's another article on how to clean up a PR: https://about.gitlab.com/2019/02/07/start-using-git/ |
|
@madig thanks for that. I agree. @JeremieHornus ? |
|
I agree the commits log is not very informative. And thank you for the 3 steps explanation on how to fix this afterwards. But I don't think the commits log is what should be looked at (they were mainly used as backups): the PR could be seen as one single commit too: 'Undo System' and the differences can be visualized with a 'git diff' before and after the PR: Thanks for the link to the article too. The command line issue thing was for fixing conflict when trying to merge: we had to make a choice. |
|
The problem is that such a massive PR is very hard to properly review because you have related and unrelated building blocks in one giant pile that you have to understand all at once (which no-one does without spending a day or two trying to work through it). In other projects, PRs usually introduce e.g. tests and base building blocks first and then use them on each subsystem. First you see how stuff is supposed to be used in the tests, then you inspect the building blocks and then you see how they are actually implemented in the subsystems. The commit messages can help with understanding, by explaining why some things were done the way there were done. It's true that none of this will make this PR have less code, but it will help with getting through it better and more quickly 👍 |
Both bugs below had the effect of moving the selection way too fast during mouse motion. First bug fix is line 274. Closes issue #19 of BlackFoundry/trufont fork. mouseDown immediately after mouseUp without OnMotion in between would have set mouseItem to None... Second bug fixed is at lines 298 and 410: The intent is to access the first point of the selected segment... but the first point of the whole contour was accessed instead. This is fixed.
There is conflict with @madig changes to main.py but we do the same (loading tfont files) with click module, and more debug options.