Moon-0xff / gnome-mpris-label

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

Simple User Defined Title Filtering #56

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

Third time lucky perhaps with a basic implementation of a user-defined filter for titles under Label in the Preferences: image

It's basically the same as the current implementation for remaster but allows additional strings to be used via coma separated entry like for the whitelist/blacklist. As these strings are eventually turned into a regex expression, I'm also catching invalid filters using the function previously developed.

Moon-0xff commented 1 year ago

As mentioned before, exposing the regex to the user introduces a lot of problems, they aren't impossible to solve but certainly isn't worth the effort.
Any user that wants to add regex filtering or even more complex string manipulation can and is encouraged to edit the source code.

But this is certainly a good addition to the extension, and I think the best way of adding this would be to not expose regex to the user.

We might be able to introduce this without much problems if we escape the regex control characters from the user input, although, that would mean that we will need to check if a gjs update adds new ones, shouldn't be a problem!

Batwam commented 1 year ago

I guess we could filter out some special characters relatively easily from the filter variable to make it unusable as regex. What characters would you want to exclude?

We can do this silently in the code before the regex validation check. Alternatively, we could filter the keywords and paste the filtered values back into the field to show that they aren't allowed.

Moon-0xff commented 1 year ago

The reference probably mentions all control characters.

Regex control codes are mostly ASCII graphical characters, so maybe escaping or forbidding those might be enough.

Batwam commented 1 year ago

See proposed implementation, filtering all special characters except , (obviously).

I have included a feedback where the filtered list gets put back into the entry so the user can get visual feedback of the fact that the special characters have been removed. This is not strictly necessary as we could do this silently but I felt that it's a bit clearer in case they don't read the tooltip and think that the fact that these characters are ignored is a bug.

Moon-0xff commented 1 year ago

Working with a regex made solely of special characters is very fun!

As I previously mentioned, filtering the values on prefs.js would be better so we don't need to pass information from label.js back to prefs.

I've tried to do this with the code for #41 with an added workaround for Gtk4 but didn't work.
Here's a non working diff.

Here's the regex line (haven't tested it):

const filterRegex = new RegExp('!"\\$%&\\(\\)\\*\\+-\\.\\/:;<=>\\?@\\[\\\\\\]\\^_`{\\|}~','gi');

I replaced the regex literal for a RegExp call because my text editor was having problems with the syntax highlighting (no wonder).

Worth mentioning that the regex is ordered.

Batwam commented 1 year ago

Thank for the response. I also tried (unsuccessfully) to implement filtering at pref.js level. I based it on the code from here which is based on changed. While I could make it do something like enter the filtered info in a different entry, it wouldn't work if I tried to put the data back into label-filtered-list. (That's only a theory but I have the feeling that there may be something preventing an entry from updating itself on changed as it could lead to an endless loop)

In addition, I remember you mentioned preferences that using changed could lead to a crash. I thought about using activate but it also wouldn't update the entry and the problem I had with this method is that, assuming that it only gets triggered when the user presses Enter, it means that the unfiltered entry would be used by label.js until the user presses Enter.

I believe that we would have the same issue with your implementation using focus: the special characters would only get filtered once the user clicks somewhere else. So, if I'm not mistaken, someone could type and run regex code as long as he doesn't click somewhere else, that string would get executed.

With the current implementation within label.js on the other hand, the special characters get filtered just they would get used so no risk of special characters making their way into the code. Also, it will only run as frequently as the extension loops (every 300ms by default) so we shouldn't have the issue you mentioned with the repeated keystrokes. (I am reading that the average time between keystrokes is 70-150ms. It was probably even shorter when you stress tested it)

Last but not least, it works 😅

Moon-0xff commented 1 year ago

Fixed the regex above and refactored the code with it.

I don't think there's a possibility of malformed regexes now, so I removed the validation.

Moon-0xff commented 1 year ago

The regex for some reason filters ,, but not #. Not sure why.

Batwam commented 1 year ago

I haven't spent too much time testing the latest commits but # has been removed from the list of flitered character as part of ba169de304eb8d794083883971b939b42aa43da0 so that seems logical?

Formating-wise, I personnaly find const filteredString = LABEL_FILTERED_LIST.replace(/[`~!@#$%^&*()_|+\-=?;:'".<>\{\}\[\]\\\/]/gi, ''); more readable than

const GraphCharactersRegex = new RegExp('[!"\\$%&\'\\(\\)\\*\\+-\\.\\/:;<=>\\?@\\[\\\\\\]\\^_`{\\|}~]','gi');
const sanitizedInput = LABEL_FILTERED_LIST.replace(GraphCharactersRegex,"")

as the first format does avoid the need for escape \\ for a lot of the characters

Batwam commented 1 year ago

the above works for me, basically copying the original string back in without quotes to avoid the multiple \\ but keeping the 2 lines as you had it. I am not sure why the previous one removed the , characters but the new regex doesn't.

With regards to #, it's debatable whether they should be removed or not as they don't appear to be regex characters anyway? Same thing with %, _ and probably a few others. In fact, based on this, the following might be sufficient: /([.?*+^$[\]\\(){}|-])/ to "de-regexify" the string. This seems consistent with the list of characters listed under "Special Regex Characters" here. being . + * ? ^ $ ( ) [ ] { } | \

Moon-0xff commented 1 year ago

I've testing it a bit and the regex behaves well, as mentioned before filtering just the control characters shouldn't be a problem.

Overall I think this is ready to be merged, but testing is still needed. It might be better to just push it and daily drive it for a while, it's not like we are pushing it as an update!

Batwam commented 1 year ago

Sounds good to me 👍

Moon-0xff commented 1 year ago

Done! I think I should have renamed the PR though! It's not very clear what it does.