-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IMP] history: add the possibility to repeat commands at redo #2126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work. Some small questions but else look good to me :)
786049c
to
894aadf
Compare
894aadf
to
869c94a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coucou :)
} | ||
|
||
/** Apply generic transforms (sheetID, target, ...) to the core command children of a local command */ | ||
export function getRepeatedLocalCommandChildrenGeneric<T extends LocalCommand>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is only used once for STOP_EDITION. maybe we could just move (and rename) this to a specific repeat function repeatStopEdition
. It would be easier to understand than this kinda barbaric function name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I don't need to use it as a generic for odoo commands, I'll do that yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we don't need that in odoo, changed it to repeatStopEdition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: cannot make it into a specific transform because of cyclic imports. repeat_commands_registry.ts
need repeatStopEdition
to put in the registry => repeatStopEdition
need repeatCoreCommand
=> repeatCoreCommand
need the registry.
I put it back in this file, but changed the name to repeatLocalCommandChildren
which is less of a mouthful
90b37ac
to
367d45e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bloup bloup :)
Almost ready :)
return repeatTransform(getters, command, childCommands); | ||
} | ||
|
||
export function getRepeatedCoreCommands( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move those "generic" helper functions to repeat_revision.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I put them there is to avoid cyclic dependencies :x
b5cfc58
to
b871128
Compare
repeatPositionDependantCommand, | ||
]; | ||
|
||
export function repeatSheetDependantCommand<T extends Command>(getters: Getters, command: T): T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized this but it's actually spelled dependent in American English, we should change that at some point
@@ -193,6 +200,17 @@ export class Session extends EventBus<CollaborativeEvent> { | |||
return this.pendingMessages.length === 0; | |||
} | |||
|
|||
/** Get the last local revision that doesn't come from a REQUEST_REDO command */ | |||
getLastLocalNonRedoRevision(): Revision | undefined { | |||
const revisions = this.revisions.getRevertedExecution(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick as most of my comments but I don't like this function - it could technically capture a "REQUEST_UNDO" command, it's just that we don't call the function in that case because the presence of request undo in the session implies the existence of some commands in the undo stack inside local history but the session is unaware of the undo/redo stack. I think it should be more 'draconian' or at least add some comments/check where it is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking a bit into that, if I understand correctly revisions from standard undo/redo are created inside the selective_history, and are empty. So they are not returned in this method. But this isn't very intuitive, so I changed things a bit and added a comment.
LMK if the code looks clearer now
forgot to mention the circular dependency at build :/ |
cb54f5e
to
41f65cf
Compare
The commit transform the history into a standard UI feature plugin. To do that : - the session is now given to the constructor of the UI plugins - `session.on("revison-redone")` and `session.on("revison-undone")` are now handled directly in the model, as they need `dispatchFromCorePlugin` Task: 2508693
Task: 2508693
Now if we do a redo and the redo stack is empty, we will try the redo the last played command if possible. The list of repeatable commands is defined in the repeat_commands_registry.ts file. Repeated commands are transformed to match the current selection/sheet. To be able to repeat local commands, the "root" command of a revision was added to the history. Locals commands of stateful UI plugins cannot be simply transformed and repeated, because the state of The plugin may have changed. There is 2 strategies to solve this : 1. If possible, transform core sub-commands instead (eg. STOP_EDITION) 2. If not possible, we have to store the last state of the plugin, and dispatch a new commands REPEAT_* (eg. REPEAT_PASTE) It's not possible to adapt the sub-commands of the paste command, because they are dependant on the state of the sheet, and may change based on the paste location. A simple example is that paste create new col/rows for the clipboard content to fit the sheet. So there will be ADD_COL_ROW_COMMANDS in the sub commands in the history, but repeating paste might not need them. Or they could only needed for the repeated paste, not for the original. Task: 2508693
41f65cf
to
06a1000
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
robodoo r+
robodoo rebase-ff bordel 😬 |
The commit transform the history into a standard UI feature plugin. To do that : - the session is now given to the constructor of the UI plugins - `session.on("revison-redone")` and `session.on("revison-undone")` are now handled directly in the model, as they need `dispatchFromCorePlugin` Task: 2508693 Part-of: #2126
Now if we do a redo and the redo stack is empty, we will try the redo the last played command if possible. The list of repeatable commands is defined in the repeat_commands_registry.ts file. Repeated commands are transformed to match the current selection/sheet. To be able to repeat local commands, the "root" command of a revision was added to the history. Locals commands of stateful UI plugins cannot be simply transformed and repeated, because the state of The plugin may have changed. There is 2 strategies to solve this : 1. If possible, transform core sub-commands instead (eg. STOP_EDITION) 2. If not possible, we have to store the last state of the plugin, and dispatch a new commands REPEAT_* (eg. REPEAT_PASTE) It's not possible to adapt the sub-commands of the paste command, because they are dependant on the state of the sheet, and may change based on the paste location. A simple example is that paste create new col/rows for the clipboard content to fit the sheet. So there will be ADD_COL_ROW_COMMANDS in the sub commands in the history, but repeating paste might not need them. Or they could only needed for the repeated paste, not for the original. closes #2126 Task: 2508693 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Merge method set to rebase and fast-forward. |
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 Task: 3916488
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 closes #4201 Task: 3916488 Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 Task: 3916488 X-original-commit: be5500d
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 Task: 3916488 X-original-commit: be5500d
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 Task: 3916488 X-original-commit: be5500d
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 closes #4217 Task: 3916488 X-original-commit: be5500d Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 closes #4216 Task: 3916488 X-original-commit: be5500d Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 Task: 3916488 X-original-commit: be5500d
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 closes #4218 Task: 3916488 X-original-commit: be5500d Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com> Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 Task: 3916488 X-original-commit: 5b7b84b
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 Task: 3916488 X-original-commit: 5b7b84b
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 closes #4219 Task: 3916488 X-original-commit: 5b7b84b Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com> Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 Task: 3916488 X-original-commit: f0f1e3d
Since pull request 2126[^1], the shortcut F4 is handled by the grid component. Unfortunately, the same shortcut is handled in the composer as well and its propabation was not stopped. This means that a user wanting to loop the references inside their formula could see some unexpected side effects due to the grid replaying some commands. [^1]: #2126 closes #4221 Task: 3916488 X-original-commit: f0f1e3d Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com> Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
Description:
Now if we do a redo and the redo stack is empty, we will try the redo
the last played command if possible. The list of repeatable commands
is in defined in the repeat_commands_registry.ts file.
Repeated commands are transformed to match the current selection/sheet.
To be able to repeat local commands, the "root" command of a revision
was added to the history.
Odoo task ID : 2508693
review checklist