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

FileBrowserView adjustments #1666

Closed
wants to merge 5 commits into from
Closed

FileBrowserView adjustments #1666

wants to merge 5 commits into from

Conversation

magland
Copy link
Contributor

@magland magland commented Jul 21, 2023

Here I propose a few modifications to the FileBrowserView component.

Add some tooltips for the actions

"View asset", "Download asset", "View asset metadata", "Open in external service"

Do not show the view/eye action if the path is a non-viewable type, i.e., the browser doesn't support it

Currently, when the user clicks to view an asset, and the browser does not support it inline (e.g., .nwb), then the behavior is equivalent to the download action (it downloads the file). Therefore I propose to hide the view action button for these cases. Rather than providing a list of supported types, I decided to take a more conservative approach and provide a list of non-viewable types, the most important being '.nwb'.

View the item on click

I think the user would expect some action on clicking the asset name (I personally keep expecting that even though I've used the system for many months). So, in the case where the item is viewable, I propose to initiate the view action on clicking the asset name. In order to make this work properly, I needed to add @click.stop attributes to the button actions so that clicking those does not propagate up to the parent item.

Here's a screenshot showing how some assets are viewable and others are not:

image

if (asset) { return; }
location.value = path;
function itemIsViewable(item: AssetPath) {
const nonViewableExtensions = ['nwb', 'zip', 'gz', 'nii', 'tif', 'tiff', 'avi'];
Copy link
Member

Choose a reason for hiding this comment

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

I was surprised to discover that .tiff is not supported by most of the browsers. It is supported by Safari though. But I also wonder if/how it would interact with some browser extensions which might be there to support viewing those types like https://www.blackice.com/broadcast/10132020TIFFV/index.html ?

@magland
Copy link
Contributor Author

magland commented Aug 4, 2023

@yarikoptic I didn't know tif was supported in Safari. I have removed .tif and .tiff from the non-viewable in my PR.

@yarikoptic
Copy link
Member

ping @waxlamp

@waxlamp
Copy link
Member

waxlamp commented Nov 3, 2023

Here I propose a few modifications to the FileBrowserView component.

Thank you for the pull request, @magland! I reviewed this with @AlmightyYakob and our collective feedback is below.

Add some tooltips for the actions

"View asset", "Download asset", "View asset metadata", "Open in external service"

Great!

Do not show the view/eye action if the path is a non-viewable type, i.e., the browser doesn't support it

Currently, when the user clicks to view an asset, and the browser does not support it inline (e.g., .nwb), then the behavior is equivalent to the download action (it downloads the file). Therefore I propose to hide the view action button for these cases. Rather than providing a list of supported types, I decided to take a more conservative approach and provide a list of non-viewable types, the most important being '.nwb'.

Two thoughts:

  1. To implement this feature, we are simply using the browser's ability to respond to a Content-disposition header with a value of inline appropriately. "Appropriately" here means (as per web standards) display the file in-browser if the capability exists; otherwise offer to download the file. There is major strength in this simplicity. Among other things, anyone with a browser
    or browser extension installed that can do things like view a TIFF file etc. can simply do so without our having to specify anything about TIFF files in our code.
  2. I understand that the uncertainty in whether the browser will display the file or offer to download it is an ergonomic wart (albeit, IMO, a small one). If we wish to intervene in the matter, I believe we need to do so on the basis of the asset's mimetype (what DANDI calls its "encoding format") and not the filename extension. Furthermore, I don't want to get into the game of maintaining whitelists or blacklists of filetypes--that way madness lies. So, if we want to be more explicit about enabling "viewable" types, I think we need to do some research into whether we can accomplish it in a more standards-compliant manner by examining mimetypes at the time we generate the view icon.

Essentially, my recommendation is to remove this portion of the PR in favor of doing nothing and letting the browser handle it (my preference) or examining this more organically and doing the right thing with mimetypes etc.

Thoughts?

View the item on click

I think the user would expect some action on clicking the asset name (I personally keep expecting that even though I've used the system for many months). So, in the case where the item is viewable, I propose to initiate the view action on clicking the asset name. In order to make this work properly, I needed to add @click.stop attributes to the button actions so that clicking those does not propagate up to the parent item.

Here's a screenshot showing how some assets are viewable and others are not:

image

Some history: at some point, I asked our frontend devs to remove the material design "floop" that occurs on click precisely for the reason you stated--the violation of expectation of something happening was bad UX that I thought we could fix easily. It turns out that the way the logic is structured, this wasn't so easy so we left it as is. We actually had plans to do something like open an info panel containing rendered metadata or something like that, but it was never in demand enough to actually do it. Furthermore, I tend to agree with you that clicking on the item should initiate "opening" it, whatever that means.

So, our recommendation is to use a click on the item itself as the "view" action, and also to get rid of the eyeball icon altogether. The eyeball icon has always struck me as kind of gross (even though I'm the one who introduced it) and I think rather than always displaying it, or conditionally displaying it (as in your example) the best thing is to get rid of it altogether and let the natural UX of clicking directly on the item to be the path to "opening" it for view. This also leaves open the opportunity to still do a nicely rendered metadata display for the "info" icon (in place of opening a separate window showing the API docs page for the metadata, but that's really a separate issue).

@magland
Copy link
Contributor Author

magland commented Nov 3, 2023

Thanks for the review! @waxlamp

I understand your points, and how you don't want to maintain lists of file extensions with different behaviors. However, DANDI is built around certain types if I am understanding the project, including NWB. We know that browsers will never natively support this format. If a user clicks on the view button of a NWB file (or clicks the file in the case where you remove the eyeball), I don't think they would expect it to start downloading - especially since there is a separate download button. And I don't think that's a recommended action anyway, since the NWB files can be tens or even hundreds of GB in size. Having to cancel a download can be annoying when all the user wanted to do was get a preview of the file. As an aside, my preference would be to have the view button (or the file click) open in Neurosift in a separate tab. I have met several people who didn't know that Neurosift was an option because it's hidden in the menu dropdown, but are now grateful to be able to preview the contents of NWB files. One person in particular told me he had been downloading terrabytes of data just so he could inspect the metadata of those files.

@yarikoptic
Copy link
Member

Essentially, my recommendation is to remove this portion of the PR in favor of doing nothing and letting the browser handle it (my preference) or examining this more organically and doing the right thing with mimetypes etc.

Thoughts?

In "principle" I am with @waxlamp on this -- it is nice to give a client (browser) a chance to handle correctly some fancy file type via e.g. some extension to support that file type and show in the browser. And I hate when that is not done e.g. for .pdf or .json files which could be handled either by Acroread of even natively by the browser (for .json), and it is not infeasible to see some .nii.gz or even .nwb viewer extension crafted by someone.

But "pragmatically", given current state (AFAIK - no such browser built-in capabilities, or extensions in popular use), I would prefer cleaner interface/actions and thus a combination of

  • what @magland proposes - to do list "custom default viewers" for some of those file types, such as .nwb and .nwb where we know that ATM browsers would trigger a download
  • no "view" icon for file types which we simply KNOW would trigger a download (the question though how to decide?). Then if someone comes back to us asking why his fancy extension doesn't work - we could enable for those file types... or I am even ok to stay with default - "View" icon with download action as a compromise between two approaches ;)

IMHO that would provide the best UX to be able to view whenever it is possible.

@waxlamp waxlamp self-assigned this Dec 4, 2023
@jjnesbitt jjnesbitt removed their request for review December 5, 2023 21:46
@waxlamp
Copy link
Member

waxlamp commented Jan 31, 2024

@magland thank you again for preparing this PR, and sorry for the long delay in processing it (working on this!).

I'm closing this PR in favor of these PRs:

which cover most of the uncontroversial ideas in this PR. I've also opened #1848 to track a good but slightly controversial idea that we didn't resolve here.

A quick note on process for the future: placing small but technically unrelated ideas into separate PRs (even if the ideas share a theme) will make it much easier to review and accept uncontroversial contributions while leaving space to argue about the controversial ones.

@waxlamp waxlamp closed this Jan 31, 2024
@waxlamp waxlamp deleted the filebrowser-adjustments-2 branch January 31, 2024 19:07
@magland
Copy link
Contributor Author

magland commented Jan 31, 2024

Thanks @waxlamp that makes sense. Looking forward to seeing the updates!

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

Successfully merging this pull request may close these issues.

3 participants