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

[MRG] add open_browser trait to ExtensionApp #128

Merged
merged 3 commits into from
Dec 14, 2019

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Oct 25, 2019

Addresses #120.

Adds a trait to ExtensionApps to configure opening browser windows on launch.

@vidartf
Copy link
Member

vidartf commented Oct 28, 2019

Should this also get a flag (inverted): --no-browser ?

@Zsailer
Copy link
Member Author

Zsailer commented Nov 8, 2019

The ExtensionApp actually already inherits the --no-browser flag from ServerApp before this PR (see here).

@Zsailer Zsailer changed the title add open_browser trait to ExtensionApp [MRG] add open_browser trait to ExtensionApp Nov 8, 2019
@vidartf
Copy link
Member

vidartf commented Nov 15, 2019

That inherited flag reads as:

flags["no-browser"] = (
    {"ServerApp": {"open_browser": False}},
    _("Prevent the opening of the default url in the browser."),
)

Is it not an issue that it says ServerApp (the extension app does not inherit it)? If so, I imagine some other flags/aliases might have an issue too.

@echarles
Copy link
Member

@vidartf As far as I have seen, the flags handling does not work as expected. I am looking at this and will open on issue (hopefully a PR). We can revisit this PR based on the findings/fixes we will get for the more global flag issue.

@echarles
Copy link
Member

echarles commented Nov 16, 2019

@Zsailer @vidartf

This branch fails on jupyter server --open-browser false [C 11:44:23.991 ServerApp] Unrecognized flag: '--open-browser'

I had that issue at other places (#117 and jupyterlab/jupyterlab#7416).

I have opened #140 to further discuss this before coming back here.

@vidartf
Copy link
Member

vidartf commented Nov 18, 2019

@echarles That is not how it is intended to be used though. jupyter notebook --open-browser false also fails.

@vidartf
Copy link
Member

vidartf commented Nov 18, 2019

PS: The right ways are either --NotebookApp.open_browser=False or via the flag --no-browser.

@vidartf
Copy link
Member

vidartf commented Nov 18, 2019

PPS: Or in this case --ServerApp.open_browser=False or the flag.

@echarles
Copy link
Member

After yesterday discussion at server weekly meeting, I now get the point that the extension flags should only be applied to an extension launched via its entrypoint.

BTW, this PR does not define flag for open_browser, so my previous attempt was out of context... Sorry for the confusion.

Going back to the parameters approach, I have tried to launch with --ServerApp.open_browser=False or --ExtensionApp.open_browser=False and it does not work (the browser if opened and it should not be).

Digging a bit more into the base ExtensionApp, I have the impression that the traits given via CLI are not taken into account by the ExtensionApp. e.g. jupyter simple-ext1 --ServerApp.custom_display_url=foo has no impact on the displayed URL.

Looking at the code that prepare the settings and printing those settings in the log, I feel that something must be done to feed the extension CLI config to the settings. Any input on this?

@Zsailer Zsailer added this to the 0.2.0 Release milestone Dec 2, 2019
@Zsailer
Copy link
Member Author

Zsailer commented Dec 11, 2019

Is it not an issue that it says ServerApp (the extension app does not inherit it)? If so, I imagine some other flags/aliases might have an issue too.

@vidartf I don't think this is a problem. These traits should affect the ServerApp directly.

open_browser is a special case because ExtensionApps can affect the ServerApp.open_browser trait using its own open_browser trait.

@Zsailer
Copy link
Member Author

Zsailer commented Dec 14, 2019

Merging, since I've addressed the concerns above. We can open a future PR if needed.

@Zsailer Zsailer merged commit 1ba5a57 into jupyter-server:master Dec 14, 2019
@Zsailer Zsailer deleted the extension-browser-open branch January 10, 2020 17:34
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.

3 participants