Mpris blacklist support #390

abmantis · 2024-02-25T19:11:14Z

Continuation of PR #324.

…nto mpris_blacklist

abmantis · 2024-02-25T19:16:39Z

When I run the formatter, there are quite a few changes to the mpris.vala file. I am assuming the style rule was updated meanwhile. Let me know if you want me to commit those changes too.

MrPenguin07 · 2024-03-05T02:50:33Z

Continuation of PR #324.

Great to see progress on this.... and to see where my code was convoluted :)
I'll pull this now and report back.

Regards

MrPenguin07 · 2024-03-05T03:17:37Z

Alright, so with config as so;

"mpris": {
    "image-size": 96,
    "image-radius": 6,
    "blacklist": [".*firefox.*"]
},

SwayNC reports at startup;

** Message: 13:13:39.774: mpris.vala:263: "org.mpris.MediaPlayer2.firefox.instance_1_5133" is blacklisted
** Message: 13:13:39.774: factory.vala:44: Loading widget: mpris

So regex is being respected, and the annoying firefox tabs each creating their own instance is no longer showing in SwayNC.

Thumbsup, I approve.

ErikReider · 2024-03-09T18:11:52Z

I'll get to this after the upcoming bug release :)

MrPenguin07 · 2024-03-10T06:57:33Z

I've noticed that firefox instances aren't always blacklisted from the mpris module;

Upon swaync startup, instances are identified and removed. Great.
However, sometimes an existing swaync session does not pickup new firefox tabs/mpris players.
When this happens, if I run $ swaync-client --reload-config the new firefox tabs are then removed from the mpris player.
I also noticed this happening with my original PR.

I've yet to produce a repeatable scenario for this, however have noticed it happening quite a few times.

ErikReider · 2024-03-25T15:02:16Z

I've noticed that firefox instances aren't always blacklisted from the mpris module;

Upon swaync startup, instances are identified and removed. Great. However, sometimes an existing swaync session does not pickup new firefox tabs/mpris players. When this happens, if I run $ swaync-client --reload-config the new firefox tabs are then removed from the mpris player. I also noticed this happening with my original PR.

I've yet to produce a repeatable scenario for this, however have noticed it happening quite a few times.

I'll wait until this is resolved then

abmantis · 2024-03-25T15:14:37Z

I've noticed that firefox instances aren't always blacklisted from the mpris module;
Upon swaync startup, instances are identified and removed. Great. However, sometimes an existing swaync session does not pickup new firefox tabs/mpris players. When this happens, if I run $ swaync-client --reload-config the new firefox tabs are then removed from the mpris player. I also noticed this happening with my original PR.
I've yet to produce a repeatable scenario for this, however have noticed it happening quite a few times.

I'll wait until this is resolved then

May I suggest that we merge this still? It still improves the functionality since currently, FF will always be duplicated anyway. Also we avoid having stale PRs for things that are already done (even if there is some unknown issue).

If there is some quirk, it can be fixed later. It would also help having more people use this and reporting so we can pinpoint the issue.

Otherwise, we risk never finding the cause and this PR will just stay open forever.

MrPenguin07 · 2024-03-25T15:27:48Z

I've noticed that firefox instances aren't always blacklisted from the mpris module;
Upon swaync startup, instances are identified and removed. Great. However, sometimes an existing swaync session does not pickup new firefox tabs/mpris players. When this happens, if I run $ swaync-client --reload-config the new firefox tabs are then removed from the mpris player. I also noticed this happening with my original PR.
I've yet to produce a repeatable scenario for this, however have noticed it happening quite a few times.

I'll wait until this is resolved then

I believe, without reviewing the code but from the observed behavior, that this is simply caused by the mpris widget lacking a hook which checks if new players are in the blacklist prior to adding them first.
Thus starting/reloading Swaync will correctly identify and blacklist existing players on startup, however especially with firefox where new instances/players come and go, we lack the mechanism to cross reference the blacklist before adding it.

@abmantis makes some points, though can also accept it's best a feature is complete before merge.
Unless you beat me to it, i'll get onto this later this week regardless.

ErikReider · 2024-03-25T15:34:20Z

