feat: port thumbnail (#390) and related features to v9.5 by CyanVoxel · Pull Request #522 · TagStudioDev/TagStudio

CyanVoxel · 2024-09-22T21:45:58Z

This PR ports the following thumbnail and related PRs from the Alpha-v9.4 branch to main (v9.5+):

Blender thumbnail support (Blender thumbnail support #273) by @050011-code
Add font thumbnail preview support (Add font thumbnail preview support #307) by @Thesacraft
refactor: move type constants to new media classes (refactor: move type constants to new media classes #331) by @CyanVoxel
feat(ui): expanded thumbnail and preview features (feat(ui): expanded thumbnail and preview features #390) by @CyanVoxel
ui: "open in explorer" action follows os name (Change "show file in explorer" to be different depending on the OS #370) by @SupKittyMeow
feat(ui): preview support for source engine files (Add support for VALVe Source 1 + 2 files #373) by @FavroiteGamers
Refactor video_player.py (Fix [BUG]: Video Playback Icons Missing in Executables #270) (Refactor video_player.py (Fix #270) #274) by @CyanVoxel
feat(ui): show file creation/modified dates + restyle path label (feat(ui): show file creation/modified dates + restyle path label #430) by @CyanVoxel
fix(ui): use default audio icon if ffmpeg is absent (fix(ThumbRenderer): Use audio icon when no ffmpeg #471) by @seakrueger
fix(ui): use birthtime for creation time on mac & win (fix(PreviewPanel): Use birthtime for creation time on Mac & Windows #472) by @seakrueger
other miscellaneous commits

Many 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.

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 <[email protected]> Co-Authored-By: Theasacraft <[email protected]> Co-Authored-By: SupKittyMeow <[email protected]> Co-Authored-By: EJ Stinson <[email protected]> Co-Authored-By: Sean Krueger <[email protected]>

yedpodtrzitko

I'd recommend doing smaller PRs instead, which would be easier to review.

yedpodtrzitko · 2024-09-23T10:08:50Z

requirements.txt

+mutagen==1.47.0
+numpy==1.26.4
+ffmpeg-python==0.2.0
+Send2Trash==1.8.3


this dependency looks unused

Send2Trash was accidentally added due to its inclusion in future a v9.4 PR, fixed 👍

yedpodtrzitko · 2024-09-23T10:10:33Z

tagstudio/src/core/media_types.py

+
+
+class MediaCategories:
+    """Contains pre-made MediaCategory objects as well as methods to interact with them."""


Suggested change

"""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."""

yedpodtrzitko · 2024-09-23T10:14:21Z

tagstudio/src/core/media_types.py

+    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


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

yedpodtrzitko · 2024-09-23T10:18:11Z

tagstudio/src/core/media_types.py

+
+    @staticmethod
+    def get_types(ext: str, mime_fallback: bool = False) -> set[MediaType]:
+        """Returns a set of MediaTypes given a file extension.


Suggested change

"""Returns a set of MediaTypes given a file extension.

"""Return a set of MediaTypes given a file extension.

yedpodtrzitko · 2024-09-23T10:22:30Z

tagstudio/src/core/media_types.py

+                type: str = mimetypes.guess_type(Path("x" + ext), strict=False)[0]
+                if type and type.startswith(cat.media_type.value):


type is a name of builtin, so it would be better to use different variable name.

yedpodtrzitko · 2024-09-23T10:34:54Z

tagstudio/src/qt/ts_qt.py

                    )

+    def thumb_size_callback(self, index: int):
+        """Performs actions needed when the thumbnail size selection is changed.


Suggested change

"""Performs actions needed when the thumbnail size selection is changed.

"""Perform actions needed when the thumbnail size selection is changed.

yedpodtrzitko · 2024-09-23T10:40:50Z

tagstudio/src/qt/widgets/collage_icon.py


-                if filepath.suffix.lower() in IMAGE_TYPES:
+                ext: str = filepath.suffix.lower()
+                if MediaType.IMAGE in MediaCategories.get_types(ext):


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.

yedpodtrzitko · 2024-09-23T10:43:01Z

tagstudio/src/qt/widgets/preview_panel.py

+        if self.afm.is_connected:
+            self.afm.done.disconnect()


afm is not really good variable name, please use some more descriptive name

yedpodtrzitko · 2024-09-23T10:48:27Z

tagstudio/src/qt/widgets/preview_panel.py

+                        f"[PreviewPanel][ERROR] Couldn't Render thumbnail for {filepath} "
+                        f"(because of {e})"


I'd suggest not to cram everything into the f-string. structlog allows to use extra variables (see previous version), which is way more readable:

yedpodtrzitko · 2024-09-23T10:53:37Z

tagstudio/src/qt/widgets/thumb_renderer.py

+        is_loading=False,
+        is_grid_thumb=False,
+        update_on_ratio_change=False,


why are the types removed here?

Add method to check if an extension is a member of a given MediaCategory.

CyanVoxel · 2024-09-26T00:29:53Z

Implemented suggested feedback, thanks for the review 👍

yedpodtrzitko

almost there :)

yedpodtrzitko · 2024-09-26T00:35:13Z

tagstudio/src/qt/resource_manager.py

                ResourceManager._map = ujson.load(f)
-                logger.info("resources registered", count=len(ResourceManager._map.items()))
+                logger.info(
+                    f"[ResourceManager] {len(ResourceManager._map.items())} resources registered"


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.

yedpodtrzitko · 2024-09-26T00:36:14Z

tagstudio/src/qt/resource_manager.py

            ResourceManager._initialized = True

+    @staticmethod
+    def get_path(id: str) -> Path | None:


this looks unused 🤔

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

yedpodtrzitko · 2024-09-26T00:38:29Z

tagstudio/src/qt/resource_manager.py

+            except FileNotFoundError:
+                logger.error(
+                    "[ResourceManager][ERROR]: Could not find resource: "
+                    f"{Path(__file__).parents[2] / "resources" / res.get("path")}"


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.

yedpodtrzitko · 2024-09-26T00:50:05Z

tagstudio/src/qt/widgets/preview_panel.py

+        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)


self.trash_term doesnt look used anywhere else than on the line below it, so a local variable trash_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 the QAction once again, which feels wasteful.

having both these labels decided in the same if-elif-else could be better.

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

yedpodtrzitko · 2024-09-26T00:53:16Z

tagstudio/src/qt/widgets/preview_panel.py

+                    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):


is it possible to use is_ext_in_category() here as well?

yedpodtrzitko · 2024-09-26T00:57:07Z

tagstudio/src/qt/widgets/thumb_renderer.py

+            logger.error(
+                f"[ThumbRenderer][ERROR]: Couldn't read album artwork for {filepath.name} "
+                f"({type(e).__name__})"
+            )


Suggested change

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)

yedpodtrzitko · 2024-09-26T00:58:14Z

tagstudio/src/qt/widgets/thumb_renderer.py

+            logger.error(
+                f"[ThumbRenderer][WAVEFORM][ERROR]: Couldn't render waveform for {filepath.name} "
+                f"({type(e).__name__})"
+            )


Suggested change

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)

yedpodtrzitko · 2024-09-26T01:02:00Z

tagstudio/src/qt/widgets/video_player.py

+        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)


❓ this code already exists in preview_panel.py, cant it be reused here somehow?

CyanVoxel · 2024-09-27T00:35:41Z

@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

yedpodtrzitko · 2024-09-27T03:38:21Z

tagstudio/src/qt/platform_strings.py

+    if platform.system() == "Windows":
+        open_file_str = "Open in Explorer"
+    elif platform.system() == "Darwin":
+        open_file_str = "Reveal in Finder"


Having the logic directly in the class isnt the usual way how to do it, but I think we can live with it here 🤔

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

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

CyanVoxel and others added 5 commits September 22, 2024 13:39

remove vscode exceptions from .gitignore

d438da1

delete .vscode directory

80fec3d

style: format for ruff check

ca2d369

fix(tests): update test_update_widgets_not_selected test

9b16c46

CyanVoxel added Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience Priority: Medium An issue that shouldn't be be saved for last Status: Review Needed A review of this is needed labels Sep 22, 2024

CyanVoxel added this to the SQL Parity milestone Sep 22, 2024

yedpodtrzitko reviewed Sep 23, 2024

View reviewed changes

CyanVoxel added 9 commits September 23, 2024 14:18

remove Send2Trash dependency

5bb3a3f

refactor: use dataclass for MediaCateogry

bd61dfc

refactor: use enums for UI colors

fb50ddd

docs: add file docstring for silent_Popen

ed3e695

refactor: replace logger with structlog

d36e0a4

use early return inside ResourceManager.get()

9a6b012

add is_ext_in_category() method to MediaCategory

8b8c8c1

Add method to check if an extension is a member of a given MediaCategory.

style: fix docstring style, missing type hints, rename afm

161b95e

fix: use structlog vars in logging

7fae3ba

CyanVoxel removed the Status: Review Needed A review of this is needed label Sep 26, 2024

yedpodtrzitko reviewed Sep 26, 2024

View reviewed changes

CyanVoxel added 3 commits September 26, 2024 17:27

refactor: move platform-dependent strings to PlatformStrings

2daca77

refactor: move parents[2] path to variable

d782bbf

fix: undo logger regressions

81ca944

yedpodtrzitko approved these changes Sep 27, 2024

View reviewed changes

seakrueger mentioned this pull request Sep 28, 2024

feat: Reimplement drag drop files on sql migration #528

Merged

CyanVoxel added the Status: Mergeable The code is ready to be merged label Oct 3, 2024

Merge branch 'main' into thumb-port-v9.5

95d631e

CyanVoxel merged commit 7dd0f3d into main Oct 7, 2024

CyanVoxel deleted the thumb-port-v9.5 branch October 7, 2024 21:14

CyanVoxel removed Priority: Medium An issue that shouldn't be be saved for last Status: Mergeable The code is ready to be merged labels Oct 7, 2024



		class MediaCategories:
		"""Contains pre-made MediaCategory objects as well as methods to interact with them."""

	"""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."""

	@dataclass
	class BranchData:
	dirs: dict[str, "BranchData"] = field(default_factory=dict)
	files: list[str] = field(default_factory=list)
	tag: Tag \| None = None

	"""Returns a set of MediaTypes given a file extension.
	"""Return a set of MediaTypes given a file extension.

		type: str = mimetypes.guess_type(Path("x" + ext), strict=False)[0]
		if type and type.startswith(cat.media_type.value):

	"""Performs actions needed when the thumbnail size selection is changed.
	"""Perform actions needed when the thumbnail size selection is changed.

		f"[PreviewPanel][ERROR] Couldn't Render thumbnail for {filepath} "
		f"(because of {e})"

Uh oh!

Conversation

CyanVoxel commented Sep 22, 2024

Uh oh!

yedpodtrzitko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyanVoxel commented Sep 26, 2024

Uh oh!

yedpodtrzitko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yedpodtrzitko Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyanVoxel commented Sep 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yedpodtrzitko Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yedpodtrzitko Sep 26, 2024 •

edited

Loading

yedpodtrzitko Sep 27, 2024 •

edited

Loading