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

Save and restore sidebar subpanels sizes and expansion states #14901

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

DenisaCG
Copy link
Member

@DenisaCG DenisaCG commented Aug 2, 2023

This PR makes possible the full saving and restoration of the sidebars, including the subpanels sizes and expansion states.

References

It listens to the new signal introduced in this PR jupyterlab/lumino#614 in lumino, namely expansionToggled, such that the saving of the sidebar's layout is triggered when one of the subpanels is collapsed or expanded.

It also listens to the signal handleMoved, which makes it possible for the layout of the sidebar to also be saved when the vertical size of any of the subpanels is changed. The horizontal size of the entire sidebar is already being saved, as a consequence of the layout saving of the main dock panel.

Code changes

To allow for the full saving and restoration of the sidebar layout, the ISideArea interface has been extended, such that it includes two new properties sizes and expansionStates which will store the vertical sizes, and respectively the expansion state of each subpanel.

The listeners for the signals mentioned above are also initialized when a widget is added to the sidebar, in the case of an AccordionPanel or SidePanel.

When a saving of the sidebar's layout is triggered, the dehydrate method now extracts the vertical sizes, as well as the expansion states, while the rehydrate method will restore them.

User-facing changes

The users will now be able to see the sidebars' layout being automatically saved with any changes that might have been made, regarding sizes of subpanels or their expansion states, and respectively restored in the case of refreshing or reopening JupyterLab.

image

Backwards-incompatible changes

@jupyterlab-probot
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@welcome
Copy link

welcome bot commented Aug 2, 2023

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @DenisaCG

The code is good. We are only missing one point: the sizes and expansionStates must be stored per children of the sidebar stack panel.

Comment on lines 64 to 65
sizes: null,
expansionStates: [true, true, true, true]
Copy link
Member

Choose a reason for hiding this comment

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

Here is the trouble, a sidebar can have multiple panels (called widgets in this object). So sizes and expansionStates should exist per panel (child of the stack panel).

The panel are identify by a id for example

"left":{
  "collapsed":false,
  "visible":true,
  "current":"filebrowser",
  "widgets":["filebrowser","running-sessions","@jupyterlab/toc:plugin","extensionmanager.main-view"]
},
"right":{
  "collapsed":true,
  "visible":true,
  "widgets":["jp-property-inspector","debugger-sidebar"]
}

So I would use a object like

left/rightArea: {
  collapsed: boolean,
  currentWidget: string | null
  widgets: string[] | null,
  widgetStates: {
    [id: string]: {
     sizes: number[] | null,
     expansionStates: boolean[] | null
    }
  }
}

Does it make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh okay. I assumed that each panel would require its own sidebar instance so we would have multiple left and right ones, but this makes more sense. Thanks @fcollonval! I will make the changes.

@@ -1993,11 +2021,35 @@ namespace Private {
const collapsed = this._sideBar.currentTitle === null;
const widgets = Array.from(this._stackedPanel.widgets);
const currentWidget = widgets[this._sideBar.currentIndex];
let sizes = new Array<number>();
let expansionStates = new Array<boolean>();
Object.entries(this).forEach(([key, value]) => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is doing what you want. You should loop on this._stackedPanel.widgets not this I guess. And then test if panel.content is an instance of SplitPanel (you don't need to test for AccordionPanel as it inherits from SplitPanel).

Tip: you can directly use the local widgets variables.

Copy link
Member Author

@DenisaCG DenisaCG left a comment

Choose a reason for hiding this comment

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

Thanks for the review @fcollonval! I restructured the code such that the sizes and expansion states of the subpanels of each sidebar instance is included in widgetStates, each also being identified by their widget id. Unfortunately, it seems to be missing something as the layout is not saved and restored properly (this was fixed with the later commits).

@fcollonval
Copy link
Member

Thanks a lot @DenisaCG this is working as expected. The integration tests should pass after #14945 is merged.

Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

Thanks @DenisaCG

@fcollonval fcollonval merged commit 205d0b8 into jupyterlab:main Aug 22, 2023
76 of 79 checks passed
@welcome
Copy link

welcome bot commented Aug 22, 2023

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants