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

Add a setting or always restore the last editor state for editors opening in new groups #102485

Closed
jrieken opened this issue Jul 14, 2020 · 15 comments
Assignees
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders papercut 🩸 A particularly annoying issue impacting someone on the team verification-needed Verification of issue is requested verification-steps-needed Steps to verify are needed for verification verified Verification succeeded workbench-state UI state across restarts
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Jul 14, 2020

While working on #101356 I have noticed that EditorMemento has an issue because it stores stated based on group#id and that keeps changing. These steps

  • have an editor
  • split the editor - it's now in group 2, no memento state found
  • close the 2nd editor, state is persisted for group 2
  • split again - it's now in group 3, no memento state found
  • close the 2nd editor, state is persisted for group 3
  • split again - it's now in group 4, no memento state found
  • this repeats...

The capture below shows the memento for a notebook that I have opened (split) and closed a few times

Screenshot 2020-07-14 at 13 07 15

@jrieken jrieken changed the title EditorMemento is bogus EditorMemento is bogus, editor state doesn't survive split-close-split cycle Jul 14, 2020
@bpasero
Copy link
Member

bpasero commented Jul 14, 2020

@jrieken if a group is closed, view state is not restored the next time the group is opened again because it may be frustrating for users to constantly get old view state back in that case. Imagine you had an editor split for a while and then forgot about it after closing. You then split the editor again and would probably expect the split editor to show you the location where you split from, not some state from the past.

There is code on shutdown that will remove groups from the memento that no longer exist:

// Remove groups from states that no longer exist. Since we modify the
// cache and its is a LRU cache make a copy to ensure iteration succeeds

I am not sure what exactly you expect how the editor memento should behave, maybe you can clarify that.

@bpasero bpasero added the info-needed Issue requires more information from poster label Jul 14, 2020
@jrieken
Copy link
Member Author

jrieken commented Jul 15, 2020

frustrating for users to constantly get old view state back in that case.

I actually want that and isn't that how the first group behaves? Then I wonder why view state is persisted at all as it won't ever be re-used, right?

@bpasero
Copy link
Member

bpasero commented Jul 15, 2020

@jrieken yes first group is special because you are not starting from a specific group, you open your first initial editor and then there is no existing state to copy from. In that case restoring from the previous one seems ok to me.

@jrieken
Copy link
Member Author

jrieken commented Jul 15, 2020

I understand the problem with split but what I often do is cmd+2 to get a new, empty group and when I open a file it never has a view state. The scenario is very similar to the "group 1 case" but works different

@bpasero
Copy link
Member

bpasero commented Jul 15, 2020

I am not brave enough to make that change to be honest, but I added a fallbackToOtherGroupState?: boolean to loadEditorState() that can optionally be passed in to fallback to the editor state of the last active group. If this makes sense for notebooks it means that a notebook in a new group without previous state will inherit the state from the last group that was active.

@bpasero bpasero added this to the July 2020 milestone Jul 15, 2020
@bpasero bpasero added workbench-state UI state across restarts and removed info-needed Issue requires more information from poster labels Jul 15, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2020
@jrieken jrieken reopened this Jan 29, 2021
@jrieken jrieken added the papercut 🩸 A particularly annoying issue impacting someone on the team label Jan 29, 2021
@microsoft microsoft unlocked this conversation Jan 29, 2021
@jrieken
Copy link
Member Author

jrieken commented Jan 29, 2021

Sorry, re-opening as paper cut. This is my biggest issue when working with side editors. I very often open an editor to side to look something up, then close it (with the whole group), just to re-open it very shortly after. I then have lost the view state (scroll position etc).

I understand the concern of breaking existing users but this could be a setting

@bpasero bpasero added the feature-request Request for new features or functionality label Feb 22, 2021
@bpasero bpasero modified the milestones: July 2020, Backlog Feb 22, 2021
@bpasero bpasero changed the title EditorMemento is bogus, editor state doesn't survive split-close-split cycle Add a setting or always restore the last editor state for editors opening in groups Feb 22, 2021
@bpasero
Copy link
Member

bpasero commented Feb 22, 2021

Yeah I think something like this would be needed:

  • anytime editor state is persisted into the memento, it is also stored in a location that is independent from the group id
  • whenever we pickup editor state for a new editor, we fallback to that last state we know of unless we have a more specific one for the group
  • we might want to have this behind a setting, but maybe it should actually be on by default because I think it is nice to restore things like folded regions across groups

One scenario we would not support even if we go down this path is the following:

  • you open an editor and accumulate view state (selection, folding, etc)
  • you open a new group to the side (but don't change the editor in the original group)
  • via quick open you pick the file that is already opened
  • since the editor did not change in the original group, we do not save state to the memento, we only do that if the input changes for an editor or gets closed
  • if we wanted to support this scenario as well, we would need to explicitly ask the editor to save state when the same editor is opened in another group

@bpasero bpasero changed the title Add a setting or always restore the last editor state for editors opening in groups Add a setting or always restore the last editor state for editors opening in new groups Feb 22, 2021
@bpasero bpasero modified the milestones: Backlog, July 2021 Jul 9, 2021
@bpasero bpasero closed this as completed in 4fb7b80 Jul 9, 2021
@bpasero
Copy link
Member

bpasero commented Jul 9, 2021

Added a new setting workbench.editor.sharedViewState to control this. Not super convinced with the name yet, maybe you have a better idea.

Enabling this setting will:

  • always store the last memento that is persisted in a location that is group independent (aka survives group lifecycle)
  • will use this memento unless a more specific memento for the group is found

recording

From the gif, even though new (unknown) groups are opened, the files that open within get the state that was last persisted.

@bpasero bpasero added the verification-needed Verification of issue is requested label Jul 9, 2021
@jrieken
Copy link
Member Author

jrieken commented Jul 9, 2021

Thank you

@alexr00 alexr00 added the verification-steps-needed Steps to verify are needed for verification label Jul 29, 2021
@alexr00
Copy link
Member

alexr00 commented Jul 29, 2021

@mjbvz do you know the steps to verify this?

@roblourens
Copy link
Member

roblourens commented Jul 29, 2021

I tried but I don't understand this, I think I am following the same steps in the gif and descriptions but it doesn't load the persisted view state and I don't see a difference with the setting on or off.

@rzhao271
Copy link
Contributor

I also tried replicating the gif. When I split the editor, another copy of the file with the same highlighting shows up. I have both the restore and share settings enabled.

@mjbvz mjbvz added the verified Verification succeeded label Jul 29, 2021
@mjbvz
Copy link
Collaborator

mjbvz commented Jul 29, 2021

I marked this as verified based on the following test:

  1. Enable workbench.editor.sharedViewState
  2. Open a file to the side of the current editor
  3. Scroll partway down in this file and make a selection
  4. Close the entire editor group
  5. Now re-open the same file to the side of the current editor and confirm that the view position and selection are restored

@roblourens
Copy link
Member

roblourens commented Jul 29, 2021

Now re-open the same file to the side

How are you doing this? I am just curious now, it doesn't really matter, but I still don't see it. For me it doesn't matter if I do a split or a "new group" and open the file

@bpasero
Copy link
Member

bpasero commented Aug 2, 2021

Thanks for figuring it out, the steps are 👍

@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders papercut 🩸 A particularly annoying issue impacting someone on the team verification-needed Verification of issue is requested verification-steps-needed Steps to verify are needed for verification verified Verification succeeded workbench-state UI state across restarts
Projects
None yet
Development

No branches or pull requests

7 participants
@roblourens @bpasero @jrieken @rzhao271 @mjbvz @alexr00 and others