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

Implement Git context menu in the file browser #877

Merged
merged 16 commits into from
Mar 5, 2021

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Feb 20, 2021

Fixes #774

Changes:

  • Added Git context menu in File Browser
  • Reworked the context commands to accept multiple files (as multiple files can be selected in the file browser)
  • Added icons to the commands (they also display in the context menu of the Git panel widget)
  • The old context menu and new context menu share the same commands and the same status assignments (the "open" command is not shown in the file browser as redundant to the browser capabilities); EDIT: "delete" is no longer shown either (GIF was not updated)
  • Fixed a typo (_getPathRespository)
  • Formalize types:
    • for context commands (as distinct from other commands): ContextCommandIDs
    • for context commands' arguments (as those were already being cast in type-unsafe way and I needed to change them all): CommandArguments

jupyterlab-git-context-menu

The Git panel context menu got icons (updated):

context-menu-in-git-panel

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch krassowski/jupyterlab-git/add-browser-context-menu

@krassowski
Copy link
Member Author

Nice binder badge action. I am not sure what rank should the Git menu have (should it be lower or higher?). I just randomly used 5 and it seems like a good position for me but might be sub-optimal.

@krassowski krassowski force-pushed the add-browser-context-menu branch 2 times, most recently from 097f01e to 2fa0bd6 Compare February 20, 2021 04:07
@krassowski
Copy link
Member Author

This is also a significant step towards #379.

@ianhi
Copy link
Collaborator

ianhi commented Feb 20, 2021

I really like this - thanks for working on this!

The Git panel context menu got icons:

❤️

The old context menu and new context menu share the same commands and the same status assignments (the "open" command is not shown in the file browser as redundant to the browser capabilities)

Perhaps the git menu shouldn't have delete as that is also redundant?

Also this made me realize that maybe for delete on untracked files we should use the same x as the file browser rather than the - that is used for unstage.

@krassowski
Copy link
Member Author

krassowski commented Feb 20, 2021

I changed the icon for delete, I think that it was me who wrongly assigned it and it was not used like that before. I also agree that delete should not be shown in the file browser context menu - changed it now too.

Also now tested that it works with git repository which is not in the JupyterLab root:

See GIF

works-if-git-not-a-top-repo

Outside of a git repository:

See GIF

outside-of-git-repo

Or within the git repository but when no action is available (which happens for files that are tracked but not changed, as jupyterlab-git does not implement git rm nor any other relevant command):

See GIF

known-file-no-changes

@fcollonval
Copy link
Member

Thanks for this awesome PR @krassowski

The change for the delete icon is welcomed indeed.

I am not sure what rank should the Git menu have (should it be lower or higher?)

I don't have any preferences neither. The current position seems ok for me too.

Some comments:

  • In the Git panel context menu, it looks like the icon are not vertically center with the text (they are a bit up). Is it possible to correct that?
  • I have an error when opening the diff for multiple files at once. Only one diff is shown correctly - note: I tested this only with plain text files not with notebooks
  • If multiple files are selected, some being untracked and some being tracked, two entries appears in the context menu - Stage and Track. But if I click on Stage they all get added and/or staged. I think this is nice to do the action for all files - but it maybe good to have only one entry in the context menu.

@krassowski
Copy link
Member Author

krassowski commented Feb 21, 2021

In the Git panel context menu, it looks like the icon are not vertically center with the text (they are a bit up). Is it possible to correct that?

I see that too. I was also looking at the trashIcon where the problem is even worse. It seems that the icons were fine-tuned for their use in the GitPanel rather than placed in the center and then fine tuned in the GitPanel via CSS. I do not know which makes more sense:

  • to fine-tune position in menu via bindattrs/css
  • to correct icons to be all the same size and centered and then adjust their use in GitPanel

Edit: looking at this again, it does not happen in the file browser context menu so maybe some styles are missing for the simple context menu on panel?

I have an error when opening the diff for multiple files at once. Only one diff is shown correctly - note: I tested this only with plain text files not with notebooks

Yes, I can reproduce that. I believe that it is not an effect of the changes that I made but some limitation of the diff renderer. I was also seeing this bug when I was opening multiple diffs manually in previous releases. My thinking is that either:

  • the tab with diff needs to be focused
  • the stream from server breaks when another diff gets open

But those are just guesses. Would be great to investigate and fix (possibly in another PR).

Edit: removed third part I misread.

@adammclaurin
Copy link

Does this also resolve #864?

@krassowski
Copy link
Member Author

No, there is no context command for "show file history" just yet. Another main area widget with history browser would need to be implemented first I think...

But what this PR does is making it super easy to add such a command to the context menu once it gets contributed.

@fcollonval
Copy link
Member

Would be great to investigate and fix (possibly in another PR).

Agree I opened #881

What is your opinion on the third point I raised:

If multiple files are selected, some being untracked and some being tracked, two entries appears in the context menu - Stage and Track. But if I click on Stage they all get added and/or staged. I think this is nice to do the action for all files - but it maybe good to have only one entry in the context menu.

@krassowski
Copy link
Member Author

What is your opinion on the third point I raised:

If multiple files are selected, some being untracked and some being tracked, two entries appears in the context menu - Stage and Track. But if I click on Stage they all get added and/or staged. I think this is nice to do the action for all files - but it maybe good to have only one entry in the context menu.

Yes, the distinction between stage and track is artificial. But it appears to me that the current logic is neat and maintainable (aggregate the commands from context menus for specific files and show all unique entries). I can however, add extra logic which replaces the two with a tailored "Add" command.

@fcollonval
Copy link
Member

But it appears to me that the current logic is neat and maintainable (aggregate the commands from context menus for specific files and show all unique entries). I can however, add extra logic which replaces the two with a tailored "Add" command.

Ok let keep it simple for now.

@krassowski
Copy link
Member Author

krassowski commented Feb 25, 2021

I merged them in 3cb6109

@krassowski krassowski force-pushed the add-browser-context-menu branch from 9e64833 to f5df357 Compare February 25, 2021 15:52
src/model.ts Outdated Show resolved Hide resolved
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.

I have a very small performance suggestion. Otherwise LGTM

@ianhi
Copy link
Collaborator

ianhi commented Mar 3, 2021

I agree, this is a great thing. One thing to consider in the future would be to add an icon for git ignore so the ignore options don't look out of place in the context menu.

krassowski and others added 2 commits March 4, 2021 20:58
Co-authored-by: Frédéric Collonval <[email protected]>
without warning if there are more matches
Copy link
Member

@telamonian telamonian left a comment

Choose a reason for hiding this comment

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

This looks great @krassowski!

My only nit is the track item. In all cases (ie track and add/stage) what you're really doing is staging the file, so it's confusing to call it anything other than just stage in all cases. I know I was confused for a second when I saw track pop up in your first gif.

@krassowski
Copy link
Member Author

Track/Stage was collapsed into a single Add in the browser menu, the top gif was not updated There still is a distinction in the git panel context menus (as it was there before)

@fcollonval fcollonval merged commit a558c76 into jupyterlab:master Mar 5, 2021
@fcollonval
Copy link
Member

Thanks @krassowski

@fcollonval
Copy link
Member

@meeseeksdev backport to jlab-2

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab-git that referenced this pull request Mar 5, 2021
fcollonval added a commit that referenced this pull request Mar 6, 2021
…on-jlab-2

Backport PR #877 on branch jlab-2 (Implement Git context menu in the file browser)
@krassowski krassowski deleted the add-browser-context-menu branch March 6, 2021 14:15
@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/what-does-ifilebrowserfactory-defaultbrowser-do/9702/2

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.

"git add" from file browser?
6 participants