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

[WIP] Add colours for files in file browser #885

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Feb 25, 2021

Further exploring the proof-of-concept from jupyterlab/jupyterlab#5499 (comment).

Can fix #124 (the issue was open for a long time, it would be nice to resolve it for users who need it here and now).

To compare changes without the Git context menu: krassowski/jupyterlab-git@add-browser-context-menu...krassowski:colour-files-in-browser

Demo GIF:

demo-with-night

Instructions for easy testing on binder

To test on binder open console and paste

echo "*.ignored" >> ".gitignore"
mkdir test
cd test
echo "added" > added-file
echo "modified" > modified-file
echo "removed" > removed-file
echo "unstaged" > unstaged-file
echo "ignored" > file.ignored
git add modified-file removed-file
git config --global user.name "Your Name"
git config --global user.email "you@example.com"
git commit -m test
git add added-file
git rm --cached removed-file
echo "modification" >> modified-file
echo "Done; refresh the git status in GitPanel and then open FileBrowser (navigate to test directory)"

If in need, consult:

demo-colors

To avoid the requirement of using a manual action to refresh the status (triggering context menu, or visiting the GitPanel) I connected signals that:

  • trigger file browser refresh after each git status update, and/or,
  • trigger initial status refresh after entering a git repository

Known issues:

  1. Ignored files are not styled; this is because the ignored files are not a part of the model status.files: IStatusFile[] property. The refreshStatus() method uses the status endpoint which does not return ignored files (which might be good for performance).
    • is it worth to transfer all the ignored files just to be able to highlight them in the file browser (performance, vs utility trade-off)?

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch krassowski/jupyterlab-git/colour-files-in-browser

@krassowski krassowski force-pushed the colour-files-in-browser branch 5 times, most recently from 13377aa to 6921f9d Compare February 25, 2021 19:48
@krassowski
Copy link
Member Author

Ready for feedback. I listed two known issues which would be nice to get feedback on. The third issue is getting the setters exposed in JupyterLab. I will submit PR if you agree that this is a good approach for now (by now I mean JupyterLab 3.x).

@telamonian, is your new file browser on roadmap for JupyterLab 4.0, or more like 5.0?

@krassowski
Copy link
Member Author

krassowski commented Feb 25, 2021

While this PR does not add another column with an icon to the right, it would be easily possible by:

  • appending another span element to the node in updateItemNode of listing renderer, and
  • appending another div element to the node in populateHeaderNode of listing renderer

So we could implement the design from #124 fully using this approach (in particular the right-most panel of #124 (comment)). I am not full convinced on the utility of such design - colors work for me well enough, but if there is a consensus that another column should be added in addition to the colors I would be happy to tackle this (we could make it customizable with showStatusColor: boolean, showStatusColumn: boolean)

Alternatively, the file icons could get an overlay by simply adding a CSS style like

    '.jp-DirListing-itemIcon::after': {
      content: '"A"',
      position: 'absolute',
      top: '6px',
      left: '10px',
      background: 'rgba(255,255,255,0.7)',
      display: 'block',
      width: '11px',
      height: '16px',
      fontSize: '11px'
    }

Screenshot from 2021-02-25 20-49-57

It would look crammed to me though...

@krassowski krassowski marked this pull request as draft February 26, 2021 09:26
@krassowski
Copy link
Member Author

krassowski commented Feb 27, 2021

I implemented the @weihwang's indicator design from #124 (comment) and added settings so either the indicator, coloring or both can be disabled/enabled.

Screenshot from 2021-02-27 13-43-16
Screenshot from 2021-02-27 13-43-47

I would really appreciate an early feedback on whether you want to go this path (i.e. I should open a PR in JupyterLab to expose the renderer), or not.

@fcollonval
Copy link
Member

fcollonval commented Feb 28, 2021

Thanks @krassowski This is awesome work.

The design of #124 is certainly heavily inspired by VS Code. A reason for adding a DOM element with the file status may be accessibility (this is purely a guess) - coloring being probably though to translate for screenreader.

The trouble at JupyterLab level is indeed if multiple extensions want to apply a custom styles.

In VS Code they use the additional columns to concatenate the information. For example in the screenshot below, the 4 states that there are 4 warnings and M that the file is modified - according to git. The style are ranked as can be seen on index.ts that appears in red because 9 errors are detected and not in yellow because it is modified.

image

As I'm using VS Code daily. So my opinion is biased towards having a indicator in addition of the color 😁 But I agree with you stacking it with the file type icon is not easy to ready. And I found the @weihwang's indicator a bit to eye catchy. I prefer the more discrete approach of VS Code of a single colored letter.

Regarding ignored files, it won't be easy. We should list them as much as possible. So the best approach seems to focus on testing only the files displayed in the file browser.

I dig into VS Code logic. They are using the command git check-ignore -v -z --stdin. And it is called when the file browser is refresh and when a folder is expanded in the tree view (I don't have access to the input provided).

Adding setting to tune the look and feel is really a great addition

@krassowski
Copy link
Member Author

krassowski commented Feb 28, 2021

In VS Code they use the additional columns to concatenate the information. For example in the screenshot below, the 4 states that there are 4 warnings and M that the file is modified - according to git. The style are ranked as can be seen on index.ts that appears in red because 9 errors are detected and not in yellow because it is modified.

PyCharm on the other hand uses only colors to indicate git status (no letter indicators).

The trouble at JupyterLab level is indeed if multiple extensions want to apply a custom styles.

I'm also hitting the limits of the current Renderer pattern in the LSP extension when it comes to comlpeter. It feels like hooks could be easily implemented using lumino.Signal (e.g. file browser emits listingItemRendered: Signal<FileBrowser, HTMLSpanElement> and listeners are free to add whatever they want), though I am not sure what would be the performance implications. We could test it by force-swapping renderer and implementing this here before upstreaming in JupyterLab.

@fcollonval
Copy link
Member

I am not sure what would be the performance implications.

Yes this is gonna be definitely a point to monitor. Especially if there are lots of items - it may require implementing windowing the listing view.

We could test it by force-swapping renderer and implementing this here before upstreaming in JupyterLab.

Definitely worth experimenting and this extension offers a good opportunity. @ianhi did some testing on the changed files list in #667 (comment). It may be a source of inspiration to challenge the JupyterLab file browser with such signal. It is certainly a good subject to bring at the JupyterLab developer meeting to.

@fcollonval
Copy link
Member

@all-contributors please add @krassowski for code, review and bug

@allcontributors
Copy link
Contributor

@fcollonval

I've put up a pull request to add @krassowski! 🎉

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

Successfully merging this pull request may close these issues.

FileBrowser icons to show git status
2 participants