Skip to content

Windows systray#1285

Merged
MDrakos merged 20 commits intomasterfrom
systray-177209445
Mar 22, 2021
Merged

Windows systray#1285
MDrakos merged 20 commits intomasterfrom
systray-177209445

Conversation

@MDrakos
Copy link
Copy Markdown
Member

@MDrakos MDrakos commented Mar 15, 2021

@MDrakos MDrakos requested a review from Naatan March 15, 2021 20:24
Comment thread cmd/state-tray/internal/open/open_win.go
Comment thread cmd/state-tray/main.go Outdated
Comment thread cmd/state-tray/main.go Outdated
Comment thread cmd/state-tray/main.go
@MDrakos MDrakos requested a review from Naatan March 15, 2021 23:00
Comment thread cmd/state-tray/main.go Outdated
err := open.Prompt("state --version")
if err != nil {
handleError(errs.Wrap(err, "Could not open command prompt"))
return errs.Wrap(err, "Could not open command prompt")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking at this again now, this feels like it should just be logging the error. Since we don't want to end things just cause the "About" click caused an error.

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.

Makes sense. What do you think about setting logging to verbose by default here so that any calls to logging.Error also print? That way we don't have to do the additional call to fmt.Fprintln

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sure, I think that makes sense.

Comment thread cmd/state-tray/main.go
}

func onReady() {
err := run()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So is the ONLY error that can happen at the moment the "About" click? The systray stuff just doesn't throw errors?

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.

Correct, nothing in the systray library returns an error. I'm not sure if this run function is necessary anymore if we are going to be logging the errors when they occur. Do you still want it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's leave run where it is, that at least establishes the contract we want to keep long term.

@MDrakos MDrakos requested a review from Naatan March 18, 2021 22:42
Naatan
Naatan previously approved these changes Mar 19, 2021
@MDrakos MDrakos merged commit bf7e7f6 into master Mar 22, 2021
@MDrakos MDrakos deleted the systray-177209445 branch March 22, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants