Skip to content
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

Undo/redo, bulk edit and onWill-file events #110930

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Undo/redo, bulk edit and onWill-file events #110930

merged 1 commit into from
Nov 19, 2020

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Nov 19, 2020

This PR makes it possible that additional workspace edits from an onWill[Create|Rename|Delete]Files are added to the same undo/redo groups as the initiating operation. The sample is the Java file rename which makes a text edit before the actual rename. Those should only require a single undo.

This is PR is kinda large but simple. It spreads data because

  • all onWill-events are triggered from the working copy service
  • that service doesn't know anything about undo/redo (on purpose)
  • the bulk edit service does know about undo/redo and consumes the working copy service
  • some undo/redo information needs to travel to onWill-event listener
  • therefore the bulk edit service can now pass data about undo/redo which the working copy service simply forwards to its event handlers

fyi - the alternative would have been to move all onWill-event logic into the bulk edit service but since there are still direct consumer of the working copy service it would have meant that the API now fires less event, like for "save as".

Somewhat open questions

  • on the ext host the onWill-events will not fire anymore when undoing/redoing. That's to safe the undo-stack but might be unexpected?
  • should the same happen for onDid-events that are caused by undoing/redoing? Those cannot piggy-bag extra edits tho.

@jrieken jrieken self-assigned this Nov 19, 2020
@jrieken jrieken requested a review from bpasero November 19, 2020 10:28
@jrieken
Copy link
Member Author

jrieken commented Nov 19, 2020

@bpasero please check the changes in the working copy service

@jrieken jrieken added this to the November 2020 milestone Nov 19, 2020
@jrieken
Copy link
Member Author

jrieken commented Nov 19, 2020

fyi @isidorn

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrieken seems ok to me, but would it make sense to put undoRedoGroupId: number | undefined, isUndoing: boolean | undefined into a single object to keep it together? It seems that it could be encapsulated in some object that describes the undo/redo state?

@isidorn
Copy link
Contributor

isidorn commented Nov 19, 2020

@jrieken Thanks for looking into this. Once this lands I can double check the Java rename flow.

@jrieken
Copy link
Member Author

jrieken commented Nov 19, 2020

related/tackles #107740

@jrieken
Copy link
Member Author

jrieken commented Nov 19, 2020

but would it make sense to put undoRedoGroupId: number | undefined, isUndoing: boolean | undefined into a single object to keep it together?

I was also wondering about that but I didn't wanna make the undo/redo world more prominent in the working copy service. When more flags come I will do.

@jrieken jrieken merged commit 5876a5e into master Nov 19, 2020
@jrieken jrieken deleted the joh/undoOpts branch November 19, 2020 14:10
@jrieken jrieken added workspace-edit api-finalization feature-request Request for new features or functionality labels Nov 19, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-finalization feature-request Request for new features or functionality workspace-edit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants