Implement a command line interface to show custom OSD#49
Draft
mkozjak wants to merge 4 commits intoalberti42:mainfrom
Draft
Implement a command line interface to show custom OSD#49mkozjak wants to merge 4 commits intoalberti42:mainfrom
mkozjak wants to merge 4 commits intoalberti42:mainfrom
Conversation
Signed-off-by: Mario Kozjak <kozjakm1@gmail.com>
Reviewer's GuideAdds a CLI app that can invoke the Tahoe volume HUD with configurable volume, title, and on-screen position, extends the HUD API and layout to support optional status-bar anchoring, positional placement, and optional icons, and wires existing callers to the new interface. Sequence diagram for CLI invocation of TahoeVolumeHUDsequenceDiagram
actor User
participant Shell
participant CLIProcess as volume_control_osd
participant NSApp as NSApplication
participant Delegate as CLIAppDelegate
participant HUD as TahoeVolumeHUD
User->>Shell: Run command with args
Shell->>CLIProcess: Launch process
CLIProcess->>CLIProcess: Parse --volume, --title, --position
CLIProcess->>NSApp: [sharedApplication]
CLIProcess->>NSApp: setActivationPolicy(Accessory)
CLIProcess->>NSApp: set delegate = CLIAppDelegate
CLIProcess->>NSApp: run()
NSApp-->>Delegate: applicationDidFinishLaunching
Delegate->>NSApp: activateIgnoringOtherApps(TRUE)
Delegate->>Delegate: Create CLIPlayerApplication
Delegate->>HUD: sharedManager()
Delegate->>HUD: showHUDWithVolume(volume, usingMusicPlayer: player, andLabel: title, anchoredToStatusButton: nil, position: position)
Note over HUD: HUD computes layout and position
Delegate->>Delegate: Schedule NSTimer(kTotalLifetime)
Delegate-->>NSApp: Timer fires
Delegate->>NSApp: terminate(nil)
NSApp-->>Shell: Process exits
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The CLI’s fixed
kTotalLifetimeduplicates internal HUD timing constants; consider exposing a completion callback or deriving this duration fromTahoeVolumeHUDso the CLI doesn’t silently drift out of sync if the animation timings change. - In
positionPanel:position:thepositionparameter is ignored when a status bar button is provided; if callers might expectHUDPositionto have an effect in both cases, either document that it’s ignored when anchored or adjust the implementation to reconcile the two behaviours.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The CLI’s fixed `kTotalLifetime` duplicates internal HUD timing constants; consider exposing a completion callback or deriving this duration from `TahoeVolumeHUD` so the CLI doesn’t silently drift out of sync if the animation timings change.
- In `positionPanel:position:` the `position` parameter is ignored when a status bar button is provided; if callers might expect `HUDPosition` to have an effect in both cases, either document that it’s ignored when anchored or adjust the implementation to reconcile the two behaviours.
## Individual Comments
### Comment 1
<location path="Sources/CLI/main_cli.m" line_range="41-42" />
<code_context>
+// fade-in + hold + fade-out cycle, then quits cleanly.
+// ---------------------------------------------------------------------------
+
+// Total lifetime = fade-in (0.25 s) + hold (1.5 s) + fade-out (0.45 s) + margin
+static const NSTimeInterval kTotalLifetime = 0.25 + 1.5 + 0.45 + 0.15;
+
+@interface CLIAppDelegate : NSObject <NSApplicationDelegate>
</code_context>
<issue_to_address>
**issue:** Avoid hard-coding HUD animation timings in the CLI to prevent drift if TahoeVolumeHUD timings change.
kTotalLifetime re-specifies the fade-in/hold/fade-out durations that already exist in TahoeVolumeHUD. If those HUD values change, the CLI timing will be wrong. Prefer either exposing a HUD API that returns its total lifecycle duration or computing this value from shared constants instead of duplicating the numbers here.
</issue_to_address>
### Comment 2
<location path="Sources/CLI/main_cli.m" line_range="58" />
<code_context>
+ // see through the window to the content behind it and renders opaque/solid.
+ [NSApp activateIgnoringOtherApps:YES];
+
+ // Show the HUD. Passing nil for the status button → centers on screen.
+ CLIPlayerApplication *player = [[CLIPlayerApplication alloc] init];
+ player.currentVolume = self.volume;
</code_context>
<issue_to_address>
**nitpick:** The comment about HUD placement with a nil status button no longer matches the actual default behavior.
The behavior described here is outdated: with the default `position` of `HUDPositionTopCenter`, passing `nil` to `positionPanel:position:` shows the HUD near the top of the screen, not centered. Please update this to describe that `nil` uses the configured `HUDPosition` (currently `HUDPositionTopCenter`, placing the HUD below the menu bar) rather than always centering it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+41
to
+42
| // Total lifetime = fade-in (0.25 s) + hold (1.5 s) + fade-out (0.45 s) + margin | ||
| static const NSTimeInterval kTotalLifetime = 0.25 + 1.5 + 0.45 + 0.15; |
There was a problem hiding this comment.
issue: Avoid hard-coding HUD animation timings in the CLI to prevent drift if TahoeVolumeHUD timings change.
kTotalLifetime re-specifies the fade-in/hold/fade-out durations that already exist in TahoeVolumeHUD. If those HUD values change, the CLI timing will be wrong. Prefer either exposing a HUD API that returns its total lifecycle duration or computing this value from shared constants instead of duplicating the numbers here.
| // see through the window to the content behind it and renders opaque/solid. | ||
| [NSApp activateIgnoringOtherApps:YES]; | ||
|
|
||
| // Show the HUD. Passing nil for the status button → centers on screen. |
There was a problem hiding this comment.
nitpick: The comment about HUD placement with a nil status button no longer matches the actual default behavior.
The behavior described here is outdated: with the default position of HUDPositionTopCenter, passing nil to positionPanel:position: shows the HUD near the top of the screen, not centered. Please update this to describe that nil uses the configured HUDPosition (currently HUDPositionTopCenter, placing the HUD below the menu bar) rather than always centering it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a configurable on-screen volume HUD position and a command-line entry point to trigger it.
New Features:
Enhancements:
This is proof-of-concept idea and is vibe-coded.
Build:
Run:
References #48.