EditSystem modernization #1156
Merged
EditSystem modernization #1156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We are trying to get to a place where we can simplify the EditSystem
- Removes methods like `perform`, `replace`, `overwrite`, `pop` from Context.
If we want to change how these things work, there can't be multiple ways to call them.
Code must go through EditSystem to do these things now.
Starting to do this with `context.graph`, `.entity`, `.hasEntity` too
- For now, `withDebouncedSave` stuff isn't happening anymore
This was the code that would wrap edits in an occasional history save.
However this should be EditSystem's job (not Context's) and I'll move it over there.
- Renames:
`pauseChangeDispatch()` -> `beginTransaction()`
`resumeChangeDispatch()` -> `endTransaction()`
(chains of edits should be wrapped in transactions..
we were doing this already but not calling it that)
- Renames:
`context.systems.edit` -> `context.system.editor`
(it reads better, and mimics "validator")
- Other general cleanups
- Prefer short names for systems (`validator` over `validationSystem`).
- Prefer to list out all prerequisite systems near the top of each file.
- In validators, get really explicit about which `graph` is being used.
- (the one passed in and validated against may not necessarily be the current graph)
- A few more ES5->ES6 conversions, raw_member_editor.js
'data' is just too generic Also shorten/fix a few other system names
(closes #1110) - Before - imageryUsed was pushed to the editsystem - photosUsed was not working - no dataUsed at all - Now - EditSystem collects the sources that are used at time of edit - these are saved in the history along with the edit stack - separate changeset tags for imagery, photos, data
Also make sure when looking up a dataset by ID that we are removing the '-conflated' string from the dataset id. (This is hacky and we should improve it)
(followup from ca9be60, also see #961) Continuing to remove editing and localization operations from Context. The idea here is to remove "magic" from the code. These functions were just redirects from the Context to the EditSystem, so using them obscured what was really going on. It was easy to get confused about "which graph" the code was really using. - Removed `context.graph`, `.entity`, `.hasEntity` Code must go through the EditSystem to do these things now. - Convert several EditSystem accessor methods to ES6 getters - `.graph()` - removed - `.base` - now a property - `.current` - added - `.current` and `.base` now return the `Edit`, which has `.graph` as a property. This makes it easier to compare the current and base graph, and just more consistent flow. - Also WIP on replacing `context.t` and `.tHtml` Code to do these things should go through the LocalizationSystem There is quite a lot of this, and it will take a while.
Previously we had methods in context that would perform edits "withDebouncedSave". This should just be the EditSystem's responsibility. This commit renames a bunch of things for clarity (we call them backups now) and makes the EditSystem responsibile for its own backing up and restoring, as well as registering `beforeunload` handler and handling the mutex lock.
This can save some space in localStorage - Reduces unnecessary float precison - Remove `tags` if there are none - Remove `visible: true`
History v2 has not been generated since 2014 Also fix the tests to be able to handle the backup nodes not having `visible: true` (this is the default value so we don't need to bloat the history by including it)
'v' increments globally, so it might be something else. The important thing is that the history includes it.
This only affects running the mocha tests locally in a browser. When I want to click on a subsection or close the tab, the browser shouldn't nag me about navigating away.
(fixes #1126) I am not super confident in this fix, but I tried to comment and cleanup whatever I could.
The final step of the walkthrough shows a giant "Start Mapping" button. This is implemented as a uiModal, and it was previously dispatching a special event which would kick off the restore process and call fromJSON. The problem was that when fromJSON wants to restore childnodes, it made _another_ modal and it was impossible to get out of this. I solved this by - Make the "Start Mapping" modal not resolve until the user actually clicks it - Make it use a normal 'done' event like the other chapters - Reorder a few things in intro cleanup (e.g. osm service should toggle back on before we call fromJSON() - it may need that service to fetch child nodes)
We frequently do someting like perform a no-op edit and then replace it with other edits as the user draws or edits tags, etc. I'd like to get to a place where: "current" points to a "work-in-progress" Edit off-the-stack "stable" points to the latest stable Edit on-the-stack Performing and replacing edits makes more sense (and overwrite is unnecessary) Setting the annotation finalizes the edit and puts it on the stack Maybe more granular/smarter emitting of events like "editchanged" vs "historychanged" Because we always want to render when the user changes the work-in-progress/current edit, but we only want to validate/backup after that edit gets finalized and the stack changes
Lots changed in this commit: - Now that "current" work-in-progress edit is off the stack, most edits can just be a `perform()` - Removed: `.replace()`, `.overwrite()`, and `.pop()` - Added: `.commit()` which sets the annotation and moves the current edit into the history - Added: `.rollback()` which replaces the current edit with last stable edit - More granular change events: `editchange` fires when current changes (on every edit) `historychange` fires when stable changes (on every commit, undo, redo) (I may adjust the names of these more)
Start removing dead code. Also rename `resetToCheckpoint()` -> `restoreCheckpoint()`.
The new way of doing things means that these are not necessary - now all work-in-progress is performed on `current` graph, so there is no need to start by performing a no-op and replacing with real work - now we dispatch 'historychanged' when the edit is accepted/stable, so the validator can listen for that, and we don't need to manually call validate() This commit also includes a change to the entity_issues / validation fixes - before there was an optional extra parameter to onClick that was only used by the "unsquare_way" validator, which was used to re-validate after the transition completes. Now the validator will validate on 'historychanged' event so this isn't needed anymore.
- mostly docs cleanup - replace some references to `current` to say `stable` - remove old `validate()` which was just a synonym for `validateAsync()`
I think it sometimes makes the code easier to understand to
include function arguments that aren't used, for example:
```
editor.on('historychange', (fromEdit, toEdit, difference) => {
// code might not use `difference`, but no harm in including it as an argument
// so that people reading the code know what the argument list looks like.
});
```
I'm trying to get to a place where all the things that this system can do emit events consistently, and all receivers listen consistently too. Previously, before I started all this: - 'change' on every edit (perform/replace/overwrite), receives a difference - 'undone/redone', receives graphs - 'restore', receives nothing - 'restoreCheckpoint' emits nothing - Not all events were honored by the begin/endtransaction stuff. So - several things could cause the edit history to just get totally rewritten, and external code wouldn't really have ways to handle this very well. My ideal state is where: - `historychange` and `editchange` convey all the information for other code to handle any changes in the history, and these events emit consistently from anything that modified the history, and begin/end transaction works. - Receiving code can just look through the history - it's ok (specifically, the RapidSystem uses this to know what ids have been "accepted") - `restore`,`undone`,`redone` can go away. these are just special cases of `historychange`
Added `.hasWorkInProgress` so other code can decide whether to finalize or not. (This is because we can't easily compare graphs - I initially considered code like `editor.current.graph === editor.stable.graph` but because current is always a copy it will never be strict equals.) Adjusted how we reassign _lastStableGraph and _lastCurrentGraph in _emitChangeEvents, to avoid situation where an event receiver does something like editor.rollback() and kicks off an infinite loop / event storm. Changed undo so that it will rollback work-in-progress first before backing out the history. This is intuitively how users would expect undo to work.
This avoids emitting an unnecessary event
Needs to happen any time we make changes to the base graph.
At some point we will want to start recording in the edit history: - the mode that the user is in, and - whatever options (selected IDs) they used in that mode This is to support situations where we are selecting a mix of OSM and non-OSM features and then doing edits to them. Currently we are assuming everything in the edit history (selectedIDs) is an OSM feature. If we want to support where users are merging data from multiple sources and then be able to undo back out of those edits, it would be helpful for everything we need to be stored in the history, and backed up like other edits. We're not there yet, but this commit takes us closer to being able to do that.
Previously we were just taking the `context.selectedIDs()` - this change gives the calling code more control over what ids will actually be stored with the history entry. It's useful for situations like when you are adding a point - the point will not be selected until _after_ the commit has happened, but we'd still want to undo back into the state where that point exists and is selected. This change also includes a separate event for `statechange` which fires a bit differently than `historychange`. It's for situations like undo/redo/restore where we want to adjust the map transform and the selection. We previously had this code in a few places - now it just lives in MapSystem's `statechange` handler. I'm not sure whether it really belongs there. This may end up being more EditSystem's responsibility eventually.
(closes #858) This commit includes a bunch of things: - Remove old commented out code in a bunch of places - Rename event from `statechange` to `historyjump`, it's more descriptive - Major changes in the DrawArea/DrawLine code to make them survive undo/redo: - Store IDs now instead of entities, and get into the habit of refreshing from the current graph - On commit, save the important entity ids in a snapshot - On historyjump, restore the important entity ids from snapshot and decide whether to remain in the mode or exit out to select/browse - I had tried this approach a while ago, but it didn't work well because the graph changed on every pointer move! Now that we differentiate between the current graph that churns, and the stable graph that doesn't churn, it's possible to use the stable graph as a key to cache state.
This makes it more consistent, and means that code elsewhere that uses `mouse()` doesn't need to have explicit fallback logic.
These all do similar things, but were inconsistent before Paste was also committing the selectedIDs as a Set instead of an Array, causing crash
Now `performAsync` allows the code to perform an eased edit like before. Because it's Promise-returning, it's a lot easier to use than the old way where we'd setTimeout for 300ms to wait for the eased edit to finish.
|
Ye gods, man, what a ton of work. 🚢 💥 🍾 👋 One small comment before diving into the code on terminology: I'd favor |
oh this is great feedback - I totally agree 👍 |
|
per conversation earlier we are going to adjust the language a little bit:
|
(see #1156) Also the events are now `stagingchanged` and `stablechanged` Also fixed a few issues with the Validator `validateAsync` that the tests exposed. Basically, it was returning a resolved Promise even though the validation had started and was in progress. Also fixed failing test in Difference.test.js constructor test. It was trying to diff 2 Graphs that had different bases - this is not a thing we do in the appliction, so there's no expectation that Difference should consider base entities. (and doing so would be a pretty big performance penalty - the base entities get huge!)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes lots of changes to the
EditSystemto make it easier to handle how edits happen, how we deal with work-in-progress and improve the developer experience.Before:
perform/replace/overwrite/pop- these things manipulated the working edit pointed to by theindexand adjusted the historyperforma "no-op" edit and thenreplacethat edit as we do more drawing/editingperformorreplace- and these were the thing that decided whether an edit state was undoable or not.changeevent was dispatched anytime anything changed - this meant that elsewhere in the system we needed to write logic to account for the current graph might contain work in progress (a consequence of this was needing to take history backups at certain times, or manually callvalidator.validate()when it was ok to do so)performwould do its work in a transition and ease into the next edit - but there was no way to know when it was finished, so there was code in places likesetTimeout( ... , 300); // after any transition has completedNow:
stable- the latest committed edit on the stackcurrent- the work-in-progress edit, a branch taken just afterstablebut it exists off the stack until committedperform/replace/overwritewe have:perform(action)- always just performs work against thecurrentgraphrollback()- makecurrenta fresh copy ofstablecommit(annotation)- moves thecurrentwork onto the stack, advance index, and makes a newcurrentcommitAppend(annotation)- appliescurrentwork tostable, doesn't advance index, and makes a newcurrentperformAsync(action)- just likeperform, but does the edit in a transition and returns a Promise fullfilled when it's finished - no need to set timeouts anymore!editchange- Fires on every edit performed (i.e. whencurrentchanges)This was like
changebefore, the sort of thing to listen to when you want to rerenderhistorychange- Fires only when the history actually changes (i.e. whenstablechanges)This is for parts of the application that only want to respond to stable graphs like the validator - no need to call
validator.validate()anymore.historyjump- Fires on undo/redo/restore.This is for situations when we may need to jump the user to a different part of the map and restore a different selection.
pauseEventDispatchandresumeEventDispatchto control whether thechangeevent would fire.These are now renamed
beginTransactionandendTransaction(but basically do the same thing - pause events from firing until the transaction is complete)context- and fixed any known bugs involving backup and restoreThey can watch for the new events and work off the stable graphs to save and restore any drawing state that they need to.