search

ErikReider / SwayNotificationCenter

A simple GTK based notification daemon for SwayWM
GNU General Public License v3.0
1.29k stars 62 forks source link

Mpris blacklist support #390

Closed abmantis closed 2 months ago

abmantis commented 7 months ago

Continuation of PR #324.

abmantis commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

MrPenguin07 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

I'd rather not merge incomplete features

abmantis commented 5 months ago

@MrPenguin07 any findings?

abmantis commented 4 months ago

@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 commented 4 months ago

@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 .... image

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 commented 4 months ago

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 commented 4 months ago

@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 commented 4 months ago

Just fixed a typo-like bug for name_owner_changed signal handler skipped, which should resolve the issue above, and also this: https://github.com/ErikReider/SwayNotificationCenter/issues/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 commented 4 months ago

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

abmantis commented 4 months ago

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

abmantis commented 4 months ago

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 commented 4 months ago

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

abmantis commented 3 months ago

@ErikReider anything else required for this?

andrewsebek commented 3 months ago

@ErikReider anything else required for this?

Should be merged, it works flawlessly.

MrPenguin07 commented 2 months ago

LGTM :)

andrewsebek commented 1 month ago

Sorry for bumping this again, but I have to comment something:

I reinstalled Arch Linux and installed my dotfiles one more time a couple days ago, and installed Swaync from the main repo (not building it from source) and the problem persists.

My code in mpris:

"mpris": { "image-size": 60, "image-radius": 12, "blacklist":["playerctld"] },

Am I missing something? I saw it was merged already, so I was expecting this correction to be available in Arch repos already.

MrPenguin07 commented 1 month ago

Sorry for bumping this again, but I have to comment something:

Am I missing something? I saw it was merged already, so I was expecting this correction to be available in Arch repos already.

"mpris": {
"image-size": 60,
"image-radius": 12,
"blacklist":["playerctld"]
},

Presuming you've copy-pasted, try adding a space between "blacklist": and ["playerctld"]

ErikReider commented 1 month ago

Sorry for bumping this again, but I have to comment something:

I reinstalled Arch Linux and installed my dotfiles one more time a couple days ago, and installed Swaync from the main repo (not building it from source) and the problem persists.

My code in mpris:

"mpris": { "image-size": 60, "image-radius": 12, "blacklist":["playerctld"] },

Am I missing something? I saw it was merged already, so I was expecting this correction to be available in Arch repos already.

It's not in a current release yet, you'll need to install swaync-git from the AUR

andrewsebek commented 1 month ago

Sorry for bumping this again, but I have to comment something:

I reinstalled Arch Linux and installed my dotfiles one more time a couple days ago, and installed Swaync from the main repo (not building it from source) and the problem persists.

My code in mpris:

"mpris": { "image-size": 60, "image-radius": 12, "blacklist":["playerctld"] },

Am I missing something? I saw it was merged already, so I was expecting this correction to be available in Arch repos already.

It's not in a current release yet, you'll need to install swaync-git from the AUR

It totally worked on the -git version. I'll be there until the stable branch is ready. Thanks for the response!