LB-1281: Link all listens from the same album #2956

MonkeyDo · 2024-08-07T13:17:23Z

The "Missing data" page that shows a user their unmatched listens has been somewhat useful, but linking each track from an album separately is a nightmare, especially for big various artist compilations for example.

This PR does two things to solve this:

Group listens by their shared release_name property, and display them in an accordion component
Add a new multi-track mapping modal allowing you to search or paste in a release or RG, and a fuzzy search-based automatic detection to match listens to the right recording

I threw these bits of helptext together, would appreciate some feedback and/or rewriting:

For the "include artist name" tickbox:

Follow-up tasks

Use Fuse.js fuzzy search library instead of fuzzysort for the work done on Improve Apple Music search with fuzzy matching #2905
~~Ask aerozol's forgiveness for ignoring his mockups and suggested UX in LB-1281~~

MonkeyDo · 2024-08-08T15:47:30Z

The failing server tests are unrelated, and already fixed by #2951

To show unmatched listens grouped by release name

based on common release_name attribute

match all listens we can based on the tracklist of a selected release, using FuseJS to do fuzzy matching . Submission not working yet

default to closed, but add a boolean prop to default to open. Fix arrow indicator layout

in the settings/missing-data folder, which makes more sense

Put the button first for a cleaner look, and also reduce its size to fit in the header better

Since I rebased (Class -> functional component) I need to change a few things. + a bit of cleanup

+ clean up component

while writing tests, ran into a couple of issues, one with the import style for React (import React vs. import * as React). Also improved accessibility while writing tests

Added these items for extra testing, forgot to commit in previous commit...

Aerozol · 2024-08-09T00:47:54Z

LOVE IT!!
I have nitty-gritty feedback, but overall this is already A+
I'm not married to any of these suggestions, and some may not work in practice. I hope you like mspaint pics!

1. Swap the link and "expand release" buttons
Having the dropdown next to the album name makes it a bit clearer what's being expanded, but mainly this will match up with where the link button lives for individual tracks as well (I got a bit confused)

2. Clicking the link button should not expand the release
It's weird to have a popup + something happening behind the popup. It stays expanded when you close the popup, and when you click the link button a second time it minimizes it behind the popup again. Probably simpler and expected to just let the expand button do the expanding.

3. Release row formatting
I like the track count in the link button, it makes sense, but I wonder if putting it next to the release title would allow you to put something like (11 tracks), which helps communicate to the user that the drop down is a release/collection of listens.
Then you could also use the orange link icon (without the square around it) that's in the track row, which is cleaner looking.
Are we using that panel-title class elsewhere? It's a little tricky to see/thin at that size, could be stronger imo.

4. New blurb text
I don't think the blurb at the top needs any of the new information re. "tracks are grouped by album..." - if anyone disagrees, maybe in one of those (i) rollovers? This is a case where the UI is pretty basic and the page performs a clear function (the process becomes confusing if someone has to add something to MB, but that is a whole other kettle of fish), having a bunch of text at the top makes it more confusing and complicated feeling than it actually is. Have faith in your lovely UI monkey!

5. Old blurb text/suggestion
Is "This page shows your top 200 submitted tracks..." still accurate? If so, it might get a bit confusing when a user thinks they have matched all the tracks from an album, but they have only matched the tracks that were in their top 200. But maybe that's a problem for another day.
While I'm here, this would be my suggestion for a blurb edit:

Your top 200 listens that haven't been automatically linked. Link the listens below, or to submit new data to MusicBrainz.

MusicBrainz is the open-source music encyclopedia that ListenBrainz uses to display information about your music.

If we want to keep a link like "Add missing data to MusicBrainz" I think it would better fit in the blurb in the following note 6. where the user will be facing the issue to be solved (not finding a match in the search box).

6. Link popup title
"Link these Listens with MusicBrainz" could be simplified to "Link listens" imo.
If someone non-tech doesn't know what MusicBrainz is, they don't need to worry about it until they want to/need to add data, they don't have to be told up top what's happening in the background.
Also applies to matching single listen/song

7. Link popup blurb
Again, the UI is pretty good, but the text makes it look like we expect users to struggle/implies that it is confusing. Can we move all of the text at the top underneath the 'add mapping' button, maybe all in a light grey and in small text?
"Include Artist name when matching" doesn't have to be bold imo - it's the strongest thing on the page. There is a bit of spacing issue with that text and the checkbox next to it, maybe could use some extra above as well.