I'd rather not merge incomplete features

abmantis · 2024-04-20T23:31:32Z

@MrPenguin07 any findings?

abmantis · 2024-05-09T23:35:24Z

@ErikReider I was now able to reproduce the issue mentioned by @MrPenguin07 and just fixed it.
@MrPenguin07 can you try out my last commit?

MrPenguin07 · 2024-05-10T09:30:44Z

@ErikReider I was now able to reproduce the issue mentioned by @MrPenguin07 and just fixed it. @MrPenguin07 can you try out my last commit?

I appreciate your taking the time to hash this one out, glad to see my suspicion was correct.
Just haven't had time to code much of anything these days :/

I've pulled your latest commit, built swaync and ....

Am happy to be greeted by this when a new player is added, without needing to reload swaync.
Good stuff @abmantis

I consider this feature to now be complete @ErikReider

abmantis · 2024-05-10T13:14:04Z

I initially misinterpreted your description of the issue, that is why I was not able to reproduce it.
Yesterday, after coming back to this and reading it again I noticed that it was in fact easy to reproduce :D

@ErikReider are you ok with moving this forward now?

rtgiskard · 2024-05-14T04:23:35Z

@abmantis I like this, but I think it's better to keep only necessary changes, flake.nix is not likely to be included in this project, and the message() can be removed, just to keep it simple. Except for the manual page, configSchema.json need to be synced too.

rtgiskard · 2024-05-14T05:57:51Z

Just fixed a typo-like bug for name_owner_changed signal handler skipped, which should resolve the issue above, and also this: #425.

It's now here in the main branch with another PR in process, If you're willing, I may create another PR including this feature with the fix.

abmantis · 2024-05-17T20:37:41Z

Closed by mistake. I thought this was pushed to main, but I was checking a fork 🤦

src/controlCenter/widgets/mpris/mpris.vala

abmantis · 2024-05-17T20:40:15Z

Removed the flake.nix file, which was committed by mistake.

abmantis · 2024-05-17T20:41:45Z

Just fixed a typo-like bug for name_owner_changed signal handler skipped, which should resolve the issue above, and also this: #425.

It's now here in the main branch with another PR in process, If you're willing, I may create another PR including this feature with the fix.

I think it would be better to keep this PR and to have separate PRs.

MrPenguin07 · 2024-05-23T11:13:45Z

Reporting: 2 weeks of testing these commits and the mpris blacklist continues to work perfectly.

rtgiskard · 2024-05-24T12:46:43Z

src/controlCenter/widgets/mpris/mpris.vala

