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

Added FileSelector widget #909

Merged
merged 26 commits into from
Jan 20, 2020
Merged

Added FileSelector widget #909

merged 26 commits into from
Jan 20, 2020

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Dec 27, 2019

Adds a FileSelector widget that looks like this and allows browsing files on the server. It only allows browsing files in the specified directory so you cannot access restricted files. The UI isn't perfect since actions like a double click on a directory are not allowed so selecting a directory will update the path in the top bar and unlock the "Enter" button. Clicking that button will actually navigate to that directory. Selections are performed just like the CrossSelector widget. Also has additional buttons to go "home" (i.e. the initially specified directory), back and forth through the history and lastly to go up a directory.

Screen Shot 2019-12-27 at 4 07 19 PM

@jbednar @jlstevens Would be great if you could try this out and report back on UI changes you think would help.

@jbednar
Copy link
Member

jbednar commented Dec 28, 2019

Can you add an example to the docs to make it easier to try out?

@philippjfr
Copy link
Member Author

Done.

@codecov
Copy link

codecov bot commented Dec 28, 2019

Codecov Report

Merging #909 into master will increase coverage by 0.28%.
The diff coverage is 93.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #909      +/-   ##
==========================================
+ Coverage   85.33%   85.61%   +0.28%     
==========================================
  Files          96       98       +2     
  Lines       10772    10996     +224     
==========================================
+ Hits         9192     9414     +222     
- Misses       1580     1582       +2
Impacted Files Coverage Δ
panel/viewable.py 81.3% <ø> (ø) ⬆️
panel/tests/widgets/test_file_selector.py 100% <100%> (ø)
panel/widgets/__init__.py 100% <100%> (ø) ⬆️
panel/widgets/base.py 83.11% <66.66%> (-1.4%) ⬇️
panel/widgets/file_selector.py 89.2% <89.2%> (ø)
panel/widgets/select.py 94.16% <95%> (+4.5%) ⬆️
panel/widgets/button.py 86.95% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cae7f46...e761704. Read the comment docs.

@jbednar
Copy link
Member

jbednar commented Dec 29, 2019

This looks really useful! I was part of the design of this CrossSelector-based interface for multiselection years ago, so I'm not blaming anyone but myself, but I definitely don't find it intuitive. Basically, the problem is that it's a selector, yet it's hard to figure out what's selected and how to use it. Specifically:

  1. When clicking on a file it highlights immediately and is highly visible. For most selection widgets that would indicate that it's been selected, but here it just means that it can be selected; nothing that happens on the left listing affects selection directly. Even if I double click on it it's still not selected, but there's no feedback about that. It sounds like double-clicking can't be detected, which is a pain, because it would be much easier to discover how this widget works if double clicking would send the item over to the right listing (where it is actually selected). (At that point a user would go "Ohhh...! Selected ones go on the right!"). I know the filter boxes do mention selection, but only obliquely, so it's still very confusing at first.

  2. Even if double-clicking can't be supported, I don't understand why a separate Enter button is needed. Can't Forward be used for that? That button already goes deeper into a subdirectory in some cases; seems like it can be the button for going deeper in all cases, using the currently highlighted directory if there is one, and otherwise moving forward to a previously entered directory.

  3. When would the "up" button be useful? Presumably we can't get to any directory above the starting directory, so why aren't the left and right arrows already sufficient? It seems to me that one just has a starting directory then goes deeper into it or goes back, which is captured by the left and right arrows. But maybe I'm missing something.

  4. I'm not sure the "home" button is needed, given that one already can't go "back" beyond the starting directory, so it seems like one can just hit "left" until it stops going backward. Here unlike 2 and 3 above I do realize that Home does offer some actual different functionality, i.e. a single-click way to get to the starting point, but I'm not sure it's worth having something on screen to do it, given how easy it is to triple-click (or whatever) the back button. So I'd vote for removing the Home and Enter buttons, leaving only left (Back) and right (Forward), where Forward enters the directory if highlighted. People are likely to explore and understand a couple of widgets, but once there are lots of them, they are likely never to learn what some of them do.

  5. Filtering appears to work on the entire pathname, which I found unintuitive. E.g. for the default of using the home directory as the base, entering my name as the filter string selected all files, presumably because all files share the prefix /Users/jbednar. I think filtering should use only the information actually displayed (so that filtering on "bednar" would only select files that actually have "bednar" in their displayed names).

  6. I don't think the file listing should include "./" in each file's name; that's probably confusing to Windows users and doesn't seem to provide any information, because there doesn't seem to be any support for selecting files at multiple directory levels simultaneously. If one could select within a subdirectory then navigate to another directory and select more files, the path should be shown in the selected files listing, but that doesn't seem to be supported, so I don't see why to show any path, and certainly not the redundant "./" path.

  7. As for the usual Multiselect, I wish that this multi-pane interface would only appear (as a popup?) when I'm actually selecting something, the way the Calendar widget usually works. It takes up so much screen real estate as it is.

