Conversation
Only supports undoing/redoing adding tools.
✅ Deploy Preview for volview-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
PaulHax
left a comment
PaulHax
left a comment
There was a problem hiding this comment.
Works good! Tests even, wowy.
| import { createPinia, setActivePinia } from 'pinia'; | ||
| import { it, beforeEach, describe } from 'vitest'; | ||
|
|
||
| interface Collection { |
There was a problem hiding this comment.
No prepended I before interface 😱 I'm actualy good with that. In good company: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#interfaces
There was a problem hiding this comment.
This is just an interface for a test, so I'm not too concerned about the I prefix.
| const remove = (id: ToolID) => { | ||
| props.toolStore.removeTool(id); | ||
| const imageID = currentImageID.value; | ||
| if (!imageID) return; |
There was a problem hiding this comment.
No imageID, but showing remove button in toolList would be strange. Throw Error or derive imageID from toolID so imageID is guarrentied? Worried that defensive programming could lead to corner case runtime quirks.
There was a problem hiding this comment.
I like deriving the image ID from the tool ID in this case.
| }; | ||
| } | ||
|
|
||
| export function createUpdateToolOperation< |
There was a problem hiding this comment.
| export function createUpdateToolOperation< | |
| // Not used at the moment | |
| export function createUpdateToolOperation< |
There was a problem hiding this comment.
I intended to add support for undoing stuff like changing labels, moving rectangle points, etc. That hasn't happened yet, so I'm good to omit this function for now instead of adding a comment that we will have to later remove anyways.
| useHistoryStore().pushOperation( | ||
| { datasetID: imageID }, | ||
| addToolOperation! | ||
| ); |
There was a problem hiding this comment.
What do you think about hiding pushOperation and its ilk in the tool stores?
There was a problem hiding this comment.
It might be good to do it later down the road. I'm opting not to for now because I don't want to make the tool stores undo/redo aware just yet.
|
|
||
| const props = defineProps<{ | ||
| toolStore: AnnotationToolStore<ToolID>; | ||
| toolStore: AnnotationToolStore<ToolID> & Store; |
There was a problem hiding this comment.
In the future, should I put & Store on the AnnotationToolStore shared type?
There was a problem hiding this comment.
Yeah, I think that is a good idea. I could add that as an extra commit to this PR if you'd like.
| export interface IHistoryOperation<T = void> { | ||
| apply(): T; | ||
| revert(): void; | ||
| isApplied(): boolean; |
There was a problem hiding this comment.
Wonder if we should remove the isApplied idempotentcy escape hatch. Is there a valid case where the HistoryManager will call apply twice? Or is isApplied helpful elsehwere?
There was a problem hiding this comment.
I left isApplied as just a way for other code to know if an operation has been applied or reverted, and a convenient way to perform idempotency checks in the operation apply/revert code. HistoryManager itself implicitly keeps track of this state via the undo/redo stacks.
|
I'm identifying possible shortcomings with the current architecture: hierarchical transactions. One motivating example is the polygon tool: when placing points, undo would ideally remove dropped points, but when the polygon is finalized, undo should remove the entire polygon and not individual points. |
|
Would be cool if we made the polygon resumable. So can drop a few points, change tools/slice, switch back and close the polygon. Can then drop placing flag headache, it becomes "closed" for polygon case. Avoids the "hierarchical transactions" problem for this case. |
I'm not sure this is desired behavior, only because I've never observed such behavior. I've always considered editing an ephemeral state. In any case, I think we still need some semblance of a hierarchical/transactional history mechanism, since tools may have sub-histories only relevant to their operation. |
|
Moving to draft. At the moment, the integration is not there, but this PR has logic that may be useful for undo/redo implementation in the future. |
2 participants
Uh oh!
There was an error while loading. Please reload this page.