Refactor video_player.py (Fix #270)

[CyanVoxel]

This PR makes various changes to video_player.py, with many involving where the SVG icons are stored and how they're loaded. In the process this fixes #270.

Changes

• Move icons files to resources/qt/images folder, some being renamed
• Reduce icon loading to single initial import
• Tweak icon dimensions and animation timings
• Remove unnecessary commented code
• Remove unused/duplicate imports
• Add license info to file

[Loran425]

Hmmmm, not seeing icons in the exe build but that might be something on my end.

[Loran425]

I think its because its a local path so tagstudio is no longer valid after packaging, swapping to _internal breaks it when running from python but works when running from the build.

I think we need to figure out how to incorporate that into the resources_rc file and then change the path to be similar to the way the splash screen is loaded.

[CyanVoxel]

I think its because its a local path so tagstudio is no longer valid after packaging, swapping to _internal breaks it when running from python but works when running from the build.

I think we need to figure out how to incorporate that into the resources_rc file and then change the path to be similar to the way the splash screen is loaded.

I wonder why these are the only assets breaking when all of the other assets that aren't in the resources_rc are still working... And I tested out building it on my end beforehand, at least the portable version, and it worked with this change

[Loran425]

Hmmm does it work for you as is with pyinstaller.exe .\tagstudio.spec called from the root of your local repo?

If I had to guess it has something to do with that Path(__file__).parents[3] bit. I think the "right" way is the resources file, but because those files are in the same structure it works for them, but for this one it didn't reference the __file__ so it didn't get updated from tagstudio to _internal
I somehow checked out a version that wasn't working, retesting

[Loran425]

@CyanVoxel I somehow pulled an old version of this PR sorry for the confusion
This is working now.

[CyanVoxel]

@CyanVoxel I somehow pulled an old version of this PR sorry for the confusion This is working now.

Great news haha 😁

[yedpodtrzitko]

I'd keep these resources outside of the VideoPlayer class, and rather have another class for handling this kind of assets, ResourceManager or something like that. Then it can be used across the whole codebase for all the resources, which would be nicer than doing open(Path(_parents, "resources/qt/something/something))` everywhere.
This will provide some level of abstraction, so if/when the underlying implementation will change, it wont affect any code using the resources.

Something like this:

class ResourceManager:
    MAP = {
        "play_icon": "qt/images/play.svg",
        # ...
    }

    def _load(self, id: str):
        # eventually implement cache like
        # if id in self.CACHE: return self.CACHE[id]
        with open(self.MAP[id], 'rb') as f:
            return f.read()

    def get(self, id: str):
        """Allow to use the manager as:
        r = ResourceManager()
        r.get('play_icon')
        """
        return self._load(id)

    def __getattr__(self, id: str):
        """Allow to use the manager as:
        r = ResourceManager()
        r.play_icon
        """
        if id in self.MAP:
            return self._load(id)
        raise AttributeError(f'attribute {id} not found')

[Loran425]

For the Qt side I'd be leaning toward utilizing the built-in resource manager

This would let the resources be managed outside this file and hopefully avoid future path issues for them. though it does add a step to remember whenever we want to add an image asset, and these are technically hidden imports as they aren't exposed to python so much as a resource made available to Qt in the background by that import src.qt.resources_rc line in ts_qt.py.

Adding these images to the resources.qrc file

<!-- TO COMPILE: pyside6-rcc resources.qrc -o resources_rc.py (in \tagstudio\src\qt)-->
<RCC>
    <qresource prefix="/">
        <file alias = "images/star_icon_empty_128.png">../../resources/qt/images/star_icon_empty_128.png</file>
		<file alias = "images/star_icon_filled_128.png">../../resources/qt/images/star_icon_filled_128.png</file>
        <file alias = "images/box_icon_empty_128.png">../../resources/qt/images/box_icon_empty_128.png</file>
		<file alias = "images/box_icon_filled_128.png">../../resources/qt/images/box_icon_filled_128.png</file>
        <!-- <file alias = "images/edit_icon_128.png">../../resources/qt/images/edit_icon_128.png</file> -->
        <!-- <file alias = "images/trash_icon_128.png">../../resources/qt/images/trash_icon_128.png</file> -->
        <!-- <file alias = "images/clipboard_icon_128.png">../../resources/qt/images/clipboard_icon_128.png</file> -->
        <file alias = "images/splash.png">../../resources/qt/images/splash.png</file>
        <file alias = "images/pause.svg">../../resources/qt/images/pause.svg</file>
        <file alias = "images/play.svg">../../resources/qt/images/play.svg</file>
        <file alias = "images/volume_mute.svg">../../resources/qt/images/volume_mute.svg</file>
        <file alias = "images/volume.svg">../../resources/qt/images/volume.svg</file>
    </qresource>
</RCC>

would let you only reference these images with a colon and their alias so the update controls function on L182 ends up like this.

    def updateControls(self) -> None:
        if self.player.audioOutput().isMuted():
            self.mute_button.load(":images/volume_mute.svg")
        else:
            self.mute_button.load(":images/volume.svg")

        if self.player.isPlaying():
            self.play_pause.load(":images/pause.svg")
        else:
            self.play_pause.load(":images/play.svg")

[CyanVoxel]

I've added a basic ResourceManager class that shoulders the work that the video_player.py was doing on its own. For this PR I'm continuing to use a system separate from the QResource system since I've run into reliability inconsistinces with it in the past. With that being said this ResourceManger class provides a layer between the resources and the rest of the app, meaning that it can abstract the QResource system for future drop-in replacements. This abstraction can also aide in returning theme-specific resources without unnecessary intervention from the classes asking for them.

I've tested the resource changes in the executable build as well and have found no regressions with the resource fix.

[yedpodtrzitko]

this would sure work, but maybe there's some room for improvement

[yedpodtrzitko]

this will raise exception in case the resource is not present in ._map, because at that point the value you will get is None, and that cant be assigned into two variables path, mode:

>>> path, mode = None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot unpack non-iterable NoneType object

storing the data as dictionary inside _map instead of extracted properties would prevent this issue.

btw. the default fallback value of .get() is None, so no need to define that explicitly as the second parameter of .get()

assert dict().get('foo') is None

[yedpodtrzitko]

I'd initialize this class inside QtDriver instead of here, as this will be needed in other places in the codebase as well, and that way you'll have the same instance available everywhere. Since driver is passed here, then it would be used as self.driver.rm.play_icon instead of VideoPlayer.rm.play_icon

[yedpodtrzitko]

• this would be better to do in constructor, otherwise doing import of this class will have "side-effect" (ie. opening and loading from json file), similar way as doing import src.qt.resources_rc has side-effect of prefilling some QT resources

• when using .items(), you can work with it as two variables (key, value), so you dont have to unpack it via numeric index (item[0] item[1]) later

-for item in json_map.items():
-    _map[item[0]] = (item[1]["path"], item[1]["mode"])
+ for key, value in json_map.items():
+    _map[key] = (value["path"], value["mode"])

• to make this future-proof, I'd assign just the whole value instead of extracting specific keys in case there will be some other property in future. This will also mitigate a potential issue further on (see my other comment elsewhere)

-_map[key] = (value["path"], value["mode"])
+_map[key] = value

...and at that point you can do just this:

-json_map: dict = ujson.load(f)
-for item in json_map.items():
-   _map[item[0]] = (item[1]["path"], item[1]["mode"])
+_map = ujson.load(f)

[yedpodtrzitko]

Suggested change

	if res.get("mode") == ["rb"]:
	if res.get("mode") == "rb":

[CyanVoxel]

Thanks for the review, @yedpodtrzitko !