Regex label filter list (superseded)

[Batwam]

As discussed, alternative implementation for filter list. This is an initial pull request to share the working version. It is not yet functional as I only currently included the field in the Settings panel and associated preference settings.

I included the current filters as default values which helps give an idea of the syntax.

[Batwam]

ok, I haven't tested extensively but it seems to work for me using regular js string match function. I am applying it to the lowercase strings so it is equivalent to the previous \i regex match. I also included some explainations in the settings as tooltip.

There should also be ways to treat the entries as regex expressions: https://www.golinuxcloud.com/use-variable-inside-regex-javascript We could look at using that instead if we wanted to allow fancier filtering solutions but the current implementation seems to do the trick.

[Batwam]

I just realised looking at your last commit on the other branch that using the match function allows to use regex syntax directly. I tried with things like remaster|mix and it works fine. it doesn't seem to like *,^ or $ though which might be a good thing although I wouldn't mind having the ability to remove anything in () or after -. No sure if there is an easy fix to implement true regex. perhaps the link above can give us some clues.

[Batwam]

ok, this seems to work with ^, $ and *. it still needs some testing. For some reason, if I put * in the list, the entire label gets deleted if it has a section in () or - ...

I noticed that if I end the filter with , everything after ( and - gets filtered out which is cool so my new filter list consists in a single , 😀

[Moon-0xff]

The first implementation works very well, I'm thinking of including it regardless of the success of the regex one. In that case, let's not constrict the regex to only work in the "Additional information" segment.

edit: and checkout the lengthy explanation i added

[Batwam]

I think that the regex version is pretty neat actually as anyone not familiar with regex can still enter the text as normal. I'm logging off now so feel free to make any updates.

[Moon-0xff]

I will add a separate entry for the regex one, and restore the first implementation, both are pretty good additions to the extension.

[Batwam]

Is there any value in having two entries if they both do the same thing and the latest regex capable code can also be used with a "normal" list?

[Moon-0xff]

Less chance of user mistakes.

[Moon-0xff]

Sorry, I can't contribute more than this today.

[Batwam]

No worries. Are you considering applying the filtering on all fields rather than just the title?

I had a look and it's starting to look a bit busy. Since both fields do the same thing, one thing I was thinking about if we want to keep the interface clean could be having a switch saying "treat entry as regex" right above the entry field so we only keep one text field. We can then have this set to be disabled by default and perhaps have any warning to be visible only when regex is on (or put the warning as a tooltip on the switch)?

Regarding the risk of malicious regex, I'm yet to find an example of how this could harm a machine since we are not exposing a server and we are simply matching expressions, not manipulating a string. Is there any example of malicious regex you could find which could apply to our situation?

If there is a specific risk like some sort of endless loop which would end up freezing gnome-shell, we could perhaps build in some sort of time limit which would exit the loop and turn regex off (refer to switch above)?

[Moon-0xff]

removeRemasterText filters all metadata fields.

The intention is not to replicate the functionality of the first entry with a regex, but to give regex filtering capabilities to all the field string. This would still be repetition as with a regex you can replicate the behaviour of the first entry.

The risk of regex is denial of service (unless the engine is bugged). The worst thing that i imagine it could happen is an early and permanent (or permanent enough) freeze at startup that would render gnome-shell unusable.

To implement a time limit we would have to decouple the function from the main thread using async. That would not only give us the ability to measure the time but also would negate the possibility of the scenario above.

[Moon-0xff]

/(?:-|\().*(?:remaster|mix|feat\.|featuring).*(?:$|\))/i for a regex that replicates the behaviour.

As a side note i did that one in a few minutes so i was skilled enough after all.

[Batwam]

Of course you are skilled enough! I'm having quite a lot of fun with these regex rules too 😅

Anyway, I think that we have a few of the building blocks, I'll leave you to it if that's ok.

I'm just worried we might be running out of things to improve on this extension if we continue at that rate... I'll try to think of something!

[Moon-0xff]

It's obviously ok but i have busy days ahead so it will not be done quickly.

And talking about improvements of the extension i agree we are almost done feature wise. Other features i can think of are arguably unnecessary or better served with another extension. This is not at all a bad thing!

We should discuss the future of the extension when the next release is done.

[Batwam]

Sounds good and we can continue the discussion on roadmap and extra features (if any) on the other thread #4 👍

[Batwam]

hey, I just noticed that some tiles use [] instead of (). Could we add this together with () and - as delimiter?

[Moon-0xff]

I tested the implementation a bit and it works as expected.

I also added [] as delimiters.

[Batwam]

Ok, I've stress tested it a little bit:

• note that if you use * in the top filter, the entire label gets wiped out as the regex match fails and matchedSubStrings gives Mpris Label: SyntaxError: nothing to repeat
• On the other hand, ,* will remove anthing between () or [] or after - which is pretty neat :-)

