search

Moon-0xff / gnome-mpris-label

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

Regex Label Filtering (clean) #31

Closed Batwam closed 1 year ago

Batwam commented 1 year ago

PR #25 final changes applied to latest main for clarity. Note sure why there were merge conflicts as it's a direct copy paste of the old code but here you go.

Batwam commented 1 year ago

for info, I am not convinced that the changes from this commit work 3044260. As far as I can tell, filtersPage._settings.connect('changed::user-regex-filter', () => { isn't getting called when the filter is modified. I can't see any warning sign and the filter value isn't changed to the code will still try to filter and therefore generate errors in the log: image

Besides fixing the above, I still think that we need some level of filtering at filterAdditionalInfo(datastring) and filterUserRegex(datastring) level to make sure we don't feed incorrect syntax.

Batwam commented 1 year ago

Would it be worth providing the ability to specify which sources the metadata filters should apply to, similarly to the Ignore/Allow lists? Leave blank to apply to all sources.

Batwam commented 1 year ago

Following up from the comments in #52, I don't think that this feature is necessarily a must have but I still think that there is value in extending the range of possible filters beyond "Remaster". If the current PR is too ambitious, we could implement just for the title field, especially since we already have functioning code to achieve this.

Would it be possible to perhaps implement a simplified version of this, focussing solely on filtering title metadata? if we want to keep things simple for the user and avoid regex, the original #25 PR commit https://github.com/Moon-0xff/gnome-mpris-label/pull/25/commits/e024a8db5b0c2c721759afc410ba0df10b775f29 was based on basic match.

Moon-0xff commented 1 year ago

Weighing the potential problems, needed changes, and maintainability concerns versus the added value to the extension, I don't think adding this is worth it.

I think the best choice would be to return to the more limited #24 implementation, and make the extension easier to understand and edit.

Batwam commented 1 year ago

Ok, I'll close this PR then