@@ -174,6 +184,7 @@ namespace SwayNotificationCenter.Widgets.Mpris {
            string[] names = dbus_iface.list_names ();
            foreach (string name in names) {
                if (!name.has_prefix (MPRIS_PREFIX)) continue;
+                if (is_blacklisted(name)) continue;
                if (check_player_exists (name)) return;


You may change this line to fix the unintended return

Maybe I'm too ignorant about it, I'm seeing that my report was merged in this PR, but I don't know how to use info written here to fix the issue with my Swaync. I uninstalled the one I got from Arch repo, and compiled this one, still having the same issue: Swaync shows MPRIS module when nothing is being played, and when there's something being played, I have two different MPRIS modules in Swaync. What I am missing?

Thanks in advance.

Maybe I'm too ignorant about it, I'm seeing that my report was merged in this PR, but I don't know how to use info written here to fix the issue with my Swaync. I uninstalled the one I got from Arch repo, and compiled this one, still having the same issue: Swaync shows MPRIS module when nothing is being played, and when there's something being played, I have two different MPRIS modules in Swaync. What I am missing?

Thanks in advance.

By "compiled this one" do you mean you've cloned SwayNotificationCenter repo, pulled the commits to your repo and then built that?
Or are you manually patching the source?

I get the feeling you may simply be building master from this repo without the changes.

Yeah, just cloned and built it. No commits pulled. I don't know how to pull the commits separately, I tried to just copy/paste the changes but apparently I did it wrongly, and I wasn't able to modify the installation...

Sorry for the ignorance :(

EDIT: I found how to download mpris.vala and replaced the one in /src/controlCenter/widgets/mpris folder. Added "blacklist": [] in my config.json (the one I have configured already, since that's the one active in my dotfiles), rebuilt it, problem continues: I have two different mpris modules, and when nothing is being played, module is there.

EDIT 2: I finally got it. I'm a complete noob and I had no idea what to do, so just intuition led me to the answer: I had to write "playerctl" to blacklist it, and that's it.

I'll close the PR. Thanks for this solution!

MrPenguin07 and others added 7 commits October 1, 2023 21:07

Initial mpris blacklist support

c9676df

Merge branch 'ErikReider:main' into main

017c4d4

Merge branch 'main' of github.com:ErikReider/SwayNotificationCenter i…

aded144

…nto mpris_blacklist

Fix merge conflict

7f8b71f

Simplify blacklist type and parse

1c2155f

Fix man

4960ac2

Trim whitespaces

97b60af

Fix blacklisting when a new player appears

13bee9c

abmantis closed this May 17, 2024

abmantis reopened this May 17, 2024

Remove flake.nix

007f59b

abmantis commented May 17, 2024

View reviewed changes

src/controlCenter/widgets/mpris/mpris.vala Outdated Show resolved Hide resolved

Remove log

eeb9574

abmantis commented May 17, 2024

View reviewed changes

src/controlCenter/widgets/mpris/mpris.vala Outdated Show resolved Hide resolved

Remove comment

fb09aaf

rtgiskard mentioned this pull request May 19, 2024

Mpris: add blacklist support, fix setup_mpris(), sync doc #430

Closed

rtgiskard reviewed May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mpris blacklist support #390

Mpris blacklist support #390

abmantis commented Feb 25, 2024

abmantis commented Feb 25, 2024 •

edited

MrPenguin07 commented Mar 5, 2024

MrPenguin07 commented Mar 5, 2024

ErikReider commented Mar 9, 2024

MrPenguin07 commented Mar 10, 2024

ErikReider commented Mar 25, 2024

abmantis commented Mar 25, 2024

MrPenguin07 commented Mar 25, 2024

ErikReider commented Mar 25, 2024

abmantis commented Apr 20, 2024

abmantis commented May 9, 2024

MrPenguin07 commented May 10, 2024

abmantis commented May 10, 2024

rtgiskard commented May 14, 2024 •

edited

rtgiskard commented May 14, 2024 •

edited

abmantis commented May 17, 2024

abmantis commented May 17, 2024

abmantis commented May 17, 2024

MrPenguin07 commented May 23, 2024

rtgiskard May 24, 2024

andrewsebek May 25, 2024

MrPenguin07 May 25, 2024

andrewsebek May 25, 2024 •

edited

Mpris blacklist support #390

Are you sure you want to change the base?

Mpris blacklist support #390

Conversation

abmantis commented Feb 25, 2024

abmantis commented Feb 25, 2024 • edited

MrPenguin07 commented Mar 5, 2024

MrPenguin07 commented Mar 5, 2024

ErikReider commented Mar 9, 2024

MrPenguin07 commented Mar 10, 2024

ErikReider commented Mar 25, 2024

abmantis commented Mar 25, 2024

MrPenguin07 commented Mar 25, 2024

ErikReider commented Mar 25, 2024

abmantis commented Apr 20, 2024

abmantis commented May 9, 2024

MrPenguin07 commented May 10, 2024

abmantis commented May 10, 2024

rtgiskard commented May 14, 2024 • edited

rtgiskard commented May 14, 2024 • edited

abmantis commented May 17, 2024

abmantis commented May 17, 2024

abmantis commented May 17, 2024

MrPenguin07 commented May 23, 2024

rtgiskard May 24, 2024

Choose a reason for hiding this comment

andrewsebek May 25, 2024

Choose a reason for hiding this comment

MrPenguin07 May 25, 2024

Choose a reason for hiding this comment

andrewsebek May 25, 2024 • edited

Choose a reason for hiding this comment

abmantis commented Feb 25, 2024 •

edited

rtgiskard commented May 14, 2024 •

edited

rtgiskard commented May 14, 2024 •

edited

andrewsebek May 25, 2024 •

edited