-
Notifications
You must be signed in to change notification settings - Fork 2
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
bugfix/ZEN-wrong-path #405
base: main
Are you sure you want to change the base?
Conversation
Is this a draft? |
Ok I tested this through Remote Desktop. Files that are cloud only grey out ZEN as an option and files that are in local cache open correctly in ZEN |
@@ -222,7 +222,7 @@ export default (fileDetails?: FileDetail, filters?: FileFilter[]): IContextualMe | |||
key: `open-with-${name}`, | |||
text: name, | |||
title: `Open files with ${name}`, | |||
disabled: !filters && !fileDetails, | |||
disabled: (!filters && !fileDetails) || !fileDetails?.localPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prevents all user defined apps not just Zen from attempting to open a local path - I'm unsure thats the best move since some applications should be able to read cloud files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the user experience better with mysterious opening failures or reduced options, which seem to be the choices? Or do we want to undertake tracking which apps are cloud ready, and showing the appropriate items for the situation?
@@ -1,4 +1,5 @@ | |||
import { isEmpty, sumBy, throttle, uniq, uniqueId } from "lodash"; | |||
import { isAbsolute } from "path"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't path a node
module aka not available for web deploys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah rats I always forget about this. (And didn't test it on web since I was using a remote workstation)
@@ -489,8 +490,14 @@ const openWithLogic = createLogic({ | |||
} else { | |||
filesToOpen = await fileSelection.fetchAllDetails(); | |||
} | |||
|
|||
// Determine if exePath is local | |||
const isLocalExecutable = isAbsolute(exePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it is likely we will always store the path as absolute for local executables but it does seem potentially buggy with the way certain systems mount apps - I'm pretty sure executionEnvService
is only used for local apps anyway right? Couldn't we just assume they are always local?
Description
The purpose of this PR is to resolve #401 and allow scientists to open local files ( VAST ) in ZEN. When we switched to cloud path being our primary path we then pass this path to external apps that try to "open it". This means that apps that need a local path like ZEN cannot open these paths.
Development Decisions
In order to complete this PR we had to make a few decisions.
In the future if we want specific apps to use a cloud path then we need to add them manually to the repo, otherwise it'll default to opening with the local path.