Skip to content

Conversation

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Jul 25, 2020

This PR implements an improved single document mode, focusing on the URL scheme, state, and interactions across multiple browser tabs.

Todo:

  • Discuss API of launcher model and widget, as it relates to adding mode. Removed mode from launcher for now.
  • Get tests to pass - looks like there are issues with kernel management that are causing apputils tests to fail. This may be a Hiesenbug - tests are passing fine locally.
  • This PR adds IInfo.workspace, but not sure that makes sense - everything else is using PageConfig. Have removed this from IInfo, we will use PageConfig for now.

References

#8292

Code changes

  • PageConfig now contains a .mode attribute with a value of either single-document or multiple-document.
  • PageConfig now has workspace and treePath values to contain the name of the workspace, and the tree path if any. Any mutations of the URL need to update the valued in the PageConfig.
  • The DocumentManager has a mode attribute that is kept in sync with the ILabShell.mode value. When DocumentManager.mode is set to single-document double clicking on a file in the file browser will open it in a new browser tab and clicking on the + button in the file browser will open a new browser tab with the launcher open. The launcher creates widgets on the current page.
  • A number of code paths that were forcing the L panel to hide show in page load have been removed to alwys defer to the layout state. The overrides are limited to empty layout states.
  • The ILabShell has new modeChanged and currentPathChanged signals that fire on the document mode changing and the path of the current document changing respectively.
  • IRouter.INavOptions now has an optional skipRouting attribute that will cause the router to skip all routing and just update the URL.
  • A new PageConfig.getUrl function is provided to make it easy to build URLs that adhere to the master URL pattern for the application (given a mode, workspace, and tree path).
  • IOpenOptions now has an optional maybeNewWorkspace attribute that will cause document opens to be done in a new browser tab if the DocumentManager is in single document mode.

User-facing changes

  • State restoration of the L/R side panels is working again.
  • The application URL will always show the tree path of the currently active document in the main work area. If the current widget is not a document, the path of the default file browser is shown in the URL. These changes also update PageConfig.
  • In single document mode, the following actions always open a new browser tab: double-clicking a file to open it, clicking the + button in the default file browser.

Backwards-incompatible changes

  • The '@jupyterlab/application-extension:main' extension now provides the ITreePathUpdater interface, which is a function that can be called to update the treePath encoding in the page URL.
  • The workspacesUrl and treeUrl valued have been removed from PageConfig as they are no longer relevant.
  • The document mode has been removed from the layout state and put into the URL. The global URL pattern is now /(lab|doc)(/workspaces/[a-zA-Z0-9\-\_]+)?(/tree/.*)? where the /lab prefix maps onto multiple-document mode and /doc maps onto single-document mode.
  • ILabShell.restoreLayout now takes two arguments, the mode and the layout. This was needed because the mode has been removed from the layout and put into the URL.
  • Auto generated workspaces no longer clone the workspace, and instead always reset to an empty state.

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@ellisonbg
Copy link
Contributor Author

Anyone with experience with puppeteer and the window.jupyterlab object? In this commit I am trying to write a test, but window.jupyterlab is undefined. Thoughts?

bcc11c9

@ellisonbg
Copy link
Contributor Author

Ahhh, I guess browser_test.py wasn't setting exposeAppInBrowser in the page config. That fixed it.

@ellisonbg
Copy link
Contributor Author

I dove into testing this with puppeteer and ran into a bunch of issues. @blink1073 and I believe that we should wait until we have the new browser testing framework that @mbektasbbg is proposing to merge into the org:

jupyterlab/frontends-team-compass#77

@ellisonbg
Copy link
Contributor Author

A couple of follow up things:

  • In single document mode, when a user double clicks on a file in the file browser to open it in a new tab, we are running into issues with the popup blocker, even though it appears the code is in a click handler in a relatively direct manner.
  • In layout restoration, the launcher opens in the black dock panel before a document has a chance to restore. We should defer the launcher creation until after full restoration is complete.

@blink1073
Copy link
Contributor

In single document mode, when a user double clicks on a file in the file browser to open it in a new tab, we are running into issues with the popup blocker, even though it appears the code is in a click handler in a relatively direct manner.

Are you not seeing this on /tree? If so there might be a hint in how that is implemented

@ellisonbg
Copy link
Contributor Author

To try this out:

  1. Install the alpha of jupyterlab_server using pip install jupyterlab_server --pre
  2. Do a regular dev install of this branch.

@blink1073
Copy link
Contributor

If you rebase, we already depend on the alpha version of jupyterlab_server in master.

@afshin
Copy link
Member

afshin commented Aug 5, 2020

For the default workspace, we could create a constant that we compare against and know could never be in a URL. One place we use a special string is in the setting registry:

/**
 * The ASCII record separator character.
 */
const RECORD_SEPARATOR = String.fromCharCode(30);

Or in this case, we might consider using the null character: const DEFAULT_WORKSPACE = String.fromCharCode(0);

@ellisonbg
Copy link
Contributor Author

I rebased on master so this branch now depends on the preview of jupyterlab_server, so you won't need to separately install that as long as you do pip install -e . when building this branch. Thanks @blink1073 !

@ellisonbg
Copy link
Contributor Author

The most recent commit requires this PR from jupyterlab_server:

jupyterlab/jupyterlab_server#112

@jasongrout jasongrout mentioned this pull request Aug 10, 2020
43 tasks
@ellisonbg
Copy link
Contributor Author

I have addressed all feedback and made all changes that are needed. I am seeing tests pass locally, so this may be a hiesenbug in the test suite for kernels.

@blink1073
Copy link
Contributor

apputils is passing but there are other failures

@blink1073
Copy link
Contributor

I'm looking into the Python failures separately, I see them in another PR as well.

@blink1073
Copy link
Contributor

Kicking CI

@blink1073 blink1073 closed this Aug 11, 2020
@blink1073 blink1073 reopened this Aug 11, 2020
Copy link
Member

@afshin afshin left a comment

Choose a reason for hiding this comment

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

Thanks, @ellisonbg! This looks ready to merge. Any UX decisions we make going forward should not modify these APIs so 👍

@ellisonbg
Copy link
Contributor Author

Everything looks good on this, ready to merge :-)

@afshin afshin merged commit 67de374 into jupyterlab:master Aug 11, 2020
jasongrout added a commit to jasongrout/jupyterlab that referenced this pull request Jan 22, 2021
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Feb 8, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

pkg:application pkg:apputils pkg:coreutils pkg:docmanager pkg:docregistry pkg:filebrowser pkg:launcher 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