(Med.)[BFF-403] Add "Go to location" option to FileAccessContextMenu #426
Conversation
tyler-foster
requested review from
hughes036,
aswallace,
SeanLeRoy,
lynwilhelm,
BrianWhitneyAI,
kmitcham and
pgarrison
| @@ -1,12 +1,22 @@ | |||
| import { ContextualMenuItemType, IContextualMenuItem } from "@fluentui/react"; | |||
| import * as React from "react"; | |||
| import { useDispatch, useSelector } from "react-redux"; | |||
| import { shell } from "electron"; | |||
There was a problem hiding this comment.
nit: bff imports are generally alphabetical (tho not enforced by linter), so would go in line 2
There was a problem hiding this comment.
alphabetical by where it is being imported from? first item imported? I'm not clear. To be fair, it isn't my PR, so if Tyler gets it, good.
There was a problem hiding this comment.
Where it is imported from. Symbols go first, so @ fluent still goes before electron
| @@ -1,12 +1,22 @@ | |||
| import { ContextualMenuItemType, IContextualMenuItem } from "@fluentui/react"; | |||
| import * as React from "react"; | |||
| import { useDispatch, useSelector } from "react-redux"; | |||
| import { shell } from "electron"; | |||
There was a problem hiding this comment.
electron is not available on web so this would break our web deploy. Platform specific dependencies have to go into the platform specific package so electron stuff would go into desktop
| key: "go-to-file-location", | ||
| text: "Go to file location", | ||
| title: "Show selected file in your native file browser", | ||
| disabled: |
There was a problem hiding this comment.
For things like this where they can only apply to one item we just use the focused item (of which there has to be one by default) so it would make sense to me to remove the restriction on fileSelection.count() > 1. This should be disabled or removed from the list of options when rendering on a selected folder though.
disabled: isNil(fileDetails?.localPath) should cover you
There was a problem hiding this comment.
That's waaaay easier. Done in 866d43b
| */ | ||
| const openNativeFileBrowser = (fileDetails: FileDetail) => { | ||
| if (fileDetails.localPath) { | ||
| shell.showItemInFolder(fileDetails.localPath); |
There was a problem hiding this comment.
This will likely need to be dispatched as an action to the redux state where a side-effect would occur that would call the FileViewerService which would do this functionality in a platform specific way. For web it would just throw a not implemented error.
|
Why are there 2 "Open" options in this image? |
…on" feature doesn't break web build
tyler-foster
changed the title
| let existsSyncStub: SinonStub; | ||
|
|
||
| before(() => { | ||
| existsSyncStub = stub(fs, "existsSync"); | ||
| }); | ||
|
|
||
| after(() => { | ||
| existsSyncStub.restore(); | ||
| }); | ||
|
|
||
| it("throws an error if file path does not exist", () => { | ||
| // Arrange | ||
| existsSyncStub.returns(false); | ||
|
|
||
| const service = new FileViewerServiceElectron(new UselessNotificationService()); | ||
|
|
||
| const fileDetail = new FileDetail( | ||
| { | ||
| annotations: [ | ||
| { | ||
| name: "Cache Eviction Date", | ||
| values: ["2000-01-01"], | ||
| }, | ||
| ], | ||
| file_path: "/allen/aics/path/to/file.txt", | ||
| }, | ||
| Environment.TEST | ||
| ); | ||
|
|
||
| // Act + Assert | ||
| expect(() => { | ||
| service.openNativeFileBrowser(fileDetail); | ||
| }).to.throw(FileNotFoundError); |
There was a problem hiding this comment.
@SeanLeRoy @aswallace Help! This unit test doesn't actually work the way it looks like it should. It's currently failing on the GitHub build but passing on my local machine (for the wrong reasons).
The issue with it is that the stub() I'm using doesn't really stub what it looks like it should. You can flip the .returns() value all you want and it doesn't cause the test to fail or pass.
Does anyone have better experience with / advice for using Sinon and mocking out module functions?
There was a problem hiding this comment.
It might be the way that sinon is running against the other tests - so I'd try the sinon changes I suggested in other comments.
it might also be that the way fs is imported in this and in the actual usage is different so perhaps try unifying them
There was a problem hiding this comment.
Looked at the branch locally, agree that it's an issue with the way fs is imported.
I was able to get the test to pass/fail by changing the fs import in this file to import fs from "fs";
| @@ -4,4 +4,8 @@ export default class FileViewerServiceWeb implements FileViewerService { | |||
| public async open(): Promise<void> { | |||
| throw new Error("FileViewerServiceWeb::open is not yet implemented"); | |||
| } | |||
|
|
|||
| public openNativeFileBrowser(): void { | |||
| throw Error("ExecutionEnvServiceWeb::openNativeFileBrowser not yet implemented"); | |||
There was a problem hiding this comment.
| throw Error("ExecutionEnvServiceWeb::openNativeFileBrowser not yet implemented"); | |
| throw Error("FileViewerServiceWeb::openNativeFileBrowser not yet implemented"); |
| let existsSyncStub: SinonStub; | ||
|
|
||
| before(() => { | ||
| existsSyncStub = stub(fs, "existsSync"); |
There was a problem hiding this comment.
You can use the sandbox established before
| existsSyncStub = stub(fs, "existsSync"); | |
| existsSyncStub = sandbox.stub(fs, "existsSync"); |
There was a problem hiding this comment.
Also could remove the after two lines below this
closes #403
Description
Adds an item to the file list's context menu, "Go to file location", which will appear if the user is running the app in desktop mode and will be enabled if their selected file has a value for
localPath. Clicking on it will open the file's Vast path in the user's native file browser.We can consider this as a little escape hatch for the "Open with..." feature, which gets a bit more difficult to use when working with next generation file formats like OME-Zarr. Users can use it to jump to a file and use their operating system's "open with" feature, or drag and drop it somewhere.
Is there a setup for testing hooks like this in BFF? I only did manual testing, and I wasn't actually able to test opening something from the Vast, because I don't have it mounted on my Macbook and I can't seem to get BFF to launch correctly on my Ubuntu setup.
I'm still open to suggestions / alternative ideas for the wording!
Skipping the fuss around adding similar functionality to the file details pane, for now.