Again, while I'm at it I would shorten the top blurb to (particularly if you want to leave it at the top):

Search by album/artist name or paste a MusicBrainz URL or MBID.

Also applies to matching single listen/song

8. Link popup: Listens to match section
Change "match" to "link"?

9. Link popup: Add mapping button
Do we use the word "mapping" anywhere else? Can we change it to "link listens" (or similar)?
Also applies to matching single listen/song

10. Mobile
Looks good.

Sorry for the long list, hope it's helpful!

anshg1214

The PR looks good overall. A few changes.

frontend/js/tests/user/missing-data/MissingMBData.test.tsx

frontend/js/src/settings/missing-data/MissingMBData.tsx

frontend/js/src/settings/missing-data/MultiTrackMBIDMappingModal.tsx

Separate the button from the rest of the title to allow clicking the action button without collapsing/opening the accordion

also remove the superfluous explanation text I previously added

lazybookwyrm · 2024-08-09T20:25:21Z

When you click the link button, the search field autofills with "album artist". Changing that to "album - artist" is giving me better search results on some releases.

kellnerd · 2024-08-12T17:12:23Z

For what it's worth, grouping the words of the artist name gives even better results:
Play artist:(Great Big Sea) for example returns less releases which match the terms "Big" and "Sea".
You may also want to look into escaping special characters.

As suggested by kellnerd in #2956 (comment)

MonkeyDo · 2024-08-13T10:26:22Z

For what it's worth, grouping the words of the artist name gives even better results: Play artist:(Great Big Sea) for example returns less releases which match the terms "Big" and "Sea". You may also want to look into escaping special characters.

Thanks for the tip! I did add the grouping.
As for escaping special lucene characters, I spent a bit of time to regexp but realized the input is most likely sanitized and escaped on the MB side, which makes sense, because I can search even using only special characters:

anshg1214

The PR looks good. Just some final changes.

frontend/js/src/settings/missing-data/MissingMBData.tsx

frontend/js/src/settings/missing-data/MultiTrackMBIDMappingModal.tsx

frontend/js/src/settings/missing-data/MissingMBData.tsx

MonkeyDo · 2024-08-13T12:21:01Z

Thanks for the tip! I did add the grouping. As for escaping special lucene characters, I spent a bit of time to regexp but realized the input is most likely sanitized and escaped on the MB side, which makes sense, because I can search even using only special characters:

Meh, actually for special-punctuation-only bands, we would need to escape the characters to get a good result:
artist:(\!\!\!) correctly returns albums by the band !!! while artist:(!!!) returns zero results by itself.

I feel that we need to make a decision, but none are optimal considering the vague data we are working with (sometimes the metadata is good, sometimes trash).
Escaping special characters makes the search input a lot less readable/modifyable, and admitedly cases such as !!! are a rarity.

All right then, I guess we need another tickbox option to escape the search input!

Allow searching with and without the artist name, and also escaping special characters from the Lucene search string (allows matching special cases)

We don't need to react to the 'page' searchparam anymore, and instead we load all of the data in one go, then parse it for internal pagination. Decoupling loader data and search params means we don't re-fetch data pueposelessly when "changing page"

pass only the function we need from the APIService rather than the whole thing.

Remove style from list of tracks, but make the toasts themselves clear

Expected a number, but it's a string representation

Aerozol · 2024-08-21T05:41:16Z

Gave this another test spin today, to link up some listens and have some more minor notes:

Can't link listens when the search bar is empty, even when the matches are already found
The search bar gives no indication when it is searching/working, meaning that when you are adjusting text or typing into the search field it can be unclear whether it is still loading results, or there are no improved results to show.

Change from form+onSubmit to button+onClick, because the search inpur has a required attribute preventing the form submission

MonkeyDo · 2024-08-21T15:18:43Z

Freshly deployed to test.LB

OK, going to stop work here for this PR, before I continue adding more features :D

It works really well from my testing, and I have implemented all the feedback as far as I know.

Thanks everyone for your help in testing and for your suggestions !

Using the wrong variable after a copy-paste

anshg1214

The PR looks much better. Just two final change change and we're good to go 🚀

anshg1214 · 2024-08-22T14:59:54Z

