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

Support Windows Paths #5051

Closed
dhirschfeld opened this issue Aug 5, 2018 · 37 comments
Closed

Support Windows Paths #5051

dhirschfeld opened this issue Aug 5, 2018 · 37 comments
Labels
bug pkg:filebrowser status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Milestone

Comments

@dhirschfeld
Copy link
Member

...as often (almost always) it won't fit and the most important part, the filename itself, is currently being truncated:

image

@dhirschfeld
Copy link
Member Author

I think the PyCharm UI here is the ideal to strive towards. In PyCharm only the filename is displayed, unless there is another tab with the same name open in which case parent folders are shown until the tabs are uniquely identified.

e.g. for the folders shown below only the filename and parent folder are shown as those are sufficient to uniquely identify the file:

/scripts/test/run.py
/scripts/data/export/run.py

image

The key aspect for me is the most important information is chosen to be displayed - the least important information is the root of the filesystem as it is common to everything.

@jasongrout
Copy link
Contributor

Interesting. The path doesn't show for me, just the filename (firefox, macOS):
screen shot 2018-08-04 at 10 11 52 pm

What version of JupyterLab do you have? What browser? From the path it seems you are on Windows?

@dhirschfeld
Copy link
Member Author

dhirschfeld commented Aug 5, 2018

I'm running 33.4 on win64, chrome 67.0.3396.99

Edit: Also seeing this in Firefox Developer 62.0b11

@dhirschfeld
Copy link
Member Author

Ok, confirmed that this happens when you use Open from Path to open a file rather than the FileBrowser:

image

@jasongrout jasongrout added this to the 0.34 milestone Aug 5, 2018
@blink1073
Copy link
Contributor

blink1073 commented Aug 11, 2018

Hi @dhirschfeld, two questions:

  • Where did you launch jupyterlab from?
  • How did you enter the path in the Open from Path dialog?

I wasn't able to repro on Win 10 in Chrome 68.

@dhirschfeld
Copy link
Member Author

JupyterLab is launched with jupyter labhub ... from JupyterHub. JupyterLab itself is running in a Windows Container. The full path name C:\Miniconda3\lib\base64.py was pasted in the Open from Path dialog.

I can do some further testing when I'm back from holiday in a weeks time...

@blink1073
Copy link
Contributor

Thanks, enjoy!

@blink1073
Copy link
Contributor

I'm confused as to how pasting the full file path opened anything. The open file path is supposed to take a server path, with starts with / at the server root directory.

@dhirschfeld
Copy link
Member Author

dhirschfeld commented Aug 12, 2018

I'll double check that. Was just quoting from memory. I'm sure I copied the path from an exception to open the file. That's a very useful capability to have IMHO which is why I start my server in C:\ - prob why it worked?

@blink1073
Copy link
Contributor

What I tried locally was launching from C: and trying to open C:/Users/<user>/Untitled.ipynb, and I got a file not found error.

@blink1073
Copy link
Contributor

Jupyter does not map directly to file system paths on purpose, because your "file system" could be a remote database. I'm not sure how I feel about allowing raw paths like that in general...

@blink1073 blink1073 modified the milestones: 0.34, 0.35 Aug 13, 2018
@dhirschfeld
Copy link
Member Author

Yep, C:\Miniconda3\lib\base64.py works perfectly well for me, except that it displays the full path in the tab title.

NB: If I use forward slashes C:/Miniconda3/lib/base64.py it also works and it just displays the filename in the tab title. So it appears that the path stripping logic doesn't seem to handle \

I'm about to update to 0.34 so can check there too...

@blink1073
Copy link
Contributor

The path utilities we're using in the browser assume unix-like paths, so we'd have to sanitize the input in that dialog first.

@blink1073
Copy link
Contributor

Specifically, here.

@dhirschfeld
Copy link
Member Author

Jupyter does not map directly to file system paths on purpose, because your "file system" could be a remote database. I'm not sure how I feel about allowing raw paths like that in general...

Just a note to say that being able to open files on the file system is critical functionality for me for debugging purposes.

At present debugging is restricted somewhat to opening the library file and instrumenting with print statements but I can imagine a future where you could put a breakpoint in the file and step through it in the JupyterLab Debugger built on top of the Debug Adaptor Protocol

@blink1073 blink1073 modified the milestones: 0.35, 1.0 Sep 5, 2018
@dhirschfeld dhirschfeld changed the title In the tab title only display the filename, not the full path Support Windows Paths Sep 25, 2018
@dhirschfeld
Copy link
Member Author

So, originally Open From Path allowed you to open a path with backslashes with the only bug being that the full path name was displayed in the tab title.

I'm now on 0.34.11 and paths with backslashes in simply don't work at all :(

I can of course manually edit all backslashes but that's pretty a painful UX for Windows users.

@afshin afshin added DISC2018 and removed DISC2018 labels Oct 25, 2018
@briansgo
Copy link

briansgo commented Nov 7, 2018

Hello
Is somebody working on this?
If not, I would like to, if you agree :)

@dhirschfeld
Copy link
Member Author

I don't think so, so go for it if you want!

@briansgo
Copy link

I can of course manually edit all backslashes but that's pretty a painful UX for Windows users.

Do you think that automatically replacing all backslashes to forward slashes would solve the problem?

