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

Notebooks with extension other than ipynb are displayed as text #4924

Closed
mwouts opened this issue Jul 18, 2018 · 11 comments
Closed

Notebooks with extension other than ipynb are displayed as text #4924

mwouts opened this issue Jul 18, 2018 · 11 comments

Comments

@mwouts
Copy link

@mwouts mwouts commented Jul 18, 2018

Alternative contents manager allow to open notebooks from files with other extensions that ipynb, and that work very well in jupyter notebook.

However, this does not appear to work in jupyter lab (see also #3896).

In the below I have isolated a very simple content manager, that does nothing else than opening files with extension .ipynb2 as notebooks. That content manager works fine in jupyter notebook, but not in jupyter lab, where the notebook with extension .ipynb2 (a copy of a valid notebook), is displayed as text:

image

For that test my jupyter config was

import notebook.transutils
from notebook.services.contents.filemanager import FileContentsManager


class SampleFileContentsManager(FileContentsManager):
    def get(self, path, content=True, type=None, format=None):
        """ Takes a path for an entity and returns its model"""
        path = path.strip('/')

        if self.exists(path) and \
                (type == 'notebook' or
                 (type is None and (
                         path.endswith('.ipynb') or path.endswith(
                     '.ipynb2')))):
            return self._notebook_model(path, content=content)
        else:
            return super(SampleFileContentsManager, self) \
                .get(path, content, type, format)


c.NotebookApp.contents_manager_class = SampleFileContentsManager
@grst
Copy link
Member

@grst grst commented Jul 19, 2018

Setting the notebook icon is implemented in

let ft = this._manager.registry.getFileTypeForModel(item);

and uses getFileTypeForModel
getFileTypeForModel(

which correctly identifies ipynb2 as notebook.


Whereas opening files is implemented in

const factories = registry.preferredWidgetFactories(path).map(f => f.name);

and uses getFileTypeForPath
getFileTypesForPath(path: string): DocumentRegistry.IFileType[] {

which relies on the extension only and therefore does not get the notebook type.


@jasongrout is there a design rationale behind this or is that something that should be changed for consistency?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

The status quo, like you point out, is that we have multiple viewers, and the viewer is chosen before the file is opened (with Open with, or simply as the default viewer). Right now, the default viewers and options for the Open with menu are all made from file extensions, not from the types returned from the contents manager.

I like your implicit suggestion: we have a specific override for the notebook file type, like in the icon code you point out, so we don't look at the file extension registry unless the file contents model declares it as of type file.

The one disadvantage to that is that it won't be possible to open notebook files as text anymore. Actually, that may good right now, since we have a problem with opening a file with both text and notebook models, in that the views will not sync, which is surprising to many people. Perhaps we just suggest that in this case, you rename the file to another file extension, open the file in the text editor and do whatever you want, then rename back to a notebook file extension.

@jasongrout jasongrout added this to the Beta 4 milestone Jul 19, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

Another approach would be to prefer a default notebook file type viewer if the contents model indicates it is a notebook, but also merge in the Open with menu options coming from the file extension.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

Assigning to the next milestone so we can look at this soon.

If you'd like to submit a PR, that would be great!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

It looks like the PR would basically change preferredWidgetFactories to somehow determine if we were dealing with a notebook file (based on the model), and insert the notebook editors first in the list, or perhaps replace the list with the notebook editors?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 19, 2018

Or perhaps a simpler PR would be to edit

if (model.type !== 'directory') {
to do a check for the notebook type.

I think you've investigated the codebase well enough to see a few ways to fix this. Can you submit a PR?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 19, 2018

Note: we should have some other way to edit notebook metadata if we don't allow opening as a text file.

@grst
Copy link
Member

@grst grst commented Jul 20, 2018

@blink1073, should I open a separate issue for that? I believe that a separate metadata editor like in jupyter notebook is more user-friendly anyway.

@jasongrout, the simplest PR you propose sounds pretty straightforward. I can try that.
But I have hardly any experience with TypeScript nor did I work with the jupyterlab codebase before.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jul 20, 2018

We have an issue for it here, I was on my phone when I typed the previous comment 😉.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jul 20, 2018

@grst, that is why we are particularly excited to help you make a pr :).

@grst
Copy link
Member

@grst grst commented Aug 2, 2018

I'm afraid I won't have time looking into this until September.

@blink1073 blink1073 removed this from the 0.34 milestone Aug 13, 2018
@blink1073 blink1073 added this to the 0.35 milestone Aug 13, 2018
grst added a commit to grst/jupyterlab that referenced this issue Aug 30, 2018
Text-files, that can be interpreted as notebook by an alternative
ContentManager get an 'Open With' -> 'Notebook' entry
in the FileManager context menu.
blink1073 added a commit that referenced this issue Aug 31, 2018
Text-files, that can be interpreted as notebook by an alternative
ContentManager get an 'Open With' -> 'Notebook' entry
in the FileManager context menu.
@blink1073 blink1073 removed this from the 0.35 milestone Sep 5, 2018
@blink1073 blink1073 added this to the 1.0 milestone Sep 5, 2018
@blink1073 blink1073 mentioned this issue Sep 28, 2018
31 tasks
@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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants