Moon-0xff / gnome-mpris-label

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

Update Broken Player Filtering (new) #34

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

Follow-up on #32 with separate rebased working branch. This is a testing branch which doesn't yet solve the issue.

As per other PR, player filtering is no longer effective since it occurs before the identity information is made available: _updateList -> _filterList -> _onEntryProxyReady

_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

Ok, I tried to do a simple fix by including the filter update in updateActiveList(). It worked for the list but the label no longer showed the song info. As I had to go do something else and didn't want to leave a broken implementation, I have reverted these changes.

This still could be a solution to explore if we can't work out a neater way to trigger the filter callback.

Edit: I just realised that it was taking th extension into an endless loop as _filterList() calls updateActiveList()...

Batwam commented 1 year ago

ok, I played around trying to put a ~2s delay with SetTimeout() to work around the premature identity check. I couldn't get it to work so updated the code to call _filterList() on updateActiveList() which means that it runs on every iteration so if it doesn't work the first time around, it will later. I don't think that it add much more complexity to the code than what we already have.

I also noticed that if a source was playing and gets filtered, it stays in the label so I am proposing to clear this.selected if the player is no longer part of the player list because it was filtered.

Batwam commented 1 year ago

By the way, have you considered increasing the default refresh delay from 300ms to 500ms or even 1s? that would reduce the number of cycles quite a bit over time and I doubt anyone would notice.

Moon-0xff commented 1 year ago

Calling _filterList inside updateActiveList is a cheat! But updateActiveList is in itself a cheat. I'm not against this approach but we should definitely seek to replace it later (alongside updateActiveList)

If we follow this implementation it would be better to call _filterList directly from _refresh and drop the underscore, for clarity.

Moon-0xff commented 1 year ago

Compared against past implementations, _refresh isn't a slow function , and it's still tied to refreshing the label (which we want to refresh reasonably fast), so I don't see the point of increasing the refresh delay.