frontend/js/src/settings/missing-data/MultiTrackMBIDMappingModal.tsx

+    const closeModal = React.useCallback(() => {
+      modal.hide();
+      document?.body?.classList?.remove("modal-open");
+      // Need to manually remove the modal backdrop
+      const backdrop = document?.body?.getElementsByClassName(
+        "modal-backdrop"
+      )?.[0];
+      backdrop?.parentElement?.removeChild(backdrop);
+      setTimeout(modal.remove, 200);
+    }, [modal]);


Right now if you close the modal by clicking on the backdrop and open another one, the states are not reset. I can still see the matches from the previous album. So we probably need to reset the states here.

I don't think I can fix that issue unfortunately.
Basically, I've been putting off replacing Bootstrap4 that we have with react-bootstrap, and replace all the modals to work better with nice-modal-react.
Clicking on the backdrop does trigger the bootstrap JS code to hide the modal, but does not trigger any of the code in the modal component so i can't run code to cleanup.
Potential solutions are very hacky and ugly, and I'd rather not.

I tried listening to the 'hidden' event as described in https://getbootstrap.com/docs/4.0/components/modal/#events but to no avail.

Let's live with this failure mode until we use react-bootstrap.

Makes sense. Cool! We'll deploy this for now, and revisit it later.

Yeah, another big refactoring job :)

But at least we'll be able to get rid of jQuery that we still load for those stupid Bootstrap modals and tooltips and whatnot

frontend/js/src/settings/missing-data/MultiTrackMBIDMappingModal.tsx

If closing modal with clicking on backdrop, modal is not currently reset and get re-opened with old data

MonkeyDo added 17 commits August 8, 2024 17:49

Create reusable Accordion component

adb648f

To show unmatched listens grouped by release name

Group and display unmatched listens by group

ce92917

based on common release_name attribute

Create multi-listen MBID mapping modal

a6ae755

match all listens we can based on the tracklist of a selected release, using FuseJS to do fuzzy matching . Submission not working yet

Improve multi-listen mapping modal

65aa6ac

Small UI fixes for MBIDMapping page

bb4a55a

Misc. accordion component improvements

b7224ba

default to closed, but add a boolean prop to default to open. Fix arrow indicator layout

Move MultiTrackMBIDMappingModal

86ce4f2

in the settings/missing-data folder, which makes more sense

Missing Data page: fix navigation +layout

8da8a45

MissingData: Move button first in accoridon header

c4f7123

Put the button first for a cleaner look, and also reduce its size to fit in the header better

MultiTrackMappingModal: re-add modal backdrop

4499b48

MultiTrackMappingModal: Fix backdrop

d749961

MissingData: Fix pagination using react-router params

58d45ef

Since I rebased (Class -> functional component) I need to change a few things. + a bit of cleanup

Missing Data: remove items from page once linked

c1b3799

+ clean up component

MissingData: add missing keys for React

1a7dadc

Lint

52d79a1

Fix MissingMBData tests + accordion accessibility

b19d79e

while writing tests, ran into a couple of issues, one with the import style for React (import React vs. import * as React). Also improved accessibility while writing tests

Test fixtures

5bfaf77

Added these items for extra testing, forgot to commit in previous commit...

MonkeyDo force-pushed the rebased-multi-track-mbid-mapping branch from d5d13ba to 5bfaf77 Compare August 8, 2024 15:49

MonkeyDo requested a review from anshg1214 August 8, 2024 15:51

anshg1214 requested changes Aug 9, 2024

View reviewed changes

MonkeyDo added 7 commits August 9, 2024 17:13

MissingData: Fix accordion component styling

4742306

MissingData: Fix accordion titles/buttons

7237c3c

Separate the button from the rest of the title to allow clicking the action button without collapsing/opening the accordion

Spark: Increase limit of top tracks for missing data > 1000

ea07f5b

MissingData: Add last updated date if available

da43479

also remove the superfluous explanation text I previously added

MissingData: Tweak help texts

62f61a2

MissingData: cleanup

795bf15

MissingData: Remove deleted listens from ambient queue

2861560

MonkeyDo requested a review from anshg1214 August 12, 2024 16:26

anshg1214 and others added 3 commits August 12, 2024 23:23

Merge branch 'brainzplayer-spa' into rebased-multi-track-mbid-mapping

