feat: merge media controls. #805
Conversation
CyanVoxel
added
Type: Enhancement
|
Hi @CyanVoxel. I think I'm getting closer, but still running into an issue with correctly fitting the overlay with the thumbnail. Is there an easy way to get the size of the rendered thumbnail image? Thanks! |
|
Thank you for your work on this so far! It should be fairly straightfoward to get the size of an image, but it differs a bit depending on what type of variable it is. PIL images have As of your last commit I don't think I see the issue you're describing, but it might be part of the screenshot I've attached below. Since you're getting closer to finishing I also went ahead and jotted down some quick feedback based on the current state of the PR:
|
|
Hi, Thanks for the comment! I'll be sure to work on those features/fixes. On my original issue, here is a screenshot of the issue I'm having: In this case, the thumbnail image stops growing, while the rest of the media player widget keeps growing. This only happens when I make the preview panel very large. Let me know if you have any questions. |
Ahh, I see! It looks like this is due to the max resolution used for the preview panel thumb which is 512x512 in most places. It's unfortunately a messy hardcoded limit sprinkled around the codebase. I won't stop you if you're interested in improving it, but I also wouldn't burden you with that for this feature. |
|
Hi, I just pushed a fix that should fix the sliders, autoplay, context menu, and the behavior when clicking on the media widget. I am running into a mypy issue that I'm not sure how to resolve yet. On the thumbnail sizing, I guess I could also lock it to 512x512 for now. I'll try to get that in the next commit. I have some questions on the your other comments:
Thank you! |
|
Thank you for implementing the feedback so far!
I see what you mean: From what I can tell, PySide6 is not implementing the
Sounds good to me!
Would this be in cases like this? If so, I think hiding the volume slider when it would normally overlap with the timecode would be an alright solution in this case
It should work the same way it currently does on I appreciate you implementing the feedback for jumping the seek position when clicking! However, I would expect it to move the handle to the clicked position as well, allowing you to continue holding down and dragging to seek. This is the behavior of any video/media player I can think of off the top of my head (e.g. YouTube, QuickTime, VLC, etc.) |
|
I just happened to notice the mypy errors here - So in the case of something like open_file_action = QAction(self)
Translations.translate_qobject(open_file_action, "file.open_file")this now becomes: open_file_action = QAction(Translations["file.open_file"])(I tried to comment on the code directly but GitHub didn't let me for some reason) |
|
Thanks for letting me know, I'll make sure to update that when pulling in the main branch. I don't think I'm able to reproduce the issue you had when clicking the slider. Clicking the slider causes the handle to move to the position I clicked. Does the most recent commit fix this issue for you? This might be a weird request, but would you be able to share a small video with me so I can test the widget overlap issues? If not, is there a resource I could find a sample video? P.S. I don't think locking the media player to 512x512 will work. It seems that the preview thumbnail grows past that size. Forgive me, as I'm not super familiar with it. I think we need a way of telling the media player when the thumbnail resizes. Currently, I'm not sure how to do that. Will research... |
|
Hi, i know this isnt open for review, i just thought i'd mention i tried dynamically resizing the volume bar. it fixes the overlap issue but its pretty hard to control at smaller sizes.
heres the patch if youre interested perhaps a better approach could be to make the volume slider vertical and popup when the media has a small width. however this does somewhat work also here is a link to find stock videos of that size, just right click and press "Save Video As" |
|
Hey @SkeleyM, Thanks for the patch! I tried it and it seemed to make it better. And I agree that it can get too small. Feel free to add more if you like :) I'd be interested in trying a vertical slider. I'll see if I can get something like that working. Also, thanks for the tip on the test videos! |
|
Hello, I refactored quite a bit in this last fix. In addition to the refactor, I added the ability to switch the orientation of the volume slider based on the size of the media player. Thank you @SkeleyM for the suggestion. How does it look? Also, I made the volume slider only visible when hovering over the mute button. I thought that was a neat feature :) There is still the issue on matching the size of the thumbnail for audio files. Need to continue researching there. |
|
I apologize for the inconvenience - a large refactor was recently merged (#844) that has moved several files throughout the codebase. This was done in order to correct some deep issues with the project's layout and was performed now as it seemed to be the best opportunity (as of 3/6/25) to disrupt as few open PRs as possible. Unfortunately this PR is one that's been disrupted by this change. A rebase targeting I also apologize for not getting back to you about the slider issue. While clicking it does move the handle to the pointer location, it does not let you continue dragging the handle from there. This makes it a bit awkward to control vs other media players. Please let me know if there's anything else I can do and if you'd like any help with this. You've done some fantastic work so far and the last thing I want is to scare you off with a rebase or too many requests! |
|
Hi @CyanVoxel, I just did a rebase and tested locally. I think I did it correctly... please let me know if I caused any issues. As for the slider issue, I see what you mean now. I'll work on that next! The only other thing I need help with is the sizing for thumbnail. If you have any good ways to fix it, please let me know. I will continue researching as well. Thanks! |
|
Something didn’t go quite as planned it seems, a merge might have been conducted in place of a rebase, so instead of your commits going on top of the new ones, things went the other way around. To amend this is a little of a complex fix that I wouldn’t expect someone to do, so I do not mind taking care of this at all. It will require me to force push into your branch, so if you have something you didn’t push it would affect that. Let me know if that’s fine and if you’re ready. |
|
Hi @xarvex, Sorry, I hope I didn't create a bunch of work for you all. I have nothing to push at the moment, so I think you should be good to go. |
Initial commit to merge audio/video files. There are still a few bugs around widget sizing that need fixing.
Add widgets to a sublayout to allow for centering in a QStackedLayout. Remove references to the legacy video player in the thumb preview.
|
That's okay! Rebases aren't an easy thing, so don't be harsh on yourself. To preserve the current state of this PR, I pushed to a new branch in our repo: https://github.com/TagStudioDev/TagStudio/tree/pull/805 I had to rebase from the point of the commit before the refactor (239a1b4) and manually redo the commit that came after (098840c). If all looks good there, I will force push to here. |
Subclass QSlider to handle click events and allow for easier seeking. Implement context menu along with autoplay setting for the media widget. Pause video when media player is clicked instead of opening file.
Start video/audio muted on initial load of the media player. Remove code causing mypy issue. Add new method for getting slider click state.
Add various layouts for positioning widgets instead of manually moving widgets. Change the volume slider orientation at smaller media sizes.
Fix position label color to white so it stays visible regardless of theme.
|
Hi @xarvex, I looked at the code and tested locally. Everything seems to be in place. Thank you so much for fixing this! Feel free to push it. |
xarvex
force-pushed
the
merge-media-controls
branch
from
098840c to
b62db08
Compare
There was a problem hiding this comment.
I know this is still a draft, but I wanted to get a jump on checking out the underlying code now that the player itself is coming further along.
Overall I think you've done a great job with this feature, and the media player is currently much more usable than the previous video and audio player setup. There's just some comments I have on the current code structure, a few bugs, and some miscellaneous suggestions.
So far I have some concerns about the new media player code being somewhat "grafted" onto the existing code for the legacy video player and previous audio player code rather than outright replacing it. When I previously refactored and split up the preview panel, I made sure to refer to the video player code as "legacy" to denote that it's going to be replaced by a future combined media player and I named the audio player as "media" to serve as a jumping off point. I understand that this wasn't really communicated on my part though, so I get if there was some hesitation on replacing this code. But all the legacy video player code, including the video_player.py file, will no longer be required thanks to this new combined player and can/should be removed. Really after this point should just be code for "images", "animated images/gifs", and "media (audio/video)".
In addition to this, I feel that the new QStackedLayout is not being fully taken advantage of and is currently just adding extra code with no current benefit. I would suggest looking into whether or not this layout can be used to replace the previous state management system that involved hiding widgets not in use with switch_preview() or to revert to the previous layout layout pattern. Currently there's one foot in the old system and one foot in this potential new system, when really it just needs to be one or the other.
Finally there's some bugs introduced here, with the current ones I've found being:
- The image and GIF previews no longer have right-click menus and cannot be opened via left click anymore
- The media player is missing the "Send to Trash" right-click option
- Videos do not loop, as was the previous default behavior
- The translation key for "Open file" is incorrect (see suggested change)
I hope this isn't too much, and please let me know if you have any questions or would like some help with this. You've done some great work so far and I would love to see this feature make it to the end!
| @@ -290,15 +303,15 @@ def _update_video_legacy(self, filepath: Path) -> dict: | |||
| stats["width"] = image.width | |||
| stats["height"] = image.height | |||
| if success: | |||
| self.preview_vid.play(filepath_, QSize(image.width, image.height)) | |||
| self.media_player.show() | |||
There was a problem hiding this comment.
I see that there's still separate _update_video_legacy() and _update_media() methods despite these now both being given responsibilities for the new combined media player. All methods and mentions of the "legacy" video player should be removed and combined into the "media" player logic, as this introduction of the combined media player is what is intended to replace the legacy video player (and greatly rework the audio player).
There was a problem hiding this comment.
Working on this, and I had an idea. The QMediaPlayer class has its own metaData property. There is a lot of useful information potentially available in the metadata, including video resolution. This could replace the use of cv2 to get these stats, making the _update_media method a bit cleaner. There is a downside though.
From what I can tell, certain metadata isn't immediately available. We need to wait for the media to load before we can access it. This would require us to connect to a signal for metadata changes. Also, this would require us to change how we get file metadata in general. Instead of an immediate response, from update_preview, we would need to have a general signal to send the stats dictionary.
What are thoughts on this? It might be a bit overkill, so I'd also be fine leaving the cv2 logic in place.
|
Hi @CyanVoxel, Sorry for not responding sooner, I was away last weekend. Thank you for doing a review! I appreciate all the feedback. I'll take some time today and work through your suggestions. |
fix: apply suggestions from code review. Co-authored-by: Travis Abendshien <[email protected]>
Combine the stats logic for video/audio into one method. Fix several issues after incorrectly implementing suggestions.
|
Hi @CyanVoxel,
Would it be beneficial to have looping as a setting, like autoplay? This is pretty easy to implement. Only downside is the setting needs to be translated. |
This would be great! I would do it exactly like how the autoplay setting is done, and have the default behavior be set to true. For the translation key, I would make it: "media_player.loop": "Loop",And remember that the en.json file is sorted alphabetically, so in this case the key will be immediately below the autoplay key. |
|
Tried adding the delete action with this last commit, but I'm not sure I got it working. The loop setting works okay, but I can't get the delete action to show for some reason. I've added the action to both the
|
Make a single method to control widget state. Works with the main QStackLayout and cleans up widget state if it is needed (i.e., stopping the media player when switching to a different preview).
The delete action seems to appear and work just fine for me (macOS) 🤔 Although I've got a new issue where the program will launch with the preview panel now taking up half the width of the application window: |
Fixes a regression in commit 4c6934. We need the pages to properly center the widgets in the QStackLayout.
|
Okay, I think I know where I went wrong. I was accidentally running an older version of the code somehow. This most recent commit should fix the issues. Thanks! |
Fix and issue where the media_player would expand past the thumbnail on resize.
|
With my latest commit, I believe I've fixed one of my biggest issues with this feature. I think it should be good enough to be reviewed now. Please let me know if there are any more issues. Thanks! |
Initial commit to merge audio/video files. There are still a few bugs around widget sizing that need fixing.