@philippjfr
Copy link
Member Author

philippjfr commented Dec 29, 2019

That button already goes deeper into a subdirectory in some cases; seems like it can be the button for going deeper in all cases, using the currently highlighted directory if there is one, and otherwise moving forward to a previously entered directory.

Seems very weird to me, back and forward traverse the stack of previously visited directories (just like your web browser or OS file browser), overloading that with some other semantics seems very odd. I also don't really see how that's any clearer tbh.

When would the "up" button be useful? Presumably we can't get to any directory above the starting directory, so why aren't the left and right arrows already sufficient?

I think you've misunderstood back and forward, and that confusion shines through in a lot of these points, so I won't address the rest of your comments until you've had a chance to reevaluate with that understanding.

Don't think the file listing should include "./" in each file's name

Sure, I agree.

If one could select within a subdirectory then navigate to another directory and select more files, the path should be shown in the selected files listing

I'd kind of like that to be supported, not straightforward though (*actually probably not too difficult).

I wish that this multi-pane interface would only appear (as a popup?)

Popups are definitely not trivial, I'm also not quite sure what you're imagining. I mean I can definitely just hide this behind a button, which expands it when needed.

@philippjfr
Copy link
Member Author

Filtering appears to work on the entire pathname, which I found unintuitive

Certainly not the intended behavior!

@philippjfr
Copy link
Member Author

philippjfr commented Dec 29, 2019

To summarize, I can agree that Home is somewhat redundant but I strongly disagree with combining back/forward with enter. I think most people are familiar with back/forward from their file/web browser and overloading it definitely seems strange to me. Enter is an annoyance but I don't see a better approach.

Up can be useful because the back/forward stack can be very much out of order (which is I think the thing you were missing), the address bar lets you enter any path so back has no guarantee of bringing you one directory up. This is also why overloading forward with enter is so problematic.

@philippjfr
Copy link
Member Author

philippjfr commented Dec 29, 2019

When clicking on a file it highlights immediately and is highly visible. For most selection widgets that would indicate that it's been selected, but here it just means that it can be selected; nothing that happens on the left listing affects selection directly.

True, the cross-selector interface is not the most convenient. Would love to hear alternatives! The only approach I see is to add a placeholder which explains things to the box on the right.

@philippjfr
Copy link
Member Author

Suggestions for fixing the placeholder text welcome:

Screen Shot 2019-12-29 at 6 18 59 PM

@jbednar
Copy link
Member

jbednar commented Dec 29, 2019

the address bar lets you enter any path so back has no guarantee of bringing you one directory up

Ok, I see what you are getting at, but I don't think it changes anything. I'm focusing on the experience of people using the buttons, which is what I think normal people will do. It's fundamentally a Selector, not a free-text field; people will typically use it to Select using a mouse or touchpad from options they are given. Using the buttons in this way, I think it's always going to be a simple left-right stack along components of a single path. I.e. if I start at /Users/jbednar and use the Enter button three times to select bokeh, then examples, then models, I get to /Users/jbednar/bokeh/examples/models. Then Back goes to examples, then bokeh, and from there Forward goes to examples, then models. Using the buttons in this way I can just go left or right along one path at any one time, i.e. /Users/jbednar/bokeh/examples/models; I can't go sideways onto some other path.

If you do type in arbitrary paths, then yes, that does let you get all out of order. But I don't think that's going to be a typical action people will take with this widget, and if they do, it seems just as likely that they would want to traverse the subcomponents of the path they pasted in as to traverse their history of pasting. It's not like a URL bar where people will keep it open and paste in new URLs all the time; that would be a different widget. This one is a selector, for selecting from displayed options, right?

With that and the fact that every extra button makes a UI less discoverable and less obvious, I don't think it's worth having Home, because multiple clicks can always get back to it. And I still don't think it's worth having a separate and deeply confusing Return button. People coming up to this widget are going to immediately get frustrated by not being able to enter the highlighted subdirectory, and I think if they are presented with only two widgets Back and Forward, they are likely to intuit "maybe I should click Forward? (because it's surely not Back!)". With three widgets, one of which has a confusing "Return" symbol, I don't think people will intuitively click Return ("I'm not clicking on that one; I don't want to return anywhere!").

So though I agree it's a bit confusing to overload Forward, I think it's much more confusing to have a separate Return button, because I think people will figure out two buttons much more easily than three.

@jbednar
Copy link
Member

jbednar commented Dec 29, 2019

BTW, part of the problem is that the symbol for Return is pointing backwards, and fundamentally entering a directory is to move forwards, not backwards. But you can't add another button pointing forwards, as Forward already does that. I don't think there's any graphical way to keep those two actions straight, so I propose that there is only one button for both types of "moving forward". I.e. if they can't be made clearly distinct; give up and just make one button for it!

@philippjfr
Copy link
Member Author

philippjfr commented Dec 29, 2019

