Render in an NSMenuItem instead of an NSPopover #11
Conversation
|
Imo a menu item is more appropriate for a status bar view, and the handling of login options and quitting is more intuitive this way. Plus it simplifies the code quite a bit. LGTM |
Vitals/AppDelegate.swift
Outdated
| var statusBar: StatusBarController? | ||
| let processMonitor = ProcessMonitor() | ||
|
|
||
| func applicationDidFinishLaunching(_ aNotification: Notification) { | ||
| processMonitor.start() | ||
|
|
||
| // Create the SwiftUI view that provides the contents | ||
| let viewModel = ContentViewModel(monitor: processMonitor, contentVisible: false) | ||
| let viewModel = ContentViewModel(monitor: processMonitor, contentVisible: true) |
There was a problem hiding this comment.
The contentVisible property can never be false anymore. Thoughts on removing it?
There was a problem hiding this comment.
Thanks for pointing this out!
I actually chose to fix it up rather than remove it (11b12dd). Without it, the SwiftUI ContentView gets re-rendered every second, even when the menu bar is closed, and that eats a bit of unnecessary CPU. If you know a better way of handling this, I'm all ears though 😄
Vitals/ContentView.swift
Outdated
| @@ -114,7 +114,7 @@ struct ContentView: View { | |||
| .frame(width: 45, alignment: .trailing) | |||
| } | |||
| } | |||
| }.frame(maxWidth: .infinity, maxHeight: .infinity).padding(10) | |||
| }.frame(maxWidth: .infinity, maxHeight: .infinity).padding(EdgeInsets(top: 4, leading: 22, bottom: 4, trailing: 12)) | |||
There was a problem hiding this comment.
I noticed the process names aren't aligned with the other menu items on Big Sur (whereas they are on Catalina). I've tried to account for this, but I still don't have access to Big Sur, so the padding value is a guess that's likely to be a bit off. If you get a second, would you mind checking and seeing what looks right?
There was a problem hiding this comment.
12 px inset
14 px inset
In my very scientific testing using a Preview.app selection box's edge to compare vertical alignment, I've found 14 px works a bit better than the current 12 px.
There was a problem hiding this comment.
Top science! 🧑🔬
I've updated it to 14px and released in v0.5.
Co-authored-by: Juan Lledo <juan.lledo@hotmail.com>
|
@jlledo thanks very much for your review, and apologies for totally missing it 😬. I've addressed most of your feedback - there's just one outstanding question about the left inset size on Big Sur, then I think this is good to go. |
|
@jlledo in fact, I'm going to merge this as-is, then we can tweak the left inset size as a follow-up PR if necessary. |
| @@ -77,6 +77,13 @@ struct ContentView: View { | |||
| private static let processListHeight = 460 | |||
| private static let topPadding = 6 | |||
| private static let bottomPadding = 4 | |||
| private static var leftPadding: Int { | |||
| if #available(macOS 10.0, *) { | |||
There was a problem hiding this comment.
This should actually be macOS 11.0, * to check for Big Sur – I just decremented it to check the conditional was working as expected and forgot to update. Fixed in db4068f.
Replace the popover with a menu item that has a custom view.
Pros:
Cons:
Thoughts on whether the new look is a "pro" or a "con" welcome!