Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Regex/Bulk Edits #3671

Merged
merged 11 commits into from May 3, 2023
Merged

Add Regex/Bulk Edits #3671

merged 11 commits into from May 3, 2023

Conversation

yayuyokitano
Copy link
Member

@yayuyokitano yayuyokitano commented Apr 23, 2023

BREAKING: upgrades required node version to 18
BREAKING: modifies some legacy pipeline behavior, potentially causing inconsisten behavior to v2, in particular with normalize pipeline.

Adds a regex/bulk editing system on top of the existing one, accessed through a "regex mode" button in the edit menu.

Some base rules of how this should work:

  • On a song being played, web scrobbler first looks for a normal edit. If it finds one, it applies that, and stops there. No bulk edit will be applied, even if there is a match before or after applying the edit.
  • If there is no edit for the particular song, web scrobbler starts to iterate over bulk edits, starting with the most recently made edit by the user, and applies only the most recent bulk edit that matches. Applying only the newest is a nice way for the user to be able to apply "overrides". So if you apply a blanket edit to delete starting from the first comma to only scrobble the first artist, you can easily apply a new bulk edit that affirms an artist that has comma in their name.
  • A bulk edit is stored as an object with both a search and a replace property, both of which have all four track fields (track, artist, album, albumartist), which can be strings or null.
  • Determining if a bulk edit should be applied is done by going through all the fields of the search property and testing against the respective song field. Null fields are automatically true, while non-null fields must match the entire song field (so the actual applied regex is /^{searchField}$/). All four fields must be true for the bulk edit to be applied.
  • When a bulk edit is applied, it takes each song field where both search and replace are not null, and applies a replace using /^{searchField}$/ to the replace field. Capture groups work.
  • After first bulk edit is applied, web scrobbler searches for metadata, then applies bulk edit again, as there may have been a new album field added because of the metadata fetching. It’s a bit magic but it’s the best way I see.

The rules of this are admittedly a bit magic, but from my experience it is the ruleset that feels the most right to use. We might need to add an explainer somewhere in the extension though.

Adds some more locale stuff. Some of it is trivial for us to add ourselves based on existing properties, but most isn't.

Closes #2588 Closes #3432 Closes #2948 Closes #2943 Closes #2608 Closes #1552 Closes #3069 Closes #3046 Closes #1643 Closes #2157

Also adds two default filters that remove - EP and - Single at the end of album fields, applied by apple music (which is also commonly fetched for other connectors when no album can be fetched from the website itself).

Hence also closes #3186

Any more issues this closes?

I think this PR needs a bit more love before being pushed so setting it to draft for now, but it's pretty close.

@yayuyokitano yayuyokitano added core This issue or pull request is related to the extension core new-feature For PRs that add new functionality major-change For major changes labels Apr 23, 2023
@yayuyokitano
Copy link
Member Author

yayuyokitano commented Apr 29, 2023

Updated the editing system to now edit each field once.
This is untested for now, and I need to make some more adjustments.

The new system means that any number of edits can be applied before/after the metadata fetching, but once a field has been modified by a regex edit, the same field cannot be modified again (the same field can be the subject of multiple searches, but not replaces)

This limitation carries over between the regex application before and after metadata fetching.

I think this behavior should feel a lot more consistent.

EDIT: It still goes from recent to oldest regex edit.

@yayuyokitano
Copy link
Member Author

Fixed up casing stuff i shouldve done in the first place.

Also changed up the previews to now show information prior to doing any regex edits instead of showing warning.
Edits from regular edits still show.
Any album that was fetched from the regex edited information will also still display.

@yayuyokitano
Copy link
Member Author

Note that this commit was breaking, and as of this commit the branch is incompatible with the previous commit of the branch. Remove and readd extension to browser to fix. This issue will not occur between this branch and V2, it should smoothly migrate.

@inverse
Copy link
Member

inverse commented Apr 29, 2023

Not sure I am doing it properly but for example if I go to https://youngspice.bandcamp.com/album/basic-couch-formula

Play a song and edit

in artist field replace left Young Spice with Spice - just for illustration.

It doesn't seem to change anything for me :/ like still plays correctly. Am I doing something wrong?

@yayuyokitano
Copy link
Member Author

Not sure I am doing it properly but for example if I go to https://youngspice.bandcamp.com/album/basic-couch-formula

Play a song and edit

in artist field replace left Young Spice with Spice - just for illustration.

It doesn't seem to change anything for me :/ like still plays correctly. Am I doing something wrong?

Found the issue, it's caused by an error from the track not existing in lastfm. I think maybe quite a lot of the pipeline actually breaks because of this, and it was only noticeable now because of the regex edits. Let me have a look into it.

@yayuyokitano
Copy link
Member Author

My suspicion was correct, all this time the normalize pipeline has literally not been running at all 🤦🏼‍♀️
Pretty sure this should be an issue in v2 too.
Let me try to patchwork this.

@yayuyokitano
Copy link
Member Author

yayuyokitano commented Apr 29, 2023

Alright, added a new pipeline step at the very top that copies parsed data into processed before starting.

Some steps rely on processed data being there even if it is the same as parsed, including the regex edits.

I don't see anything in the code that should break from this change - with that said I don't guarantee anything, this should be viewed pretty critically as it's a relatively significant change to how the pipeline operates.

Anyway the bug you described should be fixed now (and the normalize pipeline should actually work now! On the negative side that might cause some differences in scrobbled tracks for users between v2 and v3, but it would be v3 working correctly)

This PR should now be considered breaking compared to v2, though other than normalization potentially messing things up a bit I don't think it should be noticeable for users.

EDIT: needless to say that last commit should be scrutinized extra thoroughly.

