feat: Add support for taking screenshots with hotkeys#508
Conversation
Reviewer's GuideThis PR implements end-to-end screenshot support triggered by hotkeys by extending the settings model and UI, wiring up a new takeScreenshot API in player wrappers, and handling file storage and naming logic in the desktop controls. Sequence diagram for taking a screenshot via hotkeysequenceDiagram
actor User
participant "DesktopControls"
participant "MediaControlsWrapper"
participant "BasePlayer"
participant "File System"
User->>"DesktopControls": Press screenshot hotkey
"DesktopControls"->>"MediaControlsWrapper": takeScreenshot(withSubtitles)
"MediaControlsWrapper"->>"BasePlayer": takeScreenshot(format, withSubtitles)
"BasePlayer"-->>"MediaControlsWrapper": Uint8List screenshotBuf
"MediaControlsWrapper"-->>"DesktopControls": screenshotBuf
"DesktopControls"->>"File System": Save screenshotBuf to file (with padded name)
"File System"-->>"DesktopControls": Screenshot saved
Class diagram for updated VideoPlayerSettingsModel and related typesclassDiagram
class VideoPlayerSettingsModel {
+String? screenshotsPath
+ScreenshotFormat screenshotFormat
+int screenshotNamePadding
+Map<VideoHotKeys, KeyCombination> hotKeys
+copyWith(...)
}
class ScreenshotFormat {
+png
+jpeg
+label(BuildContext context)
}
class VideoHotKeys {
+takeScreenshot
+takeScreenshotClean
}
VideoPlayerSettingsModel --> ScreenshotFormat
VideoPlayerSettingsModel --> VideoHotKeys
VideoPlayerSettingsModel --> KeyCombination
File-Level Changes
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/screens/settings/player_settings_page.dart:232` </location>
<code_context>
+ (current) => current.copyWith(screenshotNamePadding: value ?? 3),
+ );
+
+ ref.read(videoPlayerSettingsProvider.notifier).setscreenshotNamePadding(value ?? 3);
+ },
+ )),
</code_context>
<issue_to_address>
**issue:** Duplicate state update for screenshotNamePadding.
Consider removing one of these calls to prevent redundant state updates.
</issue_to_address>
### Comment 2
<location> `lib/screens/video_player/video_player_controls.dart:714` </location>
<code_context>
+ final paddingAmount = ref.read(videoPlayerSettingsProvider).screenshotNamePadding;
+ int maxNumber = 0;
+
+ await for (var file in savePathDirectory.list()) {
+ final finalSegment = file.uri.pathSegments.last;
+
+ if (file is File && finalSegment.endsWith(fileExtension)) {
+ final match = RegExp(r'(\d+)').firstMatch(finalSegment);
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** File extension check may not be robust for multi-dot filenames.
Using 'endsWith(fileExtension)' may incorrectly match files with multiple dots. Use 'p.extension(finalSegment) == ".${fileExtension}"' for accurate extension checking.
```suggestion
if (file is File && p.extension(finalSegment) == ".${fileExtension}") {
```
</issue_to_address>
### Comment 3
<location> `lib/wrappers/players/lib_mpv.dart:163` </location>
<code_context>
+ Future<Uint8List?> takeScreenshot(ScreenshotFormat format, bool withSubtitles) async {
+ String screenshotFormat = "";
+
+ // I'm intentionally not provinding a default case so we explicitely handle potential new formats.
+ switch (format) {
+ case ScreenshotFormat.jpeg:
</code_context>
<issue_to_address>
**nitpick (typo):** Typo in comment: 'provinding' should be 'providing'.
```suggestion
// I'm intentionally not providing a default case so we explicitely handle potential new formats.
```
</issue_to_address>
### Comment 4
<location> `lib/wrappers/media_control_wrapper.dart:374-376` </location>
<code_context>
}
}
+
+ Future<Uint8List?>? takeScreenshot(bool withSubtitles) {
+ return _player?.takeScreenshot(ref.read(videoPlayerSettingsProvider.select((value) => value.screenshotFormat)), withSubtitles);
+ }
</code_context>
<issue_to_address>
**suggestion:** Return type is nullable Future, which may be confusing.
Consider whether both the Future and its result need to be nullable, or if using 'Future<Uint8List?>' would be clearer.
```suggestion
Future<Uint8List?> takeScreenshot(bool withSubtitles) {
final player = _player;
if (player == null) {
return Future.value(null);
}
return player.takeScreenshot(
ref.read(videoPlayerSettingsProvider.select((value) => value.screenshotFormat)),
withSubtitles,
);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PartyDonut
left a comment
PartyDonut
left a comment
There was a problem hiding this comment.
Thanks for taking a shot at this. Functionality wise it seems to work just great!.
I've left a few comments let me know if any of them are unclear.
Some other notes/suggestions
Do we need the format selection? Would it not be better to always use PNG and let the user decide or convert it themselves later on. This would also clean up the user settings.
I would also suggest to use the existing download path directory and use "Screenshots" as sub directory.
One other thing taking a screenshot gives the user no feedback, we should add some sort of message or pop-up that notifies the user that screenshot "001.png" was saved.
| } | ||
| } | ||
|
|
||
| void takeScreenshot({required bool withSubtitles}) async { |
There was a problem hiding this comment.
Let's move this function outside of the video player controls into it's own provider.
There was a problem hiding this comment.
Would it be fine to put this on the video_player_provider?
| } | ||
|
|
||
| @override | ||
| Future<Uint8List?> takeScreenshot(ScreenshotFormat format, bool withSubtitles) async { |
There was a problem hiding this comment.
MDK seems to always take screenshots without subtitles?
Lets also change the track to disabled instead of loading/reloading the entire list. Currently the mdk player stutters quite heavily when taking a screenshot this might be part of the reason.
There was a problem hiding this comment.
MDK seems to always take screenshots without subtitles?
It seems to be inconsistent, befitting of a jank approach like this. In my tests, it usually takes the screenshot without subtitles, but it fails to enable them again. Sometimes it does enable them back though...
Lets also change the track to disabled instead of loading/reloading the entire list. Currently the mdk player stutters quite heavily when taking a screenshot this might be part of the reason.
Hints welcome on how to approach this one
|
Thanks for the review! I'll be chipping away at the comments later today, or tomorrow.
Personally I always use PNG. I only added the setting for user choice, and maybe allowing support for more formats in the future, but I'm definitely not attached to it.
Do you mean as a default, or just fixed to that folder? Personally I'd like to keep the option to select the directory where my screenshots go, mostly to keep them at hand and in the folder I had all the others.
Fair enough, I'll take a look and try to put something together |
|
Thank god, I didn't break the PR after the rebase lol. I pushed the first batch of changes and fixes. Still missing getting takeScreenshot a new home, plus the other things you mentioned that I replied to |
The only reason really is that this is a "nice to have" not so much a big requirement of a Jellyfin frontend. For defaults we could use what you have now. Maybe it's a feature a lot of people will use don't know but those defaults will probably cater to most people.
I would say fixed, so it always saves to "$downloadsPath/Screenshots"/, to prevent the application from having to handle multiple paths for saving different kinds of data. All that being said it is not that I don't like the options I am just scared if we keep adding advanced settings like this we will and up with huge models and huge settings screens. |
|
Sorry for disappearing! Lack of time/sleep/willpower took turns on controlling my brain this week. Nuked the settings, replaced for the defaults and using the Downloads path for screenshots in its own folder. Got over keeping it as soon as I remembered symlinks are a thing :) I noticed that the hotkey for taking screenshots without subtitles isn't being saved for some reason. It works fine after being changed, but it resets to the default after closing Fladder. Not sure what's going on there. I'm aware I'm still dancing around the other issues - I intend to tackle them in the next few days (or try at least). |
Seemingly fixed itself somehow, won't complain |
That is perfectly fine, no rush take your time 👍🏼. |
|
I moved takeScreenshot out of video_player_controls to video_player_provider, which may or may not be good. It feels a bit more appropriate at least. |
|
Forgot to add the new strings to the big file. An issue I noticed since I added the screenshots indicator is that the seek key shortcut doesn't work anymore. If I take out the screenshot indicator, it works, and if I move the seek indicator to be after the screenshot one on |
The input handler can be a bit strange when nesting multiple listeners with autoFocus enabled. My first suggestion would be play around with who does/does not have autoFocus. But this might also require more investigating on my end to improve stability. For disabling the subtitles in mdk I would suggest looking at the LibMDK class, specifically the setSubtitleTrack function. Easiest would be to use that function, save the current model, set the subtitle to (SubStreamModel.no()). And after the screenshot, set the original subtitle again. That should work. |
I tried setting autoFocus to true on just one of them, on both, and then both on false and the result is still the same. Hotkey for screenshots works, but the seek ones don't.
I see |
This is probably a tree ordering issue. The last one in the tree gets focus, however normally it should pass on any key that is not handled so that could be the problem. But looking at your code that seems to be handled just fine.
The players should not be aware of the selected subtitles or at least not as they are presented in the UI. My suggestion would be to set the subtitles just before you ask the player to take the screenshot and then implement it like in the video_player_options_sheet.dart -> showSubSelection() function. Keep in mind disabling subtitles while transcoding could cause the playback the re-evaluate whether transcoding is needed. Make sure you keep the subtitles state as is when transcoding. |
RainbowCookie32
force-pushed
the
develop
branch
2 times, most recently
from
81169a8 to
0e660e6
Compare
RainbowCookie32
force-pushed
the
develop
branch
from
0e660e6 to
a6bb577
Compare
makes clean screenshots consistent on mdk
|
I feel like it's a miracle I haven't borked this PR yet with the jank rebases. I think I got the clean screenshots working on both players now, did a quick test and they are looking fine. Subs noticeably blink for a second now on mpv, which didn't happen before and is an unfortunate nerf, but I think this is the cleanest way to get feature parity with both players. The seek hotkeys still refuse to play ball alongside with the screenshot ones tho... |
|
Just checking in @RainbowCookie32 are you still interested in finishing this? |
|
Oof, it's been a while. Seems like I have a few rebases pending. I think that the last batch of changes I pushed addressed everything you mentioned, with only the hotkeys conflict (seek and screenshots) being left, which I never really managed to figure out. Do let me know if there's anything else I should look into tho, I'll take a look at it |
|
Re-rebased, didn't realise I could request a new review so there it goes! |
There was a problem hiding this comment.
Fixed the layered input not working by adding a "hardwareKeyboard" fallback option. Not the cleanest option but good enough for now.
Everything else looks fine. Let's merge this for now and fix anything we find in new pull-requests so this one doesn't become too large.
Thanks for implementing this 😄
|
Nice! Thanks for the guidance on this too :) |
2 participants
Pull Request Description
My take on implementing my request on #497. This bring the possibility to take screenshots with or without subtitles using hotkeys. The screenshots are saved to a folder picked by the user, using an incrementing number for file name, which can be optionally zero-padded (by default, it's padded to 3 digits).
The feature works on both backends on desktop and (I think) should only show up on settings on that platform. The default keybind is G for screenshots with subtitles, and Ctrl+G for screenshots without subtitles. Gave it 0 thought though and I was trying to figure out inputs and how hotkeys worked when setting it. S was already taken too D:
Tested the feature today through a movie with the mpv backend and worked great. Like I mentioned on the discussion post, I hadn't done any coding in Dart/Flutter before so here be dragons kinda.
Issue Being Fixed
Fixes #497
Screenshots / Recordings
Checklist