Okay, I get your point but I think making them look like back and forward keys is more confusing in that case because people are familiar with those from other file browsers. Up and down seems more appropriate in that case and getting rid of the whole notion of a stack/history.

@jbednar
Copy link
Member

jbednar commented Dec 29, 2019

Sure, that makes sense; up and down are fine for this purpose.

@philippjfr
Copy link
Member Author

philippjfr commented Dec 29, 2019

Okay so to list all the suggestions:

  • Add a placeholder with an explanation on how to select (text to be determined)
  • Get rid of all buttons except for up/down button which either go up a directory or descend into the selected directory
  • Fix the issue with the filtering working on the full path
  • Attempt to allow selecting files from different directories
  • Possibly add an option to collapse the widget by default.

@jbednar
Copy link
Member

jbednar commented Dec 29, 2019

As for the lack of double click, I don't know why double-click won't work, so I'm not sure what to suggest. Double-click is the obvious action for entering a subdirectory or for "send this file to the right pane immediately" in this interface, and anything else is going to be awkward.

Suggestions for fixing the placeholder text welcome:

I'm pretty sure I suggested it in the first place, and even a couple of years later I don't think I have any better suggestions. It totally makes sense once you understand it, and makes no sense until you do.

@philippjfr
Copy link
Member Author

We didn't actually have a TextAreaInput until quite recently, which is the only way we can add the placeholder text.

@jbednar
Copy link
Member

jbednar commented Dec 29, 2019

We didn't actually have a TextAreaInput until quite recently, which is the only way we can add the placeholder text.

I'm not sure what you mean; doesn't the placeholder text originate with Crosselector?
image

@philippjfr
Copy link
Member Author

philippjfr commented Dec 29, 2019

Yes, but I mean putting a longer placeholder in the actual select box on the right like shown here:

Screen Shot 2019-12-29 at 6 35 35 PM

That's only possible by replacing the MultiSelect widget inside the cross-selector with a TextInputArea if no selection has been made.

@philippjfr
Copy link
Member Author

entering my name as the filter string selected all files

Can't actually reproduce this.

@jbednar
Copy link
Member

jbednar commented Dec 29, 2019

Oh, I see what you mean now. Yes, adding text in the right-hand box would be a big, big help in making it clear what to do. In any case your to-do list sounds perfect.

entering my name as the filter string selected all files

Can't actually reproduce this.

Oops; I was confused. I thought it was selecting on the path because it selected all the files when I put in *bednar*. But it turns out it also selects all the files when I put in *philippjfr*, so that's not what's going on, and clearly I don't understand what's going on with filtering.

@philippjfr
Copy link
Member Author

I thought it was selecting on the path because it selected all the files when I put in "bednar". But it turns out it also selects all the files when I put in "philippjfr", so that's not what's going on, and clearly I don't understand what's going on with filtering.

Can't reproduce this either, seems to behave as expected for me.

@philippjfr
Copy link
Member Author

I still kind of think back/forward can be useful but for now I've definitely removed home. I've also implemented the ability to select across directories, removed the ./, added the placeholder. Let me know if you want an option to collapse the widget in some form and what it would look like when collapsed.

The current state:

Screen Shot 2019-12-29 at 8 06 09 PM

@jbednar
Copy link
Member

jbednar commented Dec 29, 2019

To reproduce:

  1. Start with FileSelector.ipynb
  2. Run all
  3. Type *lsadkfjladsjfadkfjadsklfjsd* or some such into the "Filter available options" box.
  4. Press the return key on the keyboard
  5. All options get selected, not just those matching that string.

It looks like a single leading * is sufficient to make it match all available options, no matter what text follows it.

@philippjfr
Copy link
Member Author

philippjfr commented Dec 29, 2019

It looks like a single leading * is sufficient to make it match all available options, no matter what text follows it.

Oh right, the filter uses a regex.match not a glob-like pattern. So *lsadkfjladsjfadkfjadsklfjsd will match 0 or more occurences of 'lsadkfjladsjfadkfjadsklfjsd'.

@jbednar
Copy link
Member

jbednar commented Dec 29, 2019

the filter uses a regex.match not a glob-like pattern

Normally filename matching is done with globs, not regexes, so that will be surprising. In any case I don't see anything about regexes in the code, only globs, and even with a regex * normally works on the preceding literal, not the following one. I.e. 8* matches any number of 8s, right? So I remain confused.

@philippjfr
Copy link
Member Author

In any case I don't see anything about regexes in the code

Have a look at the CrossSelector code:

match = re.compile(query)
matches = list(filter(match.search, options))

@philippjfr
Copy link
Member Author

Final design:

Screen Shot 2020-01-20 at 4 30 24 PM

@philippjfr philippjfr merged commit 28e6fff into master Jan 20, 2020
@jbednar
Copy link
Member

jbednar commented Jan 20, 2020

Looks great!

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

Successfully merging this pull request may close these issues.

None yet

4 participants