fix: an eval&undo/redo "lost ID" bug #1086
Merged
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
Previously `AppTestM` transformed `TestM`, thus it had both an `App` state (recall, an `App` contains some fresh `ID` and `NameCounter` state) and also a separate `ID`/`NameCounter` state via `TestM`. Thus, some things use one set of counters, and some the other. In particular, `handleRedoRequest` works via `runEditAppM` which uses the `App`'s counters, but `handleEditRequest` uses the `TestM`'s counters. This only happened in tests, and not in the actual service. This commit ensures that tests are closer to the real service by removing the `TestM` layer, ensuring that we always use the `App`'s counter. (This is needed to tickle some long-standing bugs involving "lost IDs" in the undo/redo system.) Signed-off-by: Ben Price <[email protected]>
The refactoring in the previous commit left this argument unused. Signed-off-by: Ben Price <[email protected]>
Signed-off-by: Ben Price <[email protected]>
brprice
force-pushed
the
brprice/eval-undo-bug
branch
from
f6cc38a
to
f9415f0
Compare
brprice
force-pushed
the
brprice/eval-undo-bug
branch
from
f9415f0
to
24d18a3
Compare
This is in preparation for moving Eval into `MonadQueryApp` rather than `MonadEditApp`. Signed-off-by: Ben Price <[email protected]>
This makes it consistent with `liftQueryAppM`. Signed-off-by: Ben Price <[email protected]>
This is in preparation for moving Eval into `MonadQueryApp`, as we need to know that `EvalFull` will never throw an error. Signed-off-by: Ben Price <[email protected]>
Prior to this, evaluation happened in `MonadEditApp`, and thus could change the fresh ID state embedded in the `App`. This can cause "lost ID" errors when interleaving actions, evaluations and undo/redos. Note that it is suspicious that `evalFull'` is still using `liftEditAppM` and generates a new ID via `gvar` -- this indeed leads to the same sort of issues, and will be dealt with in a subsequent commit. (The fact that this code will be changing very soon is why we have not gone to the effort of using `readerToState` here.) We have added a test showing that we have fixed this "lost ID" issue at the "richly-typed api" level. (Note that `evalFull'` is not part of this layer.) Signed-off-by: Ben Price <[email protected]>
brprice
force-pushed
the
brprice/eval-undo-bug
branch
from
24d18a3
to
29610a8
Compare
georgefst
approved these changes
Jun 28, 2023
The two issues were (a) the first commit and (b) the last two commits. The first (as mentioned in commit message) turned out not to be an issue in the deployed version (which explains why I could not manage to get a failure because of it). I am not aware of any other issues causing undo bugs after this PR. |
This finishes the work started in the previous commit, fixing the "simplified"/OpenAPI layer. Signed-off-by: Ben Price <[email protected]>
brprice
force-pushed
the
brprice/eval-undo-bug
branch
from
29610a8
to
b2e7c59
Compare
3 tasks
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.
No description provided.