Repository navigation
Conversation
…nges and history limitation
… showing undo and history on button for now. added test according to that, if drawer changes would be saved as one history in config item
…thod and history button
…tead and aligned undo button in drawelr left
…ary doubled items in InputConfigDialog were deleted
…r outside undo button
…ng config and apply in config view.
…nu bar and undo part under edit
…' into 3439/undo-for-configitem-list
…' into 3439/undo-for-configitem-list
… interface to pass test
…' into 3439/undo-for-configitem-list
…res by adding undo
|
Translation Statistics:
Language: ES (Core)
Language: FI
Language: JA
Language: KO
Language: PT
Language: RU
GitHub Workflow Comment: Translation Statistics
|
|
Build for this pull request: |
|
Build for this pull request: |
… is placed, and text edit session is moved to another hooker to check
…d be recalled only once after checking which is active
…' into 3439/undo-for-configitem-list
|
Build for this pull request: |
|
Build for this pull request: |
|
Build for this pull request: |
DocMoebiuz
left a comment
DocMoebiuz
left a comment
There was a problem hiding this comment.
I am very excited to see this coming along!
Please review my first round of feedback.
I am convinced that especially the CommandUpdateConfigItem vs CommandConfigContextMenu can be simplified.
The .NET C# side will benefit from a more thorough review in another round.
|
|
||
| setTimeout(() => setActiveConfigFileIndex(configFiles.length), 200) | ||
| } |
| const handleToggle = () => { | ||
| publish({ | ||
| key: "CommandConfigContextMenu", | ||
| payload: { | ||
| action: "toggle", | ||
| item, | ||
| }, | ||
| } as CommandConfigContextMenu) | ||
| } | ||
|
|
There was a problem hiding this comment.
CommandUpdateConfigItem - instead of adding new individual actions to the the backend, the frontend could still modify the object as needed and simply "annotate" the type of change together with the CommandUpdateConfigItem.
The benefit: backend stays KISS, and mechanism is extensible.
| const saveChanges = (newName: string) => { | ||
| if (newName === item.Name) return | ||
| publish({ | ||
| key: "CommandUpdateConfigItem", | ||
| payload: { item: updatedItem }, | ||
| } as CommandUpdateConfigItem) | ||
| key: "CommandConfigContextMenu", | ||
| payload: { action: "rename", item: { ...item, Name: newName } }, | ||
| } as CommandConfigContextMenu) | ||
| } |
There was a problem hiding this comment.
Same as comment above... but here you are also modifying the config object on the frontend side.
So that is incosistent.
My suggestion from above would make sure that our approach is consistent.
Frontend modifies the item.
Frontend publishes "CommandUpdateConfigItem" command + modify annotation "rename"
Backend only simply stores the new state, doesn't perform any modification logic.
| const deleteSelected = useCallback(() => { | ||
| const action = "delete" | ||
| const selectedConfigs = table.getSelectedRowModel().rows.map((row) => { | ||
| return row.original as IConfigItem | ||
| }) | ||
| const selectedConfigs = table | ||
| .getSelectedRowModel() | ||
| .rows.map((row) => row.original as IConfigItem) | ||
|
|
||
| if (selectedConfigs.length === 0) return | ||
|
|
||
| publish({ | ||
| key: "CommandConfigBulkAction", | ||
| payload: { | ||
| action: action, | ||
| items: selectedConfigs, | ||
| }, | ||
| }) | ||
| // Clear selection after deletion | ||
| if (selectedConfigs.length === 1) { | ||
| // Use the single-item delete path. | ||
| // The backend records an Undo snapshot here. | ||
| publish({ | ||
| key: "CommandConfigContextMenu", | ||
| payload: { | ||
| action: "delete", | ||
| item: selectedConfigs[0], | ||
| }, | ||
| } as CommandConfigContextMenu) | ||
| } else { | ||
| publish({ | ||
| key: "CommandConfigBulkAction", | ||
| payload: { | ||
| action: "delete", | ||
| items: selectedConfigs, | ||
| }, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
why is it necessary to treat single delete different?
| Command: preset ? preset.code : null, | ||
| } as MsfsInputAction, | ||
| t("Dialog.InputConfigWizard.History.ActionPresetChanged"), | ||
| ) |
There was a problem hiding this comment.
don't resolve i18n labels on creating and storing a history entry, resolve labels at the time when they are displayed to the user.
| Add, | ||
| Duplicate, | ||
| BulkDelete, | ||
| Toggle, |
There was a problem hiding this comment.
this would not be required if we use the CommandUpdateConfigItem and provide change annotation.
| ProfileAdded, | ||
| ProfileRename, | ||
| ProfileDeleted, | ||
| ItemRename, |
There was a problem hiding this comment.
this would not be required if we use the CommandUpdateConfigItem and provide change annotation.
| ProfileDeleted, | ||
| ItemRename, | ||
| ProfileMerged, | ||
| ItemEdited |
There was a problem hiding this comment.
this would not be required if we use the CommandUpdateConfigItem and provide change annotation.
There was a problem hiding this comment.
it looks like on the backend side we are applying a new concept, where you have undoActions instead of just a snapshot?
There was a problem hiding this comment.
Now that you moved classes to ConfigListUndoManager, this class requires updates
Summary
Adds Global Undo functionality for configuration item lists and profiles, allowing users to reverse accidental changes without losing their existing configuration.
Users can undo actions such as adding, deleting, duplicating, renaming, toggling, and moving configuration items, as well as several profile operations.
The implementation includes a shared Undo history, accessible from the project toolbar next to the Save button, and supports Ctrl+Z and multi-step history navigation.
Screenshots / recordings
001.mp4
Acceptance criteria
DoD Checklist
Related issues
fixes #3439
relates to #2886
depends on #3438
Out-of-scope
Notes