search

Moon-0xff / gnome-mpris-label

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

Make updating label event based (superseded by #68) #60

Closed abisammy closed 1 year ago

abisammy commented 1 year ago

This is event based way of updating the label that can be used.

However it's not fully event based, for example it has to still use the old system refreshing when no players are active, however when there is one active it is fully event based (try logging in the update method) Have tested:

Still need to test a few things such as:

Let me know if you can think of a method to not have to search for players

abisammy commented 1 year ago
Moon-0xff commented 1 year ago

Great work with this, however I definitely would prefer an implementation based around the methods and object Gjs provides.

I haven't been able to get a result using gjs signals and the other stuff but maybe you can?

Here's some links for research:

Moon-0xff commented 1 year ago

Signals might be the wrong implementation, I'm not sure, definitely read gjs.guide for good measure.

abisammy commented 1 year ago

I don't believe the players class is a GObject, so I don't think we can use signals?

I'll try it a bit later

Moon-0xff commented 1 year ago

The class needs to be subclassed from GObject to gain the ability.
As my previous attempt does, or the guide in subclassing of gjs.guide shows.

Batwam commented 1 year ago

May I suggest finalising and merging the open PRs before embarking into this exercise considering how much it might be changing the underlying code base?

Moon-0xff commented 1 year ago

It might make more sense to merge this one first actually.
Most of the open PRs benefit greatly from this change.

abisammy commented 1 year ago

TODO:

abisammy commented 1 year ago

@Moon-0xff @Batwam Made the changes I need to this branch, I think these changes need to be tested extensively... Would it be possible for you guys to test this, and let me know if you spot any issues pls.

Please note I am aware of the bug that text is not removed when paused.

Moon-0xff commented 1 year ago

The diff against main is too big.

For this reason I avoided adding extra indentation when converting MprisLabel to GObject on 28b96aed4fb8db2119f38cca2e10ceba3691d395.

There's also way too much lines with changed indentation/style but no changed code. This further breaks git diff and git blame

Sorry, but this is just too difficult to review, can you remove the extra indentation, and revert the style changes? edit: Actually, I think I prefer a new branch with current main as the starting point 😁

Batwam commented 1 year ago

As a side note, I did a bit of research on the topic and found that it is possible to ignore a commit in the blame by creating a file called .git-blame-ignore-revs and including the hash of the commit to be ignored. https://github.com/orgs/community/discussions/5033

Not sure if this is a github specific setting as I haven't tested it but if reformatting is required, this might be worth keeping im mind but perhaps as a separate cleanup PR altogether as the diff might still be affected.

Moon-0xff commented 1 year ago

As a side note, I did a bit of research on the topic and found that it is possible to ignore a commit in the blame by creating a file called .git-blame-ignore-revs and including the hash of the commit to be ignored.

Very useful. I don't think it applies here though!

If it works as I think it works then we can probably do #64 without issues.

If it doesn't or requires some user configuration we can do #64 with less problems. Not a deal breaker.

Batwam commented 1 year ago

good point, I hadn't noticed #64 but that would certainly work there. My understanding is that it should be as simple as adding the file in the root directory but it would be worth checking that it works as intended. the diff will probably still show the changes which is why it would be easier to implement if there is a PR purely dedicated to it rather than combining that with other code change.

abisammy commented 1 year ago

Been testing this, couple bugs I noticed:

Will fix in coming days. Shouldn't be too difficult, just need to link settings to relevant update methods.

note: I've also created a branch with these changes and the changes in https://github.com/Moon-0xff/gnome-mpris-label/pull/58

abisammy commented 1 year ago

Also I just noticed the update method is trigerred when switching applications, perhaps this can be disabled to further optimise performance.

Fixed!

Moon-0xff commented 1 year ago

As mentioned I can't review this. Please start another branch from main.

abisammy commented 1 year ago

Please do not add changes that doesn't pertain to the pull request, ideally the changes should be explained only by the PR title

All changes in the PR are related to removing the refresh rate, the previous commits were bug fixes for it?

I don't think this was closed fairly... The code had already been merged with main and was up to date.

Moon-0xff commented 1 year ago

Sorry, it looks we aren't on the same page, my problems with the branch are explained in this comment.

I use git diff and git blame to review and understand the submitted code, this branch doesn't play well with those commands and I can't confidently know where are the changes in the code. Therefore I can't review or merge this.

abisammy commented 1 year ago

Sorry, it looks we aren't on the same page, my problems with the branch are explained in this comment.

I use git diff and git blame to review and understand the submitted code, this branch doesn't play well with those commands and I can't confidently know where are the changes in the code. Therefore I can't review or merge this.

If you reopen the branch, I can make a commit to remove the indents and styling... This saves me from manually having to copy the changes...

Moon-0xff commented 1 year ago

Can you undo the styling changes cleanly? I'm skeptic about it.

abisammy commented 1 year ago

Done!

Moon-0xff commented 1 year ago

Amazing!
I will review these changes when I'm able. I was expecting a bigger diff honestly.

abisammy commented 1 year ago

Merged with new changes

abisammy commented 1 year ago

New bugs:

Moon-0xff commented 1 year ago

Mpris Label: Error: Expected type string for argument 'text' but got type boolean

Odd, perhaps the result value of removeTextWhenPaused?

abisammy commented 1 year ago

Mpris Label: Error: Expected type string for argument 'text' but got type boolean

Odd, perhaps the result value of removeTextWhenPaused?

I had thought so aswell...

I will look into it over the next few days

abisammy commented 1 year ago

~~For some reason the bug isn't being reproduced at the moment... It could be that I had the wrong branch installed that had the bug.~~ I'll continue monitoring over the next few days, otherwise the feature might be working as intentionally.

Seems to be occuring again, will collect more data over the next few days

Moon-0xff commented 1 year ago

I've been examining the changes of this branch and I've been thinking of rewriting the Players class so we don't need to handle the inner workings of Players from extension.js (which is a workaround, it was not intended)

I forked this branch to do this, I can't show anything useful yet.
But the general idea is to use proxy-changed and a signal when players.selected changes to update the label/icon accordingly instead of list-changed

As this implementation is working (although, last time I tested this branch it didn't work) I suggest to not change it.

I will share a branch soon enough.

Moon-0xff commented 1 year ago

Branch is here under the name refreshless, I removed a few things, added support for GNOME <42 and made the structure changes I wanted.

I started backwards to make the structure changes first, and then connected everything. Sadly I wasn't able to make it work, I got this error:

JS ERROR: Error: Could not guess unspecified GValue type
update@[...]/players.js:251:8

And I don't know how to solve it.

edit: It might be necessary to cut support for GNOME <42 to solve this correctly. If that's the case then I guess we should.

Moon-0xff commented 1 year ago

The above error doesn't appear on GNOME 43, I pushed a few more changes and now the branch works, although functionality is missing and sometimes the code is executed out of order.

It looks like the problems I had with making the extension refreshless had something to do with the gnome version I was testing with.

Moon-0xff commented 1 year ago

For a clearer log, I've submitted this changes, plus the ones I've added separately as another PR (#68). You can, of course, add more changes by creating a PR targeting the refreshless branch.

Thanks @abisammy for this PR!