Moon-0xff / gnome-mpris-label

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

Option to filter feat./Featuring text from label (superseded by #25) #24

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

As discuss, I'd like to propose an option to remove the "(feat. ...)" and other "Featuring ...)" texts in the label which take too much room and leads to excessively long strings. I've added this as an extra option under the one for Remaster (enabled by default like the Remaster one). I also slightly tweaked the tooltip wording to something which felt a bit clearer and updated the Remaster description for consistency. image

I checked against a range of titles I could find on Spotify (these are not from my personnal library, it was only for testing, I swear! 😅). It works as intended (doesn't touch the first title, filters out the next 3): image image

Moon-0xff commented 1 year ago

I added a new option to filter "Mix" text too, merged the functions together, and moved the options to the filters page.

I think the layout needs some tweaks, and the wording and "presentation" of the options isn't clear.

All this changes widens the scope of this PR by a lot, so it needs a new title.

Batwam commented 1 year ago

Ah ah, if we continue, this extension is going to become larger than Gnome itself, I'm starting to feel bad for the guy who is having to review our updates every week 😅

Good move on combining the filtering functions. It crossed my mind as there is a fair amount of repeated code but I didn't have time for it.

To be honest, there is a lot of junk in song title, I was looking at a playlist and there is a lot of "Recorded at ...", "Live ...". It would almost justify some comma separated list like what we have for the blacklist? Maybe we throw the existing ones in the list by default so people understand how it works and this way, they can add/remove as necessary?

Moon-0xff commented 1 year ago

I would like to include something like that but not in the way we are doing this.

For once, we could reduce all the code to a single regex (probably), but I'm not skilled enough to do that.
The best way of including this would be to pass the filtering and regex building to the user, but what are the implications of that or if it's safe to do it i don't know.

Batwam commented 1 year ago

I think that the other downside of the regex approach is that not everyone would be familiar with the syntax. Honestly, it's not really something the average Joe is familiar with and my guess is that most users would end up not using it. Since we aren't really doing fancy regex with text swap or removing multiple instances,... I still feel like a basic comma separated solution is fit for purpose.

Perhaps we include the regex solution as some sort of advanced feature in the future if we ever figure out how to do it but for now, I'd keep it simple and either list a few common ones with toggles or put in the text field with some comma separated examples if we want the user to be able to customise it.

Moon-0xff commented 1 year ago

I think the ones we have are sufficient. Honestly, if it wasn't for the latest Beatles releases I wouldn't have included the mix one.

Batwam commented 1 year ago

Ok, no worries.

I kind of liked the idea of the customisable list as it was fairly consistent with the general "highly customised" approach of the extension in general and the rest of the filters tab in particular. I also felt like I might want to add some more in the future but let's go with the toggles for now if you want.

Moon-0xff commented 1 year ago

Ok, your idea of including this as a comma separated list started to grow on me, it might need a lengthy explanation though.

edit: hmm but that would mean to implicitly expose the regex to the user... Once again, what are the implications of this? is it dangerous?

Batwam commented 1 year ago

Thanks! I think that by including default entries like remaster,mix ,featuring,feat. it should relatively self explanatory when it comes to the syntax.

Then we have the tooltip to explain what it does but if we are worries people may not realise there are tooltips, we could add a bit of text before (or after) the field.

Edit: on the regex part (assuming you are talking about the regex used in the code, not regex in the user input field), it would be interesting to see what happens if we put actual regex expressions like gr(a|e)y. Maybe we can implement proper regex after all. This said, I can't see how the user could do anything else than mess up the title of the song. All we are returning is a string. If it's a concern, there are probably alternative way to achieve the same thing with regular split and include type commands but again, it's not like we are hosting a website so the only data the user can access is his own.

Moon-0xff commented 1 year ago

If I'm not mistaken, badly formed regex can cause denial of service, i don't know how easy is to mess a regex up to that extent but probably we should put a warning. Better yet, we should encourage the users to test their regex in a terminal running gjs instead. Maybe we can add a test button and do the gjs testing inside prefs.

Batwam commented 1 year ago

The thing is that with the way the extension currently runs, whatever is in the field would be used straight away in the next iteration so as soon as they enter it, it's tested and the user will see the label change.

Frankly, we don't really have to promote this as a regex filter. If there is a risk of crash with people entering the wrong characters, we could perhaps strip out some of the most problematic special characters like * which could get the string to be misinterpreted, then put the filtered string back into the input field so the user can see that the characters been removed?

The best would be to give it a go a see how well it's working. Worst case we scrap it but I'm sure that there is a way.

Edit: example of regex ddos here (see comment from TimPietzcker), perhaps worth studying https://stackoverflow.com/questions/13719291/crash-proof-regex

Edit2: even better post, seems like filtering out ("+", "*") would help.

Moon-0xff commented 1 year ago

Ok, let's start another branch and try to do it as a list. If there's too much problems with the idea or implementation, then we can fallback to this branch and ultimately merge this PR.

Batwam commented 1 year ago

ok, I have created the filtering_test branch for this which is forked off the main branch. Probably easier just to start from scratch.