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
Video Player #106
Video Player #106
Conversation
I dont have strong opinion about the feature itself, and it's not up to me to decide, but the README mentions the code should follow pep-8. That means the camelCases names in code should rather be snake_cases. |
Alright, thanks for telling me. |
tagstudio/src/qt/widgets/__init__.py
Outdated
from .preview_panel import PreviewPanel | ||
from .video_player import VideoPlayer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think about the import order, if preview panel relies on video player, then the video player needs to be before the preview panel in the init file
from .video_player import VideoPlayer | |
from .video_player import VideoPlayer | |
from .preview_panel import PreviewPanel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to change some of these logging.info
calls to logging.debug
call to avoid overwhelming the other terminal information.
from src.qt.widgets import (ThumbRenderer, FieldContainer, TagBoxWidget, TextWidget, PanelModal, EditTextBox, | ||
EditTextLine, ItemThumb) | ||
from .video_player import VideoPlayer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from src.qt.widgets import (ThumbRenderer, FieldContainer, TagBoxWidget, TextWidget, PanelModal, EditTextBox, | |
EditTextLine, ItemThumb) | |
from .video_player import VideoPlayer | |
from src.qt.widgets import (ThumbRenderer, FieldContainer, TagBoxWidget, TextWidget, PanelModal, EditTextBox, | |
EditTextLine, ItemThumb, VideoPlayer) |
Should just need to add VideoPlayer
to the src.qt.widgets
import on line 24-25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried that, but python gave me a circular dependency error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tossed in another comment, I didn't think about import order, VideoPlayer
needs to be imported in the __init__
file before PreviewPanel
Code review aside, I'm leaning towards a thumbnail with a play button on it being the default and then clicking it starting the preview but I'm open to what the community wants, I'd just be worried about responsiveness in remote storage cases which might not be the typical setup. |
2e645a7
to
7ef2aa6
Compare
https://youtu.be/sWr5EN5hRI8
Added a simple video player for Tag Studio. Currently, the player will automatically mute itself when switching to another video. This is by design, as playing a really loud video before someone is ready will probably cause heart-attacks.
Also, I noticed the big refactor, and I am not fully familiar with the structure. I currently have VideoPlayer imported like this.
Could someone tell me the correct way of importing my video player in preview_panel.py? Thanks!