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.

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

[macOS] feat: Add "quick-terminal" option to "macos-hidden" config #5803

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

McNight
Copy link
Contributor

@McNight McNight commented Feb 16, 2025

Fixes #5784

I started with a notification for listening to Ghostty terminal windows but then I saw the remark about update windows so I implemented applicationDidUpdate which is called whenever a window is added/removed.

Tell me if if you want me to remove the notification for Ghostty terminals!

@McNight McNight requested a review from a team as a code owner February 16, 2025 14:10
Copy link
Contributor

@mitchellh mitchellh left a comment

Choose a reason for hiding this comment

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

I haven't actually run it to test it yet but reviewing the code superficially I have some requests. I do think we should remove the notification machinery for now, thanks for that though. 😄

@McNight McNight force-pushed the mcnight/quick_terminal_macos_hidden branch from 929d8ea to 5b77807 Compare February 16, 2025 20:36
@McNight McNight requested a review from mitchellh February 16, 2025 20:37
@McNight McNight force-pushed the mcnight/quick_terminal_macos_hidden branch from 5b77807 to d7a7da6 Compare February 16, 2025 20:38
@@ -5762,6 +5762,7 @@ pub const MacTitlebarProxyIcon = enum {
pub const MacHidden = enum {
never,
always,
@"quick-terminal",
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh we need to add docs for this to the config key in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a comment for this new case, but maybe all the related paragraphs above need to be updated as well?

@McNight McNight requested a review from mitchellh February 16, 2025 21:00
@@ -403,6 +403,10 @@ class AppDelegate: NSObject,
reloadDockMenu()
}

func applicationDidUpdate(_ notification: Notification) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Okay I tested the logic of this and imo this callback gets called way too much. On initial launch it's called 4 times and when I close the window once it gets called something like a dozen or more times (I didn't count, but it was a lot).

I think we need to do something different. Maybe "any open window" isn't the right logic and we need to do some more research into how iTerm2 behaves instead.

Copy link
Contributor Author

@McNight McNight Feb 17, 2025

Choose a reason for hiding this comment

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

Yes that's what I noticed as well :/
I saw this method from a blog post about the challenge to track managed windows...
With AppKit Notifications, you get one when a window is about to close, and when one is becoming key/main but that's it.

Maybe we could intercept (via swizzling?) when windows are about to be set to the NSApp.windows property; or even add KVO to it (because it is not unfortunately)...

Do you think that applicationDidUpdate(_:) may cause performance issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think that applicationDidUpdate(_:) may cause performance issues?

I'm not sure if it'll practically cause issues but I'm afraid it'll cause unnecessary CPU/power usage. I'm not sure how grounded in reality that is but for a feature this minor it doesn't seem worth it.

@mitchellh mitchellh added the feedback-requested Discussion needs feedback before anything can be done. label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback-requested Discussion needs feedback before anything can be done. os/macos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS: Add "quick-terminal" option to "macos-hidden" config
3 participants