-
Notifications
You must be signed in to change notification settings - Fork 380
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
feat: port thumbnail (#390) and related features to v9.5 #522
Conversation
Ports the following thumbnail and related PRs from the `Alpha-v9.4` branch to `main` (v9.5+): - (#273) Blender thumbnail support - (#307) Add font thumbnail preview support - (#331) refactor: move type constants to new media classes - (#390) feat(ui): expanded thumbnail and preview features - (#370) ui: "open in explorer" action follows os name - (#373) feat(ui): preview support for source engine files - (#274) Refactor video_player.py (Fix #270) - (#430) feat(ui): show file creation/modified dates + restyle path label - (#471) fix(ui): use default audio icon if ffmpeg is absent - (#472) fix(ui): use birthtime for creation time on mac & win Co-Authored-By: Ethnogeny <111099761+050011-code@users.noreply.github.com> Co-Authored-By: Theasacraft <91694323+Thesacraft@users.noreply.github.com> Co-Authored-By: SupKittyMeow <77246128+supkittymeow@users.noreply.github.com> Co-Authored-By: EJ Stinson <93455158+favroitegamers@users.noreply.github.com> Co-Authored-By: Sean Krueger <71362472+seakrueger@users.noreply.github.com>
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'd recommend doing smaller PRs instead, which would be easier to review.
requirements.txt
Outdated
mutagen==1.47.0 | ||
numpy==1.26.4 | ||
ffmpeg-python==0.2.0 | ||
Send2Trash==1.8.3 |
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.
this dependency looks unused
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.
Send2Trash was accidentally added due to its inclusion in future a v9.4 PR, fixed 👍
tagstudio/src/core/media_types.py
Outdated
|
||
|
||
class MediaCategories: | ||
"""Contains pre-made MediaCategory objects as well as methods to interact with them.""" |
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.
"""Contains pre-made MediaCategory objects as well as methods to interact with them.""" | |
"""Contain pre-made MediaCategory objects as well as methods to interact with them.""" |
tagstudio/src/core/media_types.py
Outdated
def __init__( | ||
self, | ||
media_type: MediaType, | ||
extensions: set[str], | ||
is_iana: bool = False, | ||
) -> None: | ||
self.media_type: MediaType = media_type | ||
self.extensions: set[str] = extensions | ||
self.is_iana: bool = is_iana |
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.
Since this is just a data wrapper, I'd suggest to use dataclass
instead. It will save you from writing the constructor and assigning the values in it manually.
@dataclass(frozen=True)
class MediaCategory:
media_type: MediaType
extensions: set[str]
is_iana: bool # maybe assign default value, so you dont have to initialize it explicitly half of the times?
example of usage eg. here:
TagStudio/tagstudio/src/qt/modals/folders_to_tags.py
Lines 33 to 37 in 49d071c
@dataclass | |
class BranchData: | |
dirs: dict[str, "BranchData"] = field(default_factory=dict) | |
files: list[str] = field(default_factory=list) | |
tag: Tag | None = None |
tagstudio/src/core/media_types.py
Outdated
|
||
@staticmethod | ||
def get_types(ext: str, mime_fallback: bool = False) -> set[MediaType]: | ||
"""Returns a set of MediaTypes given a file extension. |
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.
"""Returns a set of MediaTypes given a file extension. | |
"""Return a set of MediaTypes given a file extension. |
tagstudio/src/core/media_types.py
Outdated
type: str = mimetypes.guess_type(Path("x" + ext), strict=False)[0] | ||
if type and type.startswith(cat.media_type.value): |
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.
type
is a name of builtin, so it would be better to use different variable name.
tagstudio/src/qt/ts_qt.py
Outdated
@@ -800,6 +817,34 @@ def run_macro(self, name: MacroID, grid_idx: int): | |||
content=strip_web_protocol(field.value), | |||
) | |||
|
|||
def thumb_size_callback(self, index: int): | |||
"""Performs actions needed when the thumbnail size selection is changed. |
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.
"""Performs actions needed when the thumbnail size selection is changed. | |
"""Perform actions needed when the thumbnail size selection is changed. |
@@ -74,7 +76,8 @@ def render( | |||
color=self.get_file_color(filepath.suffix.lower()), | |||
) | |||
|
|||
if filepath.suffix.lower() in IMAGE_TYPES: | |||
ext: str = filepath.suffix.lower() | |||
if MediaType.IMAGE in MediaCategories.get_types(ext): |
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.
This check is quite inefficient - get_types()
accumulates all the categories and then you check if it's present in the results.
It would be better to do the check in a way that it returns True
as soon as it finds a match, so it doesnt need to go through all the remaining categories if it knows it will be a match after the (for example) first comparison.
Same applies for the other similar checks below.
if self.afm.is_connected: | ||
self.afm.done.disconnect() |
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.
afm
is not really good variable name, please use some more descriptive name
f"[PreviewPanel][ERROR] Couldn't Render thumbnail for {filepath} " | ||
f"(because of {e})" |
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.
is_loading=False, | ||
is_grid_thumb=False, | ||
update_on_ratio_change=False, |
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.
why are the types removed here?
Add method to check if an extension is a member of a given MediaCategory.
Implemented suggested feedback, thanks for the review 👍 |
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.
almost there :)
tagstudio/src/qt/resource_manager.py
Outdated
@@ -23,9 +24,26 @@ def __init__(self) -> None: | |||
if not ResourceManager._initialized: | |||
with open(Path(__file__).parent / "resources.json", encoding="utf-8") as f: | |||
ResourceManager._map = ujson.load(f) | |||
logger.info("resources registered", count=len(ResourceManager._map.items())) | |||
logger.info( | |||
f"[ResourceManager] {len(ResourceManager._map.items())} resources registered" |
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.
Is there any rationale to shove the variables into f-string, or is it a bad merge? The extra parameters are better place where to keep them.
ResourceManager._initialized = True | ||
|
||
@staticmethod | ||
def get_path(id: str) -> Path | None: |
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.
this looks unused 🤔
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.
This gets used by another feature in 9.4 (file deleting) - I could remove it here but I would be adding it back in the next PR here anyways
tagstudio/src/qt/resource_manager.py
Outdated
except FileNotFoundError: | ||
logger.error( | ||
"[ResourceManager][ERROR]: Could not find resource: " | ||
f"{Path(__file__).parents[2] / "resources" / res.get("path")}" |
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.
nit: Path(__file__).parents[2] / "resources" / res.get("path")
is repeated three times in total, that could be worth extracting it into a separate variable.
self.trash_term: str = "Trash" | ||
if platform.system() == "Windows": | ||
self.trash_term = "Recycle Bin" | ||
self.delete_action = QAction(f"Send file to {self.trash_term}", self) | ||
|
||
self.open_explorer_action = QAction("Open in explorer", self) # Default text (Linux, etc.) | ||
if platform.system() == "Darwin": | ||
self.open_explorer_action = QAction("Reveal in Finder", self) | ||
elif platform.system() == "Windows": | ||
self.open_explorer_action = QAction("Open in Explorer", self) |
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.
-
self.trash_term
doesnt look used anywhere else than on the line below it, so a local variabletrash_term
would be sufficient, no reason to keep it in scope of the class. -
doing the same thing for Open action is totally different appoach - instead of changing only the label value, it creates a whole
QAction
, and in case the platform doesnt match, it creates theQAction
once again, which feels wasteful.
having both these labels decided in the same if-elif-else could be better.
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've removed the trash_term/delete_action as those were part of a separate PR that got mixed in here. Otherwise I've split out the explorer action string so it (and future strings) can be reused
if ( | ||
(MediaType.IMAGE in MediaCategories.get_types(ext)) | ||
and (MediaType.IMAGE_RAW not in MediaCategories.get_types(ext)) | ||
and (MediaType.IMAGE_VECTOR not in MediaCategories.get_types(ext)) | ||
): | ||
image = Image.open(str(filepath)) | ||
elif filepath.suffix.lower() in RAW_IMAGE_TYPES: | ||
elif MediaType.IMAGE_RAW in MediaCategories.get_types(ext): |
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.
is it possible to use is_ext_in_category()
here as well?
logger.error( | ||
f"[ThumbRenderer][ERROR]: Couldn't read album artwork for {filepath.name} " | ||
f"({type(e).__name__})" | ||
) |
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.
logger.error( | |
f"[ThumbRenderer][ERROR]: Couldn't read album artwork for {filepath.name} " | |
f"({type(e).__name__})" | |
) | |
logger.error("Couldn't read album artwork", path=filepath, error=e) |
logger.error( | ||
f"[ThumbRenderer][WAVEFORM][ERROR]: Couldn't render waveform for {filepath.name} " | ||
f"({type(e).__name__})" | ||
) |
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.
logger.error( | |
f"[ThumbRenderer][WAVEFORM][ERROR]: Couldn't render waveform for {filepath.name} " | |
f"({type(e).__name__})" | |
) | |
logger.error("Couldn't render waveform", path=filename, error=e) |
system = platform.system() | ||
open_explorer_action = QAction("Open in explorer", self) # Default, usually Linux | ||
if system == "Darwin": | ||
open_explorer_action = QAction("Reveal in Finder", self) | ||
elif system == "Windows": | ||
open_explorer_action = QAction("Open in Explorer", self) |
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.
❓ this code already exists in preview_panel.py
, cant it be reused here somehow?
@yedpodtrzitko Implemented the new feedback 👍 Let me know how you feel about the PlatformStrings file and if there's anything else I may have missed |
if platform.system() == "Windows": | ||
open_file_str = "Open in Explorer" | ||
elif platform.system() == "Darwin": | ||
open_file_str = "Reveal in Finder" |
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.
Having the logic directly in the class isnt the usual way how to do it, but I think we can live with it 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.
For future reference what would be the expected way to do something like this? I was mulling over different implementations and just went with this since it minimizes the performance impact of the checks while keeping the imports somewhat clean
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.
just briefly - I would probably go slightly different way - rather than doing everything in one class, I'd create a class with common phrases, and then inherit from that class like WindowStrings
where platform-specific phrases would be overriden.
class SharedStrings:
open_file_str: str = 'generic value'
generic_string: tr = 'shared among all classes'
class WindowsStrings(SharedStrings):
open_file_str: str = 'windows specific value'
TRANSLATIONS = # `platform.system()` logic to decide which class to use
This PR ports the following thumbnail and related PRs from the
Alpha-v9.4
branch tomain
(v9.5+):video_player.py
(Fix #270) #274) by @CyanVoxelMany of these features and changes were closely intertwined with the focused PR (#390) to where it didn't make sense to spend the extra time and effort to try and comb them apart just for a cleaner git history. If I missed any other PRs or attributions, please let me know.