I think that if someone enters , he would expect the fields to get cleared so `should behave the way,currently behaves. Not sure if this can be fixed by updating the regex formula issue mishandlingor we would need to manually changeto,` as an intermediate step if the user entered it?

For the second filter, similar thing, it wipes out the text and icon if I enter * while I would expect each field to become empty and see only the separator and icon. This time, ,* does nothing. At the moment, entering * basically crashes the label and icon like with the first field.

On a slightly unrelated topic, I was thinking that the user might want not to display any label considering that he has the icon and the mouse controls would still allow source control + access to the menu. Would we consider to allow showing the icon only (allow setting first field to none)?

[Moon-0xff]

We should catch those syntax errors (hoping that all of those errors are indeed catch-able syntax errors)

For the last idea i would say it kinda negates the point of the extension, but implementing controls to the label widens the scope of the extension to the point I'm considering to rename it (on ego at least) to Mpris Label & Controls or something like that.

[Batwam]

Yeah, I see what you mean, although the icon only case wouldn't really be the primary mode, just something which is possible due to the high customisation abilities. Maybe we keep that for another PR as it doesn't really bother me.

If you are going to rename, I would suggest including "Media" or "Music" somewhere as Mpris isn't that well known and probably not the first thing people would search for as some users already pointed out.

I have to say that after testing this PR for a few minutes, I already miss the mouse controls from the other PR for volume and play/pause. I can't wait for both of these PRs to be merged!

[Moon-0xff]

I'm also thinking about using "Media" in the title. Specially because it seems to me that mpris-less players are a minority. And of those that don't support mpris right out of the box (like rhythmbox or mpv) they are plugins or patches with mpris support added to them.

[Batwam]

ok, I looked into this and technically * is not a valid regex syntax (.* is) so I think that the fact that it doesn't work is fair enough.

I have now included a check for regex syntax as well as a warning so the user knows that the syntax used in incorrect. This will hopefully catch all errors and minimise the risk of label crash. I could have included some sort of notification or set it too red but this felt the easiest.

I also updated the descriptions to something which felt clearer (to me).

Finally, I also note that, .* will clear all fields so if someone would like to have icon only, this is already possible in a somewhat convoluted way by setting the label filter to .* and separator to :-)

[Moon-0xff]

Good idea i was thinking of simply logging the error once per session. This is a better approach.

The commit confuses me though, and it looks (haven't tested it) non functional.
We can probably catch the syntax errors with a try...catch block.

[Batwam]

works fine on my end. it's a two step process ( 1) check if there is an error using the funciton, 2) if there is an error, return datastring) which would probably made into one.

Do you mean merging the function and simply do return datastring within the catch(e) part?

[Moon-0xff]

I missed the try...catch part! 😅

Either way a check inside prefs.js would be better, that way we don't need to pass the result back to prefs to display the message.

[Moon-0xff]

I added a warning icon to signify a syntax error. The icon gets loaded correctly and at the right time, getting the icon to go away is pending.

We should connect the entry to the test instead of the option directly, that way we avoid the need to recheck the regex in label.js

The extra additions of the last commit were problematic (settings can't be a global object and the additional information filter works for all the fields) so i reverted it completely.

This latest version spams the logs so that needs to be fixed too.

[Batwam]

Yeah, it's better to have it in Pref.js. I kind of wanted to move it and do some formatting like turning the regex red but I ran out if time.

I'll leave you to it and focus on the controls PR as it looks like you have this under control.

[Batwam]

I will close this and resubmit as a clean PR based on latest main.

I don't think that you already have a working branch version implementing these changes do you?