Skip to content
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

Fix loading albumArt from Chromium MPRIS #280

Merged
merged 6 commits into from
Oct 23, 2024

Conversation

EndarValuk
Copy link
Contributor

@EndarValuk EndarValuk commented Oct 10, 2024

Added new handle for albumArt from MPRIS metadata provided by Chromium based browser in a PlayerRow.

Before:
Снимок экрана от 2024-10-04 13 36 27

After:
Снимок экрана от 2024-10-10 10 13 01

Fixes #279

@EndarValuk EndarValuk marked this pull request as ready for review October 10, 2024 05:23
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Historically we have avoided adding application specific workarounds. Can this be reproduced with other flatpak apps as well? Can this be solved in way that isn't specifically hardcoding the Chrome RDNN?

For example at a minimum maybe can we check for the existing of the file in /tmp and if it isn't there, then we can check if it exists in .flatpak / application_id? I think we should be able to get the app ID from MPRIS

Probably should also use GLib.Path to build paths https://valadoc.org/glib-2.0/GLib.Path.build_path.html

@EndarValuk
Copy link
Contributor Author

@danirabbit

Historically we have avoided adding application specific workarounds. Can this be reproduced with other flatpak apps as well? Can this be solved in way that isn't specifically hardcoding the Chrome RDNN?

Checked, its not a flatpak-specific "feature" of chromium. Its a "chromium" way.
Same behaviour persists even when using "deb-based" chromium distribution.
I will take look on it, for which apps its an applicable case.

For example at a minimum maybe can we check for the existing of the file in /tmp and if it isn't there, then we can check if it exists in .flatpak / application_id? I think we should be able to get the app ID from MPRIS

I will take look on it, to implement a cleaner way.

Probably should also use GLib.Path to build paths https://valadoc.org/glib-2.0/GLib.Path.build_path.html

I will update code according to that note

@EndarValuk
Copy link
Contributor Author

EndarValuk commented Oct 15, 2024

@danirabbit

I think we should be able to get the app ID from MPRIS

atleast, we got very strange behavior, dbus sending us something like that:
org.mpris.MediaPlayer2.chromium.instance33959
but we got an com.google.Chrome app name actually.

And if i'm right, current implementation doesnt allow us to get an actual app_info from stripped ifaces.
It will require refactoring a bit.

Fast way solution, updated:

            /**
            * For some MPRIS sources, we can see strange file uri like "file:///tmp/.com.google.Chrome.{Hash}",
            * files being stored in users runtime directory, and we should handle it properly to display albumArt.
            */
            if (! FileUtils.test (fname, FileTest.EXISTS)) {
                fname = Path.build_path (Path.DIR_SEPARATOR_S, GLib.Environment.get_user_runtime_dir (), ".flatpak", "com.google.Chrome", fname);
            }

UPDATED:

According to MPRIS spec we wont get actual google chrome app name, because chrome developers set it as chromium, but they r storing files under com.google.Chrome folder.
Working on it. Help appreciated.

@danirabbit
Copy link
Member

danirabbit commented Oct 15, 2024

Hm yeah this seems like it's just a bug upstream in Chromium and will need to be addressed there since we can't get the proper app id from either the Identity or DesktopEntry properties (DesktopEntry is null even). This happens in both Chrome and Chromium so just hardcoding Chrome's app ID here doesn't fix it.

Another thing I can think of, it's kind of brute force, but you could iterate over every folder in user_runtime_directory / .flatpak and check if the file exists in any of them. Not super clean, but it should work

Added Regex solution for strange app<->folder relations
@EndarValuk
Copy link
Contributor Author

Another thing I can think of, it's kind of brute force, but you could iterate over every folder in user_runtime_directory / .flatpak and check if the file exists in any of them. Not super clean, but it should work

Added little bit more elegant solution with a regex

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely a lot smarter than iterating over every folder, nice job! Just gotta make sure to stick to code style conventions re variable naming, spaces, etc.

https://docs.elementary.io/develop/writing-apps/code-style

See also linter output: https://github.com/elementary/wingpanel-indicator-sound/actions/runs/11359271917/job/31695048276?pr=280

@EndarValuk
Copy link
Contributor Author

EndarValuk commented Oct 21, 2024

@danirabbit code updated

UPDATE:
Re-done Regex from literal to instance, to prevent lint false erroring.

@danirabbit
Copy link
Member

Hey sorry for taking a couple days to respond. We usually prefer printf syntax over string literals. See https://docs.elementary.io/develop/writing-apps/code-style#string-formatting

@EndarValuk
Copy link
Contributor Author

EndarValuk commented Oct 23, 2024

Hey sorry for taking a couple days to respond. We usually prefer printf syntax over string literals. See https://docs.elementary.io/develop/writing-apps/code-style#string-formatting

I'm new in vala lang development(mostly use managed languages), but never encountered printf syntax over regexp literals.

Anyway I've updated PR code. No more linter errors occur.
Is it now ok, on ur perception, to be merged?

Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Nice fix!

@danirabbit danirabbit merged commit 66f44a0 into elementary:master Oct 23, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MPRIS artUrl not loaded from Chromium based browser
2 participants