Add F key shortcut to toggle player fullscreen - #446
Closed
tavyandy97 wants to merge 2 commits into
Closed
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
@KhooLy When you get a chance, could you take a look? If anything's missing or you'd like changes to the approach, let me know and I'll update it. Thanks for your time! |
Member
|
Hi thanks for your PR but #594 is going to make much more besides this feature so If his PR gets merged this is stale, if his doesnt then I can merge this. Thanks for your time. |
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
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
Adds
Fas a shortcut to toggle fullscreen in the desktop player. Previously fullscreen was only reachable from the keyboard viaF11or, on macOS,Ctrl+Cmd+F.PR type
Why
Plain
Fis the near-universal fullscreen binding across video players (YouTube, VLC, Plex, mpv). The player already handledF11andCtrl+Cmd+F, so users reaching forFgot no response.Desktop scope
Desktop only, all platforms (Windows, macOS, Linux). The change is confined to the desktop player's WebView controls UI at
composeApp/src/desktopMain/resources/player-ui/controls.js. No shared/common code changed, and no Kotlin changed — it reuses the existingtogglePlayerFullscreen()helper, which sends the sametoggleFullscreencommand the on-screen fullscreen button already sends.Issue or approval
#445 — filed, but not yet approved. Opening this for maintainer consideration rather than claiming approval it doesn't have.
Flagging honestly: per
CONTRIBUTING.mdthis is a behavior change (keyboard input / fullscreen) and needs an approved feature request first, so no PR type box below is truthful today and I've left them unticked rather than check a false one. The template check will fail on that. If #445 is approved, "Approved larger or directional change" applies and I'll update. If the change isn't wanted, happy to close this.UI / behavior impact
This PR does change behavior (adds a keyboard binding), so none of the behavior boxes above can be ticked truthfully until #445 is approved.
Policy check
CONTRIBUTING.md.Scope boundaries
Intentionally not changed:
F11andCtrl+Cmd+Fhandling is untouched.DesktopAppFullscreen.ktis untouched, so bareFdoes nothing outside the player.actionShortcutCommandForEventtable (S/T/C/E/P) is left as-is rather than bundled in.Fis routed through the existingshortcutCommandForEventtable rather than theF11branch, so it inherits the guards a letter key needs: modifier keys are rejected (Cmd+F,Ctrl+F,Alt+Ffall through untouched), and dispatch happens after theactiveModal || isTextEntryTargetcheck so typing "f" in a search field or with a modal open does not toggle fullscreen.Testing
node --checkoncontrols.jspasses.shortcutCommandForEvent, modal/text-entry guard ahead of dispatch, andnoteChromeActivity()confirmed to no-op while chrome is hidden.Not yet verified by running the app. Outstanding manual checks before this should be merged:
Ftoggles into and out of fullscreen during playbackCmd+F/Ctrl+Fdo not toggleF11andCtrl+Cmd+Fstill workScreenshots / Video
Not a UI change.
Breaking changes
None.
Linked issues
#456