@saulshanabrook
Copy link
Member

@Zsailer So this issue would be fixed in Jupyter Server by fixing this issue jupyter-server/jupyter_server#34? If so, I would like to move this off 1.0.

@saulshanabrook
Copy link
Member

TODO @jasongrout:

Try to reproduce this on a windows machine and see what is coming from the server.

@jasongrout
Copy link
Contributor

jasongrout commented May 23, 2019

I reproduced this. I created C:\test.txt and started jupyterlab from C:\. I did File | Open from path... and typed in C:\test.txt. This sent an api request to http://localhost:8888/api/contents/C%3A%5Ctest.txt?content=0&1558646027657. The response to this was:

{
  name: 'C:\\test.txt',
  path: 'C:\\test.txt',
  last_modified: '2019-05-23T21:13:19.666963Z',
  created: '2019-05-23T21:13:19.666337Z',
  content: null,
  format: null,
  mimetype: 'text/plain',
  size: 0,
  writable: true,
  type: 'file'
}

You'll notice that the name that came back included the C:\.

If I do the same thing with C:/test.txt, the response name comes back as test.txt.

So we could say this is a server issue, or we could say that the server api accepts unix-style paths, and we are not conforming to the api by sending a path that looks like C:\.

@jasongrout
Copy link
Contributor

@ivanov and I discussed this, and we think this is a problem with the server. The server api should not accept backslash paths, or paths that start with a drive letter. The server api is supposed to respond to url-style forward-slash-delimited paths, relative to the server root. We think there should be a different way to map between absolute paths given by a kernel and these server-relative paths. I'll open an issue in the notebook server.

@jasongrout
Copy link
Contributor

I created jupyter/notebook#4640. We'll close this for now as an upstream issue - feel free to continue the discussion on jupyter/notebook#4640

@dhirschfeld
Copy link
Member Author

From a UX perspective, I'd like the to be able to open files by copying and pasting paths - which on Windows means accepting standard backslashes. It's very painful (and Windows hostile) to force users to manually change every backslash to a forward-slash.

@jasongrout
Copy link
Contributor

There are two separate potential issues I see here:

A. Windows-style backslash paths vs url-style forward-slash paths that the api expects. We could provide this translation in JupyterLab, perhaps keyed to a setting. The C:\ paths still should probably not work (being absolute), but I agree that replacing \ with / manually for relative paths is painful.

B. Mapping absolute paths you see from kernels (like in tracebacks) to server-relative paths. The frontend can't really do this since it doesn't know how kernels and the server are related. Perhaps the server can provide some information to the frontend about how to appropriately map kernel paths to server contents api paths for various kernels. For example, in your case the server could tell the frontend "For this kernel, when you see a path like C:\myproject\mydir\*, that corresponds to a server contents api path of /mydir/*. Ideally the kernel could tell the server what its current working directory was so we could even map relative paths, but perhaps we could just start with absolute paths.

@jasongrout
Copy link
Contributor

We could reopen this issue for a setting that would translate backslash paths in "Open from path..." to forward-slash paths. Would that ease the pain in the short term?

@dhirschfeld
Copy link
Member Author

I don't think you need any fancy translation since passing an absolute path actually works - all I would suggest is normalising \ to / and for security possibly check that the path starts with the server root so you can't "break out" - e.g. pseudo-python-code:

path = path.replace('\\', '/')
if not path.startswith('/'):
	assert path.startswith(self.serverRoot)
contents = self.get_contents(path)
name = os.path.basename(path)

@dhirschfeld
Copy link
Member Author

The fact that C:\test.txt returns {"name": "C:\test.txt"} whereas C:/test.txt returns {"name": "test.txt"} seems like an api which doesn't recognise Windows paths but that should hopefully be fixed by simply normalising the backslashes to begin with.

@jasongrout
Copy link
Contributor

normalising \ to /

Yes, behind a setting that you manually set, right?

the path starts with the server root so you can't "break out" - e.g. pseudo-python-code:

The security check already (should) happen on the server side.

Reopening for the issue of "A filebrowser setting to normalize 'open from path' paths from \ to /"

@jasongrout jasongrout reopened this May 23, 2019
@dhirschfeld
Copy link
Member Author

Yes, behind a setting that you manually set, right?

Is a backslash a valid path character on linux? If so, then yeah I guess you'd have to opt into it. Alternatively you could match on ^[a-zA-Z]:\.*$ before applying the normalisation

@jasongrout
Copy link
Contributor

backslash is a valid filename character on linux.

@jasongrout
Copy link
Contributor

So I think https://github.com/jupyterlab/jupyterlab/pull/5626/files can basically be resurrected, except instead of conditioning on the browser's operating system, we should condition on a setting.

@jasongrout jasongrout removed their assignment May 23, 2019
@jasongrout jasongrout modified the milestones: 1.0, 1.1 Jun 5, 2019
@jasongrout
Copy link
Contributor

Moving to 1.1. If anyone can work on this in the next week, feel free and we can get it in 1.0.

@jasongrout
Copy link
Contributor

Closing in deference to #6932, an idea we came up with in discussion on #5626.

@jasongrout jasongrout modified the milestones: 1.1, Reference Aug 2, 2019
@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 Sep 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug pkg:filebrowser 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 a pull request may close this issue.

6 participants