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

Fix notebook URL opened in browser when redirect file not used #1326

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

randomir
Copy link

Fix #1325.

With notebook~=7.0 ServerApp is responsible for launching browser, not NotebookApp anymore. In notebook~=6.0, launch_browser will append notebook path for opening in browser even when use_redirect_file is set to false.

ServerApp used to do the same, last time in v1.3.0. The change in behavior/bug seems to be introduced in 1bbcbcb359, released as 1.4.0.

@welcome
Copy link

welcome bot commented Sep 18, 2023

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

@randomir
Copy link
Author

If this PR is on the right track, I can try adding some tests. Lmk.

@blink1073 blink1073 added the bug label Sep 19, 2023
@blink1073
Copy link
Collaborator

Hi @randomir, this seems like a reasonable change to me. @afshin, since you made the original change, do you agree?

With `notebook~=7.0` ServerApp is responsible for launching browser, not
NotebookApp anymore. In `notebook~=6.0`, `launch_browser` will append
notebook path for opening in browser even when `use_redirect_file` is
set to `false` - unlike ServerApp, ever since 60c66b6.
@randomir randomir force-pushed the fix/notebook-open-in-browser-without-redirect-file/issue-1325 branch from 00447c1 to 3083173 Compare September 19, 2023 21:29
@Zsailer
Copy link
Member

Zsailer commented Dec 6, 2023

Hi @randomir, this seems like a reasonable change to me. @afshin, since you made the original change, do you agree?

This looks right to me. @randomir do you want to take a crack at writing a test? Otherwise, I can take this PR over to get it merged.

@randomir
Copy link
Author

randomir commented Dec 7, 2023

@Zsailer, thanks for taking a look. I can take a crack at writing a test, and then ping you when I think it's ready for review.

@Zsailer
Copy link
Member

Zsailer commented Dec 7, 2023

Great! thanks @randomir! Let me know if you need any help or have any questions around our unit tests.

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

Successfully merging this pull request may close these issues.

Notebook not opened in browser when use_redirect_file=false set
3 participants