Skip to content
This repository was archived by the owner on Jul 22, 2025. It is now read-only.

Fix show UI when opening Station twice. Closes #266#274

Merged
juliangruber merged 5 commits intomainfrom
fix/show-ui-when-opening-station-twice
Nov 22, 2022
Merged

Fix show UI when opening Station twice. Closes #266#274
juliangruber merged 5 commits intomainfrom
fix/show-ui-when-opening-station-twice

Conversation

@juliangruber
Copy link
Copy Markdown
Member

In #266, @walkerlj0 thought Station didn't open when she opened it, although it was already open (and hidden in the tray).

This change makes it so the app show its UI when you open it a second time, to try to recover from that situation.

Closes #266.

@juliangruber
Copy link
Copy Markdown
Member Author

Converted to a draft because CI is failing

@juliangruber juliangruber marked this pull request as ready for review November 19, 2022 13:19
Copy link
Copy Markdown
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Great UX improvement 👏🏻

Let's make the implementation easier to reason about and more robust in edge cases 👇🏻

Comment thread main/index.js Outdated
app.on('second-instance', () => {
ctx.showUI()
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! I did not know about this event.

Here is the link to API docs: https://www.electronjs.org/docs/latest/api/app#event-second-instance

I think it would be better to have app.on('second-instance') and app.requestSingleInstanceLock() closer together. Can you perhaps move these new four lines to the top of the file, insert it at line 74?

https://github.com/filecoin-station/filecoin-station/blob/4e2125e0213e921c18969ec6a3fd27077a198f98/main/index.js#L70-L73

I think we should also change the condition !app.requestSingleInstanceLock() && !inTest to skip requesting single instance lock when running in the test environment. Otherwise we request the lock just to ignore it, while still triggering second-instance event in the other running instance.

if (!inTest && !app.requestSingleInstanceLock()) { 

Thoughts?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These are really great suggestions! I implemented both :)

@juliangruber juliangruber requested a review from bajtos November 21, 2022 21:33
Copy link
Copy Markdown
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Much better 👍🏻

Comment thread main/index.js
app.quit()
}

app.on('second-instance', () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nitpick: feel free to ignore.

I would add a comment to explain the UX flow we are implementing here. E.g.

// When the user attempts to start the app from the menu because 
// they don't notice the Station icon in the tray, then help 
// them finding the Station by showing our main window.
app.on('second-instance', () => {

After I wrote this comment, I realised it would be even nicer if the main window could also show a dialog explaining to the user that the Station has been running in the background and that they can always find it in the Tray (macOS menu bar). That's probably out of the scope of this PR, though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

great suggestions, added docs and opened #282

@juliangruber juliangruber merged commit 4d26548 into main Nov 22, 2022
@juliangruber juliangruber deleted the fix/show-ui-when-opening-station-twice branch November 22, 2022 19:40
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.

Cannot open GUI after restarting Computer

2 participants