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

Adds a "Show in file browser" entry in tab context menu #4500

Merged
merged 2 commits into from
May 7, 2018

Conversation

telamonian
Copy link
Member

@telamonian telamonian commented Apr 30, 2018

Fulfills the request made in #4472. Adds a "Show in file browser" entry to the context menu for tabs. Clicking on this menu item will activate the file browser pane and select the currently active file.

One weakness is that it will only work with the currently open file, even if you right click on another tab. However, this seems to be a flaw in the context menu itself (since the same thing happens when you select "Rename...") rather than my patch.

I'm not particularly familiar with JS or TS, so if there's anything basic (style, spacing, etc) I should fix, please let me know

@telamonian telamonian changed the title Fulfills request in #4472 Fulfills request in #4472: adds a "Show in file browser" entry in tab context menu Apr 30, 2018
@telamonian telamonian changed the title Fulfills request in #4472: adds a "Show in file browser" entry in tab context menu Adds a "Show in file browser" entry in tab context menu Apr 30, 2018
@jasongrout
Copy link
Contributor

The focus issue is probably because the app's current widget app.shell.currentWidget doesn't change apparently by right-clicking on a tab.

@ian-r-rose
Copy link
Member

@jasongrout Yeah, I also left a comment about this in #4472. We may need to think some of the handling of the universal context menus, as dispatching entirely based on CSS selectors may not be enough information.

@telamonian
Copy link
Member Author

telamonian commented Apr 30, 2018

Some integration tests are failing, but I'm not entirely sure what the problem is, or even if there is one. AppVeyor ran one test successfully, hung and timed out in the middle of the second, and then cancelled the third.

Should I be concerned? Or is this SOP for AppVeyor?

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

Thanks @telamonian, this is looking good.

One thing that this won't work for: if the user has any file browsers installed that are not the main one (for instance, the Google Drive or GitHub browsers), 'navigate-main' will not be able to find them. It should be possible to also make this function work for those, but it would take some more work. We could try to do that here, or in a follow-up later.

label: () => `Show in file browser`,
isEnabled,
execute: () => {
if (isEnabled()) {
Copy link
Member

Choose a reason for hiding this comment

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

Typically this command won't be executed if isEnabled is not true, so this check is redundant.

isEnabled,
execute: () => {
if (isEnabled()) {
let context = docManager.contextForWidget(app.shell.currentWidget);
Copy link
Member

Choose a reason for hiding this comment

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

If the current widget is not a document, the context object will be undefined here. We should probably return without any action in that case.

@ian-r-rose
Copy link
Member

Also, I think the test failures were spurious. I have restarted the tester.

@telamonian
Copy link
Member Author

So, I worked out a fix for the tab focus issue. It's completely robust/reliable (at least in my own testing), though it does smell a bit.

Basically, JupyterLab objects will now capture the most recent contextmanager Event in a contextEvent property. The code in the showInFileBrowser command can then inspect contextEvent.target in order to find the path information it needs.

Annnd, while I was uploading it I now see there's a code review. Whoops

@telamonian telamonian force-pushed the open-tab-in-browser branch from 201b107 to db44d4a Compare May 2, 2018 16:17
@telamonian
Copy link
Member Author

I rolled back the focus fix (since I think it's complex/messy enough to need its own PR) and applied the requested changes.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

Thanks @telamonian. I think we probably want to think through a bit more how to handle the context-menu/active-widget problem, so I don't view that bug as a blocker for your PR. That was a clever solution, though!

isEnabled,
execute: () => {
let context = docManager.contextForWidget(app.shell.currentWidget);
if (context == null) {
Copy link
Member

Choose a reason for hiding this comment

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

I think docManager.contextForWidget returns undefined if it doesn't find the context (thanks, Javascript, for having two falsy values!). We can check for both by doing if (!context)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change it. I was under the impression (from the three seconds of reading docs that I just did) that if (x == null) tests for both null and undefined (unlike if (x === null) which is "strict" and only tests for null). Do you know if that's correct?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you may be right. We usually try to avoid the coercive == just to avoid these types of confusions :)

@telamonian telamonian force-pushed the open-tab-in-browser branch from db44d4a to 25aff6b Compare May 2, 2018 16:35
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

Thanks @telamonian, this works great!

@ian-r-rose ian-r-rose merged commit 29a939a into jupyterlab:master May 7, 2018
@ian-r-rose
Copy link
Member

We can fix the focus issue in a follow-up, once we decide to proceed.

telamonian added a commit to telamonian/jupyterlab that referenced this pull request May 16, 2018
telamonian added a commit to telamonian/jupyterlab that referenced this pull request May 16, 2018
telamonian added a commit to telamonian/jupyterlab that referenced this pull request May 16, 2018
telamonian added a commit to telamonian/jupyterlab that referenced this pull request May 17, 2018
@jasongrout jasongrout added this to the Beta 3 milestone Jun 12, 2018
telamonian added a commit to telamonian/jupyterlab that referenced this pull request Jul 27, 2018
blink1073 pushed a commit that referenced this pull request Aug 10, 2018
* fix for context menu problems in #4500/#4529

* Generalized the focus fix
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 8, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants