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.

Sign up for GitHub

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

Jump to bottom

Allow 'Open Workspace' to open workspace whenever possible, even if logfile does not exist #1618

Merged
merged 8 commits into from
Sep 5, 2024
Merged

Allow 'Open Workspace' to open workspace whenever possible, even if logfile does not exist #1618

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e9f1e6c
Allows 'Open Workspace' to open workspace whenever possible, even if …
emilyanndavis Aug 21, 2024
c182f29
Adds/updates tests
emilyanndavis Aug 22, 2024
c505209
Adds "error opening workspace" modal & updates tests
emilyanndavis Aug 23, 2024
d824e97
Removes unneeded log messages & updates HISTORY
emilyanndavis Aug 23, 2024
3276d21
Simplifies 'open workspace' behavior, omitting selection of logfile
emilyanndavis Aug 29, 2024
0f7dbcf
Merge branch 'main' of github.com:natcap/invest into bugfix/1598-impr…
emilyanndavis Sep 5, 2024
ee148ad
Fixing rst formatting
emilyanndavis Sep 5, 2024
0846c72
Removes openWorkspaceDir helper function
emilyanndavis Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ Unreleased Changes
------------------
* Workbench
* Several small updates to the model input form UI to improve usability and visual consistency (https://github.com/natcap/invest/issues/912)
* Fixed a bug that caused the application to crash when attempting to open a workspace without a valid logfile (https://github.com/natcap/invest/issues/1598)

3.14.2 (2024-05-29)
-------------------
Expand Down
1 change: 1 addition & 0 deletions workbench/src/main/ipcMainChannels.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const ipcMainChannels = {
IS_FIRST_RUN: 'is-first-run',
LOGGER: 'logger',
OPEN_EXTERNAL_URL: 'open-external-url',
OPEN_PATH: 'open-path',
OPEN_LOCAL_HTML: 'open-local-html',
SET_SETTING: 'set-setting',
SHOW_ITEM_IN_FOLDER: 'show-item-in-folder',
Expand Down
6 changes: 6 additions & 0 deletions workbench/src/main/setupDialogs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,10 @@ export default function setupDialogs() {
shell.showItemInFolder(filepath);
}
);

ipcMain.handle(
ipcMainChannels.OPEN_PATH, async (event, dirpath) => {
return await shell.openPath(dirpath);
}
);
}
205 changes: 123 additions & 82 deletions workbench/src/renderer/components/InvestTab/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import TabContainer from 'react-bootstrap/TabContainer';
import Nav from 'react-bootstrap/Nav';
import Row from 'react-bootstrap/Row';
import Col from 'react-bootstrap/Col';
import Modal from 'react-bootstrap/Modal';
import Button from 'react-bootstrap/Button';
import {
MdKeyboardArrowRight,
} from 'react-icons/md';
Expand Down Expand Up @@ -45,10 +47,6 @@ async function investGetSpec(modelName) {
return undefined;
}

function handleOpenWorkspace(logfile) {
ipcRenderer.send(ipcMainChannels.SHOW_ITEM_IN_FOLDER, logfile);
}

/**
* Render an invest model setup form, log display, etc.
* Manage launching of an invest model in a child process.
Expand All @@ -64,13 +62,17 @@ class InvestTab extends React.Component {
uiSpec: null,
userTerminated: false,
executeClicked: false,
showErrorModal: false,
};

this.investExecute = this.investExecute.bind(this);
this.switchTabs = this.switchTabs.bind(this);
this.terminateInvestProcess = this.terminateInvestProcess.bind(this);
this.investLogfileCallback = this.investLogfileCallback.bind(this);
this.investExitCallback = this.investExitCallback.bind(this);
this.handleOpenWorkspace = this.handleOpenWorkspace.bind(this);
this.openWorkspaceDir = this.openWorkspaceDir.bind(this);
this.showErrorModal = this.showErrorModal.bind(this);
}

async componentDidMount() {
Expand Down Expand Up @@ -154,6 +156,7 @@ class InvestTab extends React.Component {
updateJobProperties(tabID, {
argsValues: args,
status: undefined, // in case of re-run, clear an old status
logfile: undefined, // in case of re-run where logfile may no longer exist, clear old logfile path
});

ipcRenderer.send(
Expand Down Expand Up @@ -186,13 +189,40 @@ class InvestTab extends React.Component {
);
}

handleOpenWorkspace(logfile, workspace_dir) {
if (logfile) {
ipcRenderer.send(ipcMainChannels.SHOW_ITEM_IN_FOLDER, logfile);
}
if (workspace_dir) {
// Always call shell.openPath.
// If shell.showItemInFolder failed, it will have failed silently;
// if it succeeded, the subsequent call to shell.openPath will not "undo" the result of shell.showItemInFolder.
this.openWorkspaceDir(workspace_dir);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a benefit to defining both handleOpenWorkspace and openWorkspaceDir? As opposed to doing all the logic in one function? It's fairly inconsequential so if you prefer it this way feel free to leave it as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not. I think this is a holdover from when handleOpenWorkspace also called shell.showItemInFolder when the logfile was valid. With that complexity gone, it does look a little silly now.


async openWorkspaceDir(workspace_dir) {
const error = await ipcRenderer.invoke(ipcMainChannels.OPEN_PATH, workspace_dir);
if (error) {
logger.error(`Error opening workspace (${workspace_dir}). ${error}`);
this.showErrorModal(true);
}
}

showErrorModal(shouldShow) {
this.setState({
showErrorModal: shouldShow,
});
}

render() {
const {
activeTab,
modelSpec,
argsSpec,
uiSpec,
executeClicked,
showErrorModal,
} = this.state;
const {
status,
Expand All @@ -213,88 +243,99 @@ class InvestTab extends React.Component {
const sidebarFooterElementId = `sidebar-footer-${tabID}`;

return (
<TabContainer activeKey={activeTab} id="invest-tab">
<Row className="flex-nowrap">
<Col
className="invest-sidebar-col"
>
<Nav
className="flex-column"
id="vertical tabs"
variant="pills"
activeKey={activeTab}
onSelect={this.switchTabs}
<>
<TabContainer activeKey={activeTab} id="invest-tab">
<Row className="flex-nowrap">
<Col
className="invest-sidebar-col"
>
<Nav.Link eventKey="setup">
{t('Setup')}
<MdKeyboardArrowRight />
</Nav.Link>
<Nav.Link eventKey="log" disabled={logDisabled}>
{t('Log')}
<MdKeyboardArrowRight />
</Nav.Link>
</Nav>
<div
className="sidebar-row sidebar-buttons"
id={sidebarSetupElementId}
/>
<div className="sidebar-row sidebar-links">
<ResourcesLinks
moduleName={modelRunName}
docs={modelSpec.userguide}
/>
</div>
<div
className="sidebar-row sidebar-footer"
id={sidebarFooterElementId}
>
{
status
? (
<ModelStatusAlert
status={status}
handleOpenWorkspace={() => handleOpenWorkspace(logfile)}
terminateInvestProcess={this.terminateInvestProcess}
/>
)
: null
}
</div>
</Col>
<Col className="invest-main-col">
<TabContent>
<TabPane
eventKey="setup"
aria-label="model setup tab"
<Nav
className="flex-column"
id="vertical tabs"
variant="pills"
activeKey={activeTab}
onSelect={this.switchTabs}
>
<SetupTab
pyModuleName={modelSpec.pyname}
userguide={modelSpec.userguide}
modelName={modelRunName}
argsSpec={argsSpec}
uiSpec={uiSpec}
argsInitValues={argsValues}
investExecute={this.investExecute}
sidebarSetupElementId={sidebarSetupElementId}
sidebarFooterElementId={sidebarFooterElementId}
executeClicked={executeClicked}
switchTabs={this.switchTabs}
<Nav.Link eventKey="setup">
{t('Setup')}
<MdKeyboardArrowRight />
</Nav.Link>
<Nav.Link eventKey="log" disabled={logDisabled}>
{t('Log')}
<MdKeyboardArrowRight />
</Nav.Link>
</Nav>
<div
className="sidebar-row sidebar-buttons"
id={sidebarSetupElementId}
/>
<div className="sidebar-row sidebar-links">
<ResourcesLinks
moduleName={modelRunName}
docs={modelSpec.userguide}
/>
</TabPane>
<TabPane
eventKey="log"
aria-label="model log tab"
</div>
<div
className="sidebar-row sidebar-footer"
id={sidebarFooterElementId}
>
<LogTab
logfile={logfile}
executeClicked={executeClicked}
tabID={tabID}
/>
</TabPane>
</TabContent>
</Col>
</Row>
</TabContainer>
{
status
? (
<ModelStatusAlert
status={status}
handleOpenWorkspace={() => this.handleOpenWorkspace(logfile, argsValues?.workspace_dir)}
terminateInvestProcess={this.terminateInvestProcess}
/>
)
: null
}
</div>
</Col>
<Col className="invest-main-col">
<TabContent>
<TabPane
eventKey="setup"
aria-label="model setup tab"
>
<SetupTab
pyModuleName={modelSpec.pyname}
userguide={modelSpec.userguide}
modelName={modelRunName}
argsSpec={argsSpec}
uiSpec={uiSpec}
argsInitValues={argsValues}
investExecute={this.investExecute}
sidebarSetupElementId={sidebarSetupElementId}
sidebarFooterElementId={sidebarFooterElementId}
executeClicked={executeClicked}
switchTabs={this.switchTabs}
/>
</TabPane>
<TabPane
eventKey="log"
aria-label="model log tab"
>
<LogTab
logfile={logfile}
executeClicked={executeClicked}
tabID={tabID}
/>
</TabPane>
</TabContent>
</Col>
</Row>
</TabContainer>
<Modal show={showErrorModal} onHide={() => this.showErrorModal(false)} aria-labelledby="error-modal-title">
<Modal.Header closeButton>
<Modal.Title id="error-modal-title">{t('Error opening workspace')}</Modal.Title>
</Modal.Header>
<Modal.Body>{t('Failed to open workspace directory. Make sure the directory exists and that you have write access to it.')}</Modal.Body>
<Modal.Footer>
<Button variant="primary" onClick={() => this.showErrorModal(false)}>{t('OK')}</Button>
</Modal.Footer>
</Modal>
</>
);
}
}
Expand Down
1 change: 1 addition & 0 deletions workbench/tests/main/main.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ describe('createWindow', () => {
ipcMainChannels.GET_N_CPUS,
ipcMainChannels.INVEST_VERSION,
ipcMainChannels.IS_FIRST_RUN,
ipcMainChannels.OPEN_PATH,
ipcMainChannels.SHOW_OPEN_DIALOG,
ipcMainChannels.SHOW_SAVE_DIALOG,
];
Expand Down
Loading
Loading