Moon-0xff / gnome-mpris-label

A music related GNOME extension.
GNU General Public License v3.0
50 stars 10 forks source link

Fix broken player filtering #32

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

Player filtering is currently broken. Please see proposed update.

Moon-0xff commented 1 year ago

Creating a dbus proxy without a callback provokes shell freezes when the player is slow to respond (like with shortwave).
This isn't noticeable this time because _filterList is called only when an mpris address is added or removed of dbus but the freezes are still there.

To experience continuous short freezes again: Open shortwave and start typing on the ignore list.

Moon-0xff commented 1 year ago

It looks like I simply forgot to match identity with blacklist/whitelist. This should be solved just by matching with the right thing: .filter(element => whitelist.includes(element.identity.toLowerCase().replaceAll(' ','')));

But! doing this will not solve the problem because entryProxy is called with a callback and identity isn't ready when _filterList is called, therefore it errors out with element.identity is undefined

Batwam commented 1 year ago

Yeah, I kept getting element.identity undefined. I figured that it was because this.list only contains the mpris list from DBUS but not the players info.

Batwam commented 1 year ago

I guess a quick and dirty alternative would be to use address like chromium for the purpose of filtering... since the active sources are listed at the top, the user would see that there is no Brave or Chrome in the list. Or we ignore it and let Shortwave fix its issue.

Moon-0xff commented 1 year ago

Too dirty! Although we haven't released a version with the identity changes so I don't discard the option.

I think the best way would be to emit a signal from _onEntryProxyReady or adding a pause in the code using GLib.timeout_add

Batwam commented 1 year ago

Wasn't there a way to make this asynchronous so it wouldn't freeze the shell? How did you manage to fix the Shortwave slowdowns last time?

Moon-0xff commented 1 year ago

Creating entryProxy with a callback is making the proxy creation asynchronously, in the sense that doesn't slow down or freeze the main thread while is doing it.
This however will delay the execution of the callback getting us to this scenario, where we need to access a value that's declared in the callback but the callback still hasn't been called, so the value isn't defined.

Moon-0xff commented 1 year ago

The execution order is this one: _updateList -> _filterList -> _onEntryProxyReady

But we are expecting values declared in _onEntryProxyReady which comes later in execution. We are trying to bake the cake before adding any eggs.

Batwam commented 1 year ago

Shouldn't the player filtering work after a few refresh iterations once element.identity becomes available though? I waited for a while and I don't think that it did. The Identity info is used to build the menu and the names were definitely listed but element.identity remained undefined. Or we need to call the list filtering once _onEntryProxyReady has completed?

Maybe a stupid idea but would there be value in checking the filtering as part of the Player Class after _onEntryProxyReady? Perhaps we include a flag filtered true/false so when the info becomes available, at the next iteration where the player list is updated, we check this flag element.filtered (not the identity) and that source get filtered out? I hope that makes sense...

Obviously, we would also need to update the filtered status if the blacklist/whitelist get updated so maybe that gets us back to square one... anyway, just a thought.

Moon-0xff commented 1 year ago

It works right after, but the code needs to be called again, the simplest way is to redefine the ignore list after the player is spawned to include it. This also lead me to a crash but let's ignore that fact.

Keep in mind that _filterList is only called when: a. An mpris address is added or removed to the dbus list b. filter options are modified

This in code translates to:

this.dBusProxy.connectSignal('NameOwnerChanged',this._updateList.bind(this)); //and the code in _updateList
this.settings.connect('changed::mpris-sources-blacklist',this._filterList.bind(this));
this.settings.connect('changed::mpris-sources-whitelist',this._filterList.bind(this));
this.settings.connect('changed::use-whitelisted-sources-only',this._filterList.bind(this));

Which is part of _initList

Batwam commented 1 year ago

sorry, I messed up as I pushed this PR having modified my main. As the initial proposed solution isn't the effective, I will resubmit using a separate branch and we can work through the proposed solution. See new working branch in #34