FileBrowser: Added the `sortNotebooksFirst` option #14497

tpatel · 2023-05-05T10:23:52Z

References

Fixes #14332

Code changes

Changes to filebrowser-extension:

New sortNotebooksFirst setting
New menu item shown when right-clicking on the filebrowser header

Changes to filebrowser:

Expose a sortNotebooksFirst property storing the setting internally as _sortNotebooksFirst that propagates changes to the listing
The listing exposes a setNotebooksFirstSorting method storing the setting internally as _sortNotebooksFirst. Modifying it triggers a sorting operation.
Updated the sorting method to take into account the two sorting behaviors.

User-facing changes

New setting in the filebrowser header setting menu (which opens when right-clicking on the header).

The behavior is:

When disabled, the file browser sorts the directories above all other files (doesn't modify the current behavior)
When enabled, the file browsers sorts the directories above everything, then the notebooks above other files (matches the Notebook V6 behavior).

Backwards-incompatible changes

None

closes jupyterlab#14332 - When disabled, the file browser sorts the directories above all other files (doesn't modify the current behavior) - When enabled, the file browsers sorts the directories abover everything, then all notebooks above other files (matches the Notebook V6 behavior)

jupyterlab-probot · 2023-05-05T10:23:54Z

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

welcome · 2023-05-05T10:23:54Z

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.

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! 🎉

for more information, see https://pre-commit.ci

jtpio · 2023-05-05T12:40:10Z

Thanks @tpatel for working on this!

* comments, console.log * fixed fallback sorting to be alphabetical ascending (instead of descending) * better menu label * added a test

krassowski

Looking good!

packages/filebrowser/src/browser.ts

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

fcollonval

Thanks @tpatel

I have some questions. But generally this looks good, thanks!

packages/filebrowser/src/listing.ts

fcollonval · 2023-05-10T07:49:14Z

packages/filebrowser/src/browser.ts

+  }
+
+  set sortNotebooksFirst(value: boolean) {
+    if (this.listing.setNotebooksFirstSorting) {


What is the reason for testing for setNotebooksFirstSorting?

As the listing is of type DirListing (no interface used), it will always exist, isn't it?

I couldn't find why the setColumnVisibility was being checked. I believe it should always exist but I was a bit confused and went on the safe side. Do you have more info about this?

https://github.com/jupyterlab/jupyterlab/blob/d47d50bbeb2db80df09489eb7c9588c29a140d22/packages/filebrowser/src/browser.ts#L164C7-L171

Given how downstream swaps listing, maybe it should become an interface? (this is not a discussion for this PR though). I just wanted to highlight that unfold extension goes pretty far with the modifications and depending on the version of DirListing it would load the guard may actually be useful (well at least if this lands in 4.x rather than 4.0). We also wanted to modify listing in jupyterlab-git but can probably make dedicated APIs for that use case.

Probably this condition was indeed added to make it easier for downstream applications to customize the file browser.

Thanks for providing the extra context, agree we can track follow-ups in other issues 👍

Ok then I'll agree we should keep the safe guard test. But I still would like to switch to a getter/setter pattern. @tpatel, you could replace the safeguard condition by

if (this.notebooksFirstSorted !== undefined)

switch to a getter/setter pattern

Would it be possible to address this in a separate PR? And address this for showLastModifiedColumn or showFileSizeColumn as well for consistency?

I think it makes sense to address moving both methods to the getter/setter pattern in a separate PR to avoid being in a state with two different patterns 👍

packages/filebrowser/test/listing.spec.ts

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

This reverts commit 1abef20. This shouldn't be commited until the discussion https://github.com/jupyterlab/jupyterlab/pull/14497/files/d47d50bbeb2db80df09489eb7c9588c29a140d22#r1189492709 is resolved.

jtpio · 2023-05-14T09:17:04Z

It would be great to get this one in so it makes it to the final 4.0 release, and would directly benefit Notebook 7.

Leaving it open until tomorrow in case @krassowski or @fcollonval have any more feedback.

jtpio

Thanks!

As discussed during the JupyterLab weekly meeting today, it would be great to get this PR in so it can be used in Notebook 7.

This can be seen as a new feature to be included in a 4.1 release. But this can also be considered a "bug fix" for feature parity with the classic notebook.

Also having it opt-in like it is the case in this PR makes it easier to ship in a 4.0.x release.

welcome · 2023-05-22T09:33:50Z

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[pre-commit.ci] auto fixes from pre-commit.com hooks

272f684

for more information, see https://pre-commit.ci

github-actions bot assigned tpatel May 5, 2023

github-actions bot added the pkg:filebrowser label May 5, 2023

jtpio added this to the 4.0.0 milestone May 5, 2023

jtpio added the enhancement label May 5, 2023

tpatel added 2 commits May 5, 2023 16:24

clean up

e12f4de

* comments, console.log * fixed fallback sorting to be alphabetical ascending (instead of descending) * better menu label * added a test

fixed test flakiness

40a5e29

krassowski reviewed May 8, 2023

View reviewed changes

packages/filebrowser/src/browser.ts Outdated Show resolved Hide resolved

Update packages/filebrowser/src/browser.ts

d47d50b

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>

tpatel marked this pull request as ready for review May 8, 2023 12:38

fcollonval added the tag:Feature Parity label May 10, 2023

fcollonval requested changes May 10, 2023

View reviewed changes

tpatel and others added 2 commits May 10, 2023 09:59

Update packages/filebrowser/src/listing.ts

1abef20

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

Revert "Update packages/filebrowser/src/listing.ts"

f3e9140

This reverts commit 1abef20. This shouldn't be commited until the discussion https://github.com/jupyterlab/jupyterlab/pull/14497/files/d47d50bbeb2db80df09489eb7c9588c29a140d22#r1189492709 is resolved.

jtpio mentioned this pull request May 14, 2023

Notebook 7, tree view should promote notebooks higher in the (default) tree view jupyter/notebook#6819

Closed

jtpio modified the milestones: 4.0.0, 4.0.x May 15, 2023

Merge branch 'main' into sort-notebooks-first

3b1df0f

jtpio approved these changes May 17, 2023

View reviewed changes

jtpio merged commit 1c55f92 into jupyterlab:main May 22, 2023

tpatel deleted the sort-notebooks-first branch May 22, 2023 13:00

jtpio mentioned this pull request May 30, 2023

Update to JupyterLab 4.0.1 jupyter/notebook#6892

Merged

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators May 22, 2024

Uh oh!

FileBrowser: Added the sortNotebooksFirst option #14497

FileBrowser: Added the sortNotebooksFirst option #14497

Uh oh!

Conversation

tpatel commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Code changes

User-facing changes

Backwards-incompatible changes

Uh oh!

jupyterlab-probot bot commented May 5, 2023

Uh oh!

welcome bot commented May 5, 2023

Uh oh!

jtpio commented May 5, 2023

Uh oh!

krassowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fcollonval left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fcollonval May 10, 2023

Choose a reason for hiding this comment

Uh oh!

tpatel May 10, 2023

Choose a reason for hiding this comment

Uh oh!

krassowski May 12, 2023

Choose a reason for hiding this comment

Uh oh!

jtpio May 14, 2023

Choose a reason for hiding this comment

Uh oh!

fcollonval May 14, 2023

Choose a reason for hiding this comment

Uh oh!

jtpio May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tpatel May 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtpio commented May 14, 2023

Uh oh!

jtpio left a comment

Choose a reason for hiding this comment

Uh oh!

welcome bot commented May 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FileBrowser: Added the `sortNotebooksFirst` option #14497

FileBrowser: Added the `sortNotebooksFirst` option #14497

tpatel commented May 5, 2023 •

edited

Loading

fcollonval left a comment •

edited

Loading

jtpio May 15, 2023 •

edited

Loading