1f651cb

MissingData: Group artist search terms

7cc6efc

As suggested by kellnerd in #2956 (comment)

refactor: cleanup and format

c97c5a4

anshg1214 requested changes Aug 13, 2024

View reviewed changes

MonkeyDo added 6 commits August 13, 2024 14:59

MissingData: Add more advanced search options

2336987

Allow searching with and without the artist name, and also escaping special characters from the Lucene search string (allows matching special cases)

MissingData: Reduce dependencies

a0d9913

pass only the function we need from the APIService rather than the whole thing.

MissingData: Colorful background for success and error toasts

da268fc

Remove style from list of tracks, but make the toasts themselves clear

MissingData: Fix last_updated type

85b50c0

Expected a number, but it's a string representation

MissingData: Fix use of undefined recording_msid

af354b6

Base automatically changed from brainzplayer-spa to master August 13, 2024 15:15

MonkeyDo added 2 commits August 19, 2024 18:44

Cleanup

f2bb711

Merge branch 'master' into rebased-multi-track-mbid-mapping

31077a9

MonkeyDo added 4 commits August 21, 2024 14:50

Allow submitting mapping if search input empty

8d0fc59

Change from form+onSubmit to button+onClick, because the search inpur has a required attribute preventing the form submission

MissingMBData: Fix tests

b5e0de1

Add loader indicator in album and track search components

109c307

Formatting

3ee6570

MonkeyDo requested a review from anshg1214 August 21, 2024 15:17

Fix bug in success message

6147398

Using the wrong variable after a copy-paste

anshg1214 requested changes Aug 22, 2024

View reviewed changes

MonkeyDo added 2 commits August 22, 2024 18:25

Resert MultiMapping modal on close

6a96ae7

If closing modal with clicking on backdrop, modal is not currently reset and get re-opened with old data

Missing space in help text

4447809

MonkeyDo merged commit fce8bea into master Aug 22, 2024
4 checks passed

MonkeyDo deleted the rebased-multi-track-mbid-mapping branch August 22, 2024 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LB-1281: Link all listens from the same album #2956

LB-1281: Link all listens from the same album #2956

MonkeyDo commented Aug 7, 2024 •

edited

Loading

MonkeyDo commented Aug 8, 2024

Aerozol commented Aug 9, 2024 •

edited

Loading

anshg1214 left a comment

lazybookwyrm commented Aug 9, 2024

kellnerd commented Aug 12, 2024

MonkeyDo commented Aug 13, 2024 •

edited

Loading

anshg1214 left a comment

MonkeyDo commented Aug 13, 2024 •

edited

Loading

Aerozol commented Aug 21, 2024

MonkeyDo commented Aug 21, 2024

anshg1214 left a comment

anshg1214 Aug 22, 2024

MonkeyDo Aug 22, 2024 •

edited

Loading

anshg1214 Aug 22, 2024

MonkeyDo Aug 22, 2024

LB-1281: Link all listens from the same album #2956

LB-1281: Link all listens from the same album #2956

Conversation

MonkeyDo commented Aug 7, 2024 • edited Loading

Follow-up tasks

MonkeyDo commented Aug 8, 2024

Aerozol commented Aug 9, 2024 • edited Loading

anshg1214 left a comment

Choose a reason for hiding this comment

lazybookwyrm commented Aug 9, 2024

kellnerd commented Aug 12, 2024

MonkeyDo commented Aug 13, 2024 • edited Loading

anshg1214 left a comment

Choose a reason for hiding this comment

MonkeyDo commented Aug 13, 2024 • edited Loading

Aerozol commented Aug 21, 2024

MonkeyDo commented Aug 21, 2024

anshg1214 left a comment

Choose a reason for hiding this comment

anshg1214 Aug 22, 2024

Choose a reason for hiding this comment

MonkeyDo Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

anshg1214 Aug 22, 2024

Choose a reason for hiding this comment

MonkeyDo Aug 22, 2024

Choose a reason for hiding this comment

MonkeyDo commented Aug 7, 2024 •

edited

Loading

Aerozol commented Aug 9, 2024 •

edited

Loading

MonkeyDo commented Aug 13, 2024 •

edited

Loading

MonkeyDo commented Aug 13, 2024 •

edited

Loading

MonkeyDo Aug 22, 2024 •

edited

Loading