Skip to content

Conversation

@ReactorScram
Copy link
Contributor

@ReactorScram ReactorScram commented Aug 23, 2024

Also adds a "Download update" button in the bottom section of the tray menu when an update is ready.

image

@ReactorScram ReactorScram self-assigned this Aug 23, 2024
@vercel
Copy link

vercel bot commented Aug 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 28, 2024 2:14pm

Copy link
Member

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

I guess the advantage of runtime compositing is that our code is easier to write because "update available" can be coded as "take the current icon and add the dot to it" instead of matching on the current state and figuring out what the next state composed with the icon is?

That is the main thing I'd probably be focusing on when making a decision: Where do you want to set the icons and what is the most ergonomic way to write that code.

)?;
assert!(
start_instant.elapsed().as_millis() < 6,
"Should be able to compose all 6 icons in 1 ms each"
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't be surprised if the majority of time here is spent on FS access.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. If it slows down I can dig in and profile it, I just wanted to make sure all those per-pixel powf operations weren't hurting anything.

Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

Looks nice! I think macOS will also need similar treatment, but before that #6304 should be implemented probably.

@ReactorScram
Copy link
Contributor Author

I'm biased towards runtime compositing because the compositor was fun to write, but if everyone's fine, I'll go ahead with it.

The big advantage I see is that there are no files that can become out-of-date. With build-time compositing we have to write similar code but also tie it into the build system. With design-time compositing we have to duplicate a bunch of stuff in Figma and make humans do the computer's work. With runtime it costs a couple milliseconds of CPU work but it's always correct.

@thomaseizinger
Copy link
Member

With build-time compositing we have to write similar code but also tie it into the build system.

That would be easy to do with build.rs. cargo will re-run your build-script if any of the inputs change (as long as you emit the correct hint which files to watch). Then you can include_bytes! them directly into the binary as constants.

I'm biased towards runtime compositing because the compositor was fun to write, but if everyone's fine, I'll go ahead with it.

I am fine with whatever really. I'd also take into account the developer ergonomics when using the icons:

  • Do I want to dynamically compose them, i.e. remove / add layers without know which icon is currently displayed?
  • Is it fine to always compute the exact icon we want to display and select a single, composed constant?

@ReactorScram ReactorScram changed the title refactor(rust/gui-client): add a compositor for the tray icon feat(rust/gui-client): add "update ready" notification dot to the tray icon using a runtime compositor Aug 27, 2024
@ReactorScram ReactorScram marked this pull request as ready for review August 27, 2024 22:30
@ReactorScram
Copy link
Contributor Author

ReactorScram commented Aug 27, 2024

For now I'm always replacing the icon completely. There's only 6 possible composed results, and only a couple layers, so the GUI always knows all the inputs.

I integrated it into the code and removed all the debug scaffolding

Copy link
Member

Choose a reason for hiding this comment

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

Spaces in filenames? 😱

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figma seems to put out whatever you type as the frame name. I could name the object busy-layer I guess?

Comment on lines +167 to +169
IconBase::Busy => &[LOGO_GREY_BASE, BUSY_LAYER][..],
IconBase::SignedIn => &[LOGO_BASE][..],
IconBase::SignedOut => &[LOGO_GREY_BASE, SIGNED_OUT_LAYER][..],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IconBase::Busy => &[LOGO_GREY_BASE, BUSY_LAYER][..],
IconBase::SignedIn => &[LOGO_BASE][..],
IconBase::SignedOut => &[LOGO_GREY_BASE, SIGNED_OUT_LAYER][..],
IconBase::Busy => [LOGO_GREY_BASE, BUSY_LAYER],
IconBase::SignedIn => [LOGO_BASE, &[]],
IconBase::SignedOut => [LOGO_GREY_BASE, SIGNED_OUT_LAYER],

Out of curiosity, would this work too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I could make a special case in the compositor that a blank byte string is just skipped.

@jamilbk
Copy link
Member

jamilbk commented Aug 28, 2024

Maybe we won't need the option to ignore updates if we use a less intrusive update notification, like this notification dot for the daily check? Just thinking about how not to annoy users, will probably think through some more edge cases for #6304 et al

@ReactorScram
Copy link
Contributor Author

@jamilbk Option to ignore updates?

Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
Signed-off-by: Reactor Scram <ReactorScram@users.noreply.github.com>
@ReactorScram ReactorScram added this pull request to the merge queue Aug 28, 2024
Merged via the queue into main with commit 176ef05 Aug 28, 2024
@ReactorScram ReactorScram deleted the refactor/tray-icon-compositor branch August 28, 2024 15:23
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.

4 participants