@yayuyokitano
Copy link
Member Author

Added tests now.

This uses the node fetch API, meaning required node version is bumped to node 18. It's LTS so I think it's fine.

Now that there are tests. I think I'm ok with undrafting this, though more tests might be needed.

@yayuyokitano yayuyokitano marked this pull request as ready for review April 30, 2023 09:55
@inverse
Copy link
Member

inverse commented Apr 30, 2023

Nice improvements! It's def working for me now.

However I noticed this behaviour.

Play music and edit regex rule -> all works

How does one remove this rule? I tried changing it to match and replace with $1 which the preview it works but it seems to only scrobble the first change?

Perhaps do you think we need some UI to see regex rules to remove them? like with edited tracks (just noticed we can't remove them either here too)

@yayuyokitano
Copy link
Member Author

Nice improvements! It's def working for me now.

However I noticed this behaviour.

Play music and edit regex rule -> all works

How does one remove this rule? I tried changing it to match and replace with $1 which the preview it works but it seems to only scrobble the first change?

Perhaps do you think we need some UI to see regex rules to remove them? like with edited tracks (just noticed we can't remove them either here too)

The way it currently works is that there are four flags for each of the four fields indicating if they have been edited by a regex edit. Once a regex edit is applied to a field (So that an actual replace operation occurs, this would include just a $1 replace), that field can no longer be modified by any other regex edit. This allows you to use regex edits to override other regex edits (such as having a regex edit changing last comma to ampersand, and then having a separate rule telling it to be ignored for a specific artist with comma in the name). The most recently added regex edit will be applied first.

The full list of regex edits can be viewed and deleted in options menu, just like normal song edits. Should I add a link to the popup linking to the section of options? I don't see an elegant way of adding regex edit deletion within the popup itself.

@inverse
Copy link
Member

inverse commented Apr 30, 2023

The full list of regex edits can be viewed and deleted in options menu, just like normal song edits. Should I add a link to the popup linking to the section of options? I don't see an elegant way of adding regex edit deletion within the popup itself.

How did I miss that UI section in options 🙈 i think it's clear enough for now 👍

@yayuyokitano
Copy link
Member Author

The full list of regex edits can be viewed and deleted in options menu, just like normal song edits. Should I add a link to the popup linking to the section of options? I don't see an elegant way of adding regex edit deletion within the popup itself.

How did I miss that UI section in options 🙈 i think it's clear enough for now 👍

I think it will become clearer once there is a separate entry in the sidebar for edits, which I want to do.

I want this to be merged first though in the interest of trying to keep the PR at least somewhat remotely atomic.

try {
await RegexEdits.loadSongInfo(song);
} catch (e) {
// Do nothing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log debug here to make debugging easier if something really went wrong?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. To be honest I dont know how we would even get an error here.

But this pipeline is directly copied from pipeline/user-input, as the user-input function works basically the same due to regex edit class being directly based off the user edit model.

I assume the try catch is there for some reason, but there was no comment in the original telling me why, so I mindlessly copy pasted.

We could add some logging for sure, it shouldnt spam the user.

I’ll merge this as is for now and I can make a new PR adding some logging later, as due to the size of the changes on this PR this not being merged is blocking several future modifications needed before release.

Copy link
Member

@inverse inverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside the comments seems reasonable 👍

@yayuyokitano yayuyokitano merged commit 00d1ffc into web-scrobbler:master May 3, 2023
8 checks passed
@RAM237
Copy link

RAM237 commented May 20, 2023

Hey, awesome news! Which version it is supposed to be released with?
As I understand it is not yet in 2.89.0

@yayuyokitano
Copy link
Member Author

It is scheduled for release in version 3.0.0, we’re working on getting it out now

@maxelo
Copy link

maxelo commented Jun 6, 2023

love the new web scrobbler design, thanks for your work
I was very excited for adding regex rules
But I can't get it to work
Can anyone help me?

help

search: "(\s?(con\s)"

replace "(feat. "

example:

https://regex101.com/r/wEwpHH

@yayuyokitano
Copy link
Member Author

love the new web scrobbler design, thanks for your work I was very excited for adding regex rules But I can't get it to work Can anyone help me?

help

search: "(\s?(con\s)"

replace "(feat. "

example:

https://regex101.com/r/wEwpHH

Currently, regex demands full text matches. I plan to add some options that will allow you to tweak this behavior a bit and help make the behavior more clear to the user in the future.
So in this case, you might want something along the lines of search: (.*)\(con\s(.*) replace: $1(feat. $2

@maxelo
Copy link

maxelo commented Jun 7, 2023

love the new web scrobbler design, thanks for your work I was very excited for adding regex rules But I can't get it to work Can anyone help me?
help
search: "(\s?(con\s)"
replace "(feat. "
example:
https://regex101.com/r/wEwpHH

Currently, regex demands full text matches. I plan to add some options that will allow you to tweak this behavior a bit and help make the behavior more clear to the user in the future. So in this case, you might want something along the lines of search: (.*)\(con\s(.*) replace: $1(feat. $2

it works =)

hope you can put this rules in the future, thanks for your work!

https://github.com/web-scrobbler/web-scrobbler/issues/3069#issuecomment-1013927834

@maxelo
Copy link

maxelo commented Jun 7, 2023

I managed to improve the regex rule if you want to add it in the future:

https://regex101.com/r/rz997G/1

Regular Expression
(.*)\(con\s(.*)()()(.*)\sy\s(.*)
Substitution
$1(feat. $2 and $6

Example:
Get Lucky (con Pharrell Williams y Nile Rodgers)
Into:
Get Lucky (feat. Pharrell Williams and Nile Rodgers)

thanks for your work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment