Skip to content
This repository has been archived by the owner on Jan 14, 2022. It is now read-only.

[wip] adding nwb viewer #849

Closed
wants to merge 9 commits into from
Closed

Conversation

djarecka
Copy link
Member

@djarecka djarecka commented Aug 20, 2021

@waxlamp , @mvandenburgh - please take a look if this is a good place and way to add this extra button.

This is still not finished, since I'm having problem with providing a correct url to the bioimagesuiteweb viewer.
@waxlamp - I remember it worked for you, but I'm having some issues now. Trying different URLs from the dandi info website, but each time the viewer gives me an error.
Could you please recreate a proper url for this file as an example.

closes #821

@djarecka djarecka marked this pull request as draft August 20, 2021 03:11
@yarikoptic
Copy link
Member

This is still not finished, since I'm having problem with providing a correct url to the bioimagesuiteweb viewer.

No URL from dandi would work for it just yet, bioimagesuiteweb/bisweb#127 but I hope the fix is soon to come and IMHO it should be just the asset download url (to API, which mints signed url with content disposition) which web ui already provide for download

@djarecka
Copy link
Member Author

@yarikoptic - thanks for clarifying. I thought, that @waxlamp was able to see the image last week, but maybe it was something special about that image

@yarikoptic
Copy link
Member

re bisweb - fix was implemented, and available in "alpha" deployment to try out on e.g. https://bioimagesuiteweb.github.io/alphaapp/viewer.html?image=https://api.dandiarchive.org/api/assets/5ab0247b-659a-4593-b251-b338676de725/download/

@djarecka
Copy link
Member Author

@djarecka
Copy link
Member Author

djarecka commented Aug 20, 2021

@yarikoptic - wait, I think this might just work for nii.gz. I thought that this was for nwb...

edit
nevermind, I've got lost in all the issues, looks like http://nwbexplorer.opensourcebrain.org/ is the one I should be using

@djarecka
Copy link
Member Author

I will need some help with testing this functionality

}
if (file_ext === 'nwb') {
return `http://nwbexplorer.opensourcebrain.org/nwbfile=${publishRest.assetDownloadURI(this.identifier, this.version, asset_id)}`;
}
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if code was separated from registry of services, so that registry could be reused later by multiple portals.
See https://github.com/datalad/datalad-deprecated/blob/master/datalad_deprecated/resources/website/assets/js/main.js#L21 and how used

Copy link
Member

Choose a reason for hiding this comment

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

Note: we need to add maxsize field there and set it to eg 1GB or might be even less for bisweb. Sure to limitations extracted data must not exceed 4GB and anyways, download might be too slow etc. So I wouldn't bother exposing that service for known to fail out be problematic files

if (file_ext === 'nwb') {
return `http://nwbexplorer.opensourcebrain.org/nwbfile=${publishRest.assetDownloadURI(this.identifier, this.version, asset_id)}`;
}

Copy link
Member

Choose a reason for hiding this comment

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

after bioimagesuiteweb/bisweb#129 is addressed, we should also add .tiff into this "registry" (also with similar limits on the file size). ATM bisweb falls into "it is a nifti" if there is no file extension (as our endpoints and blob files have none), but IMHO should work whenever content-disposition is respected (where we do provide the extension)

Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

Looks great, I think the button placement and icon selection is perfect.

Just for future reference, it's better to create a branch and work off of that for PRs then to fork the repo. Doing so enables Netlify deploy previews, which makes testing out changes easier.

src/views/FileBrowserView/PublishFileBrowser.vue Outdated Show resolved Hide resolved
@djarecka
Copy link
Member Author

djarecka commented Aug 23, 2021

@mvandenburgh , @yarikoptic - thanks for your suggestions. Trying to figure out the code from datalad and wondering if we want to have a list of multiple services and multiple buttons to open the file? Or we want to provide just a default viewer for some extensions?

edit
question answered - instead of the icon, I should use a context menu to list all the possible websites

@mvandenburgh
Copy link
Member

@mvandenburgh , @yarikoptic - thanks for your suggestions. Trying to figure out the code from datalad and wondering if we want to have a list of multiple services and multiple buttons to open the file? Or we want to provide just a default viewer for some extensions?

edit
question answered - instead of the icon, I should use a context menu to list all the possible websites

btw @djarecka not sure if you are already aware, but the vuetify docs are really good and have a bunch of different examples with the corresponding Vue/Vuetify code for you to copy/paste and modify to your liking. Looks like this example is similar to what @yarikoptic was showing today https://vuetifyjs.com/en/components/menus/#use-in-components

@djarecka
Copy link
Member Author

djarecka commented Aug 23, 2021

Thank you @mvandenburgh ! that's very helpful!

…ion that gets all possible services; creating a contex menu for the services instead of button
@djarecka
Copy link
Member Author

@mvandenburgh - thanks for review, I will add one more thing that @dchiquito suggested and I will create a branch from this repo, to enable CI

…e file is bigger than the maxsize, the service will not be added to the list and shown to the user
@djarecka djarecka mentioned this pull request Aug 24, 2021
@djarecka
Copy link
Member Author

open #854 instead (created a new branch on upstream)

@djarecka djarecka closed this Aug 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a context menu with options for "opening" an asset (or dandiset, folder?) on external services
4 participants