Hide 'Open Workspace' button when logfile does not exist #1617
Conversation
There was a problem hiding this comment.
Thanks, @emilyanndavis . This looks good to me. It did make me think of a case where logfile is defined, but the file does not exist. This could happen if a user deletes the logfile from the workspace after the model run, and then goes back to the list of "Recent Runs", and opens that model tab. In this case the "Open Workspace" button would display, but be non-functional (I tried it out).
This is a very rare case, so I don't really think we need to handle it right now, but curious what you think.
Also, the PR that I merged earlier is causing a conflict in HISTORY in this PR. That's the only thing that really needs changing here. Thanks!
| @@ -208,7 +208,7 @@ class InvestTab extends React.Component { | |||
| return (<div />); | |||
| } | |||
|
|
|||
| const logDisabled = !logfile; | |||
| const logDoesNotExist = !logfile; | |||
There was a problem hiding this comment.
It did make me think of a case where logfile is defined, but the file does not exist. This could happen if a user deletes the logfile from the workspace after the model run, and then goes back to the list of "Recent Runs", and opens that model tab.
Interesting point. I agree, it seems like that would be a very rare scenario. Still, it makes me wonder if we could refactor the open workspace method so that it doesn't require a logfile at all. You'd know better than I why it was written to be dependent on the logfile, but if there's a way to pass in just the workspace path instead, this could also be a more elegant solution to the issue resolved in this PR. Instead of hiding the Open Workspace button, leaving the user with a total mystery, we could keep the button active, and when it opened the workspace, it would become immediately clear to the user that the workspace is empty (which would still leave them with a mystery, but at least there's a clue as well).
There was a problem hiding this comment.
I completely agree, de-coupling this button from the logfile makes a lot of sense. And opening up to an empty workspace sounds just fine. There's still probably a case where the workspace folder never exists and we need some error-handling.
I can't say for sure why it is the way it is. I guess the logfile is the most reliable artifact left behind by running an invest model, so it's possible that we were just avoiding some redundancy by not storing both the logfile path and the workspace path in an InvestJob instance, for example.
Also, the available electron.shell APIs must have been a factor: https://www.electronjs.org/docs/latest/api/shell
Maybe shell.openPath would work instead of showItemInFolder if given a path to a directory.
There was a problem hiding this comment.
I tested shell.openPath, and it does open a folder in Finder, if you pass in a path to a directory (whereas if you pass in a file path, it will actually open the file). I'm going to try going down that (ahem) path and put this PR on hold. If the other approach works, I'll abandon this PR; if I run into a snag, we can merge this PR as a stopgap.
…-open-workspace-btn-when-no-workspace-exists
|
Closing this in favor of PR 1618 |
Description
Fixes #1598 by hiding the 'Open Workspace' button when there is no logfile
Checklist