Replace `Track.artist` and `Track.extraArtists` with `Track.artists` #1681

Lucki · 2024-08-21T12:46:45Z

Ref: https://discord.com/channels/647373417091170314/1275405997736726609
Fix #1668

nukeop · 2024-08-22T16:37:02Z

Thanks for contributing this. Is it ready for review?

Lucki · 2024-08-22T17:52:47Z

Yes. Please let me know when you find something non-functional.

Edit: ~~Found one - the MPRIS thing is showing unknown artist~~
Edit2: ~~Another one - A discogs track without an artist but with extraartists, wanna use the album artist when no specific track artist is available.~~
Edit3: ~~LastFM track search needs adjustments converting to internal~~

nukeop · 2024-08-25T23:08:38Z

packages/main/src/services/@linux/system-api.ts

+      // @ts-expect-error NuclearMeta is not a descendant of Track, but Track.artists should be available here anyway
+      'xesam:artist': track.artists ?? [track.artist]


NuclearMeta should be updated the same way. I'm afraid this is also going to need that:

Scanned local library tracks will need to be migrated on first run

Scanner will need to return data in this new format

I adjusted the obvious bits, although blind (no manual testing).

As written in the Discord thread and as I have no usage experience with that part of the program, I think it's better you take this part.

Line 203 should be updated to artists as well, otherwise it crashes on start.

nukeop · 2024-08-25T23:13:04Z

packages/app/app/actions/downloads.ts

+function getDownloadsBackwardsCompatible(): Download[] {
+  const downloads: Download[] = store.get('downloads');
+
+  downloads.forEach(download => {
+    // @ts-expect-error For backwards compatibility we're trying to parse an invalid field
+    if (download.track.artists || !download.track.artist) {
+      return;
+    }
+
+    // @ts-expect-error For backwards compatibility we're trying to parse an invalid field
+    if (download.track.artist) {
+      // @ts-expect-error For backwards compatibility we're trying to parse an invalid field
+      download.track.artists = _.isString(download.track.artist) ? [download.track.artist] : [download.track.artist.name];
+    }
+
+    // Assuming we have `extraArtists` on a track, we must had an `artist` which
+    // was already saved into `artists`, so this `track.artists` shouldn't be undefined
+    // @ts-expect-error For backwards compatibility we're trying to parse an invalid field
+    download.track.extraArtists?.forEach(artist => {
+      download.track.artists.push(artist);
+    });
+  });
+
+  return downloads;
+}


This function does the exact same thing as the one in playlists.ts and almost the same as the one in favorites.ts. The common parts of this logic should be extracted and can live in e.g. packages/app/app/actions/helpers.ts.

Not quite. All three are slightly different:

Download[]: { track: Track }[] Playlist[]: { tracks: Track[] }[] Favorites: { tracks: Track[] }

I tried combining all into a generic function but wasn't successful with it.

Just for laughs

export interface TrackMember { track: Track; } export interface TracksMember { tracks: Track[]; } export interface Favorites { albums: Album[]; artists: Artist[]; tracks: Track[]; } function isTracksMember(config: TrackMember | TracksMember): config is TracksMember { return (config as TracksMember).tracks !== undefined; } function isTrackMember(config: TrackMember | TracksMember): config is TrackMember { return (config as TrackMember).track !== undefined; } function isTracksArrayMember(config: TrackMember | TracksMember | Favorites): config is Favorites { return (config as Favorites).tracks !== undefined && (config as Favorites).tracks instanceof Array; } function rewriteTrackArtists(track: Track) { // @ts-expect-error For backwards compatibility we're trying to parse an invalid field if (track.artists || !track.artist) { // New format already present, do nothing return; } // @ts-expect-error For backwards compatibility we're trying to parse an invalid field track.artists = _.isString(track.artist) ? [track.artist] : [track.artist.name]; // @ts-expect-error For backwards compatibility we're trying to parse an invalid field track.artists = track.artists.concat(track.extraArtists?.map(artist)); } /** +* Helper function to read the old track format into the new format. +* +* `Track.artist` and `Track.extraArtists` are written into {@link Track.artists} +*/ export function getFromStoreBackwardsCompatible<T extends Favorites | TrackMember[] | TracksMember[]>(configName: 'downloads' | 'favorites' | 'playlists'): T { let config: T = store.get(configName); if (config instanceof Array) { // Playlist[] || Download[] config = config.map((member: TrackMember | TracksMember) => { if (isTrackMember(member)) { // Download[] rewriteTrackArtists(member.track); } else if (isTracksMember(member)) { // Playlist[] member.tracks.forEach(track => { rewriteTrackArtists(track); }); } return member; }); } else if (isTracksArrayMember(config)) { // Favorites: { tracks: Track[] } config.tracks = config.tracks.map(track => { rewriteTrackArtists(track); return track; }); config.albums = config.albums.map(album => { album.tracklist = album.tracklist?.map(track => { rewriteTrackArtists(track); return track; }); return album; }); } return config; }

nukeop · 2024-08-25T23:15:01Z

packages/app/app/actions/favorites.ts

+    album.tracklist?.forEach(track => {
+      if (track.artists || !track.artist) {
+        return;
+      }
+
+      if (track.artist) {
+        track.artists = _.isString(track.artist) ? [track.artist] : [track.artist.name];
+      }
+
+      // Assuming we have `extraArtists` on a track, we must had an `artist` which
+      // was already saved into `artists`, so this `track.artists` shouldn't be undefined
+      track.extraArtists?.forEach(artist => {
+        track.artists.push(artist);
+      });
+    });
+  });
+


This part can use the function that can be extracted from all getXBackwardsCompatible.

As mentioned in the Discord thread, this is very strange.
I moved it in a separate helper function and then the tests start failing because the track is suddenly undefined.

nukeop · 2024-08-25T23:15:56Z

packages/app/app/actions/downloads.ts

+    download.track.extraArtists?.forEach(artist => {
+      download.track.artists.push(artist);
+    });


Instead of mutating by using forEach/push, you can use map and assign what map returns.

This also applies to other places where forEach is used.

Changed them all to use map now.
Aside from inside the helper functions there was only one single occurrence when reading from the DB.

nukeop · 2024-08-28T17:54:27Z

I'm still working on the track table refactor, after that I'll get back to this.

nukeop · 2024-10-15T22:58:32Z

Finally got some time (and willpower) to get this into a working shape! I'll do it and merge it asap.

nukeop · 2024-10-21T00:27:15Z

After these changes everything should be ready. If the CI passes I'll merge it tomorrow.

GearKite · 2024-10-21T08:49:03Z

This breaks lastfm scrobbling, all song artists are Undefined now.

nukeop · 2024-10-21T08:54:42Z

Thanks for testing. I'll take a look at this (once I'm back from 1955).

nukeop · 2024-10-21T23:22:43Z

Bad news. This is going to need replacing several JS files with TS, they also need to be typed correctly. These changes also break autoradio in addition to scrobbling. I'm currently working on this. The most troublesome component to update is going to be the SoundContainer which controls playback, and is in a catastrophically bad state due to being neglected over the years.

nukeop · 2024-10-28T00:55:05Z

Ok, I managed to fix this.

nukeop · 2024-10-29T23:55:00Z

There's still one more problem, the local library tracks still have a single artist field, and I'm not sure what to do about them. I think a backwards compatibility layer is needed like in all those other cases. Until that's added, you can't play anything from the library.

GearKite · 2024-10-30T05:45:34Z

Queue playback doesn't seem to really work now. It seems that the YouTube playback source expires after a few minutes, so, after listening to a song, all the next ones will show a:
The audio playback can not be loaded, either because the server or network failed or because the format is not supported.
Notably, this does not happen on an older version.

waldo121 · 2024-10-30T15:02:11Z

Queue playback doesn't seem to really work now. It seems that the YouTube playback source expires after a few minutes, so, after listening to a song, all the next ones will show a: The audio playback can not be loaded, either because the server or network failed or because the format is not supported. Notably, this does not happen on an older version.

Can you create an issue with more details please? I have also observed this problem and it is not related to this pr.

nukeop · 2024-10-30T16:57:30Z

I am pretty sure ytdl-core needs to be updated: https://www.npmjs.com/package/@distube/ytdl-core

Youtube changed something a couple of days ago.

nukeop · 2024-11-12T23:46:30Z

I'm not giving up yet, working on a one time migration for existing libraries that will hopefully solve this.

Lucki force-pushed the fix/1668 branch from 32c865b to bcfc355 Compare August 22, 2024 14:10

Lucki force-pushed the fix/1668 branch from 506af29 to b31cedd Compare August 22, 2024 17:50

nukeop requested changes Aug 25, 2024

View reviewed changes

nukeop added the needs changes The author needs to make changes to this pull request. label Aug 25, 2024

Lucki added 10 commits August 27, 2024 02:05

Replace Track.artist and Track.extraArtists with Track.artists

5923449

Read autoradio artist into new artists array

e7b2e90

Copy old format history DB entries to new array

dfca722

[Spotify] Adapt to new artists

bac2ba3

[MusicBrainz] Adapt to new artists

5075fe7

[Discogs] Adapt to new artists

ab6e0c8

Display artists in TrackRow when deviating from album artist

ddf6d61

Adjust reading the config into the new format

6479d6f

[WIP] fixup! Adjust reading the config into the new format

ca09d7c

[WIP] Adjust NuclearMeta

129ee02

Lucki force-pushed the fix/1668 branch from 676d177 to 129ee02 Compare August 27, 2024 00:06

nukeop added under review This pull request is being reviewed by maintainers. and removed needs changes The author needs to make changes to this pull request. labels Aug 28, 2024

nukeop added 2 commits October 16, 2024 00:47

Reset xesam:artist to an array

0ecdce9

Merge branch 'master' into fix/1668

a67de60

nukeop added 7 commits October 16, 2024 02:15

Adapt GridTrackTable to multiple artists per track

a9906b3

Avoid mutation when converting a playlist to the new format

2a4e2c8

Avoid mutation when remapping downloads and favorites

8efdb52

Use new artist format in GridTrackTable tests

6a214c5

Use new metadata format in linux system api (mpris)

dd1fd00

Favorites multi-artist support, snapshot update

37a607a

Fix add to favorites action

97d2488

Update grid track table snapshots

84b7216

Rewrite SoundContainer in Typescript

d558aee

nukeop added 3 commits October 29, 2024 23:35

Update last.fm action types

4dcbd58

Replace failing playlist

e2805bb

Merge branch 'master' of github.com:nukeop/nuclear into fix/1668

3580b36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `Track.artist` and `Track.extraArtists` with `Track.artists` #1681

Replace `Track.artist` and `Track.extraArtists` with `Track.artists` #1681

Lucki commented Aug 21, 2024

nukeop commented Aug 22, 2024

Lucki commented Aug 22, 2024 •

edited

Loading

nukeop Aug 25, 2024

Lucki Aug 27, 2024

GearKite Oct 5, 2024

nukeop Aug 25, 2024

Lucki Aug 27, 2024

nukeop Aug 25, 2024

Lucki Aug 27, 2024

nukeop Aug 25, 2024

nukeop Aug 25, 2024

Lucki Aug 27, 2024

nukeop commented Aug 28, 2024

nukeop commented Oct 15, 2024 •

edited

Loading

nukeop commented Oct 21, 2024

GearKite commented Oct 21, 2024

nukeop commented Oct 21, 2024

nukeop commented Oct 21, 2024

nukeop commented Oct 28, 2024

nukeop commented Oct 29, 2024

GearKite commented Oct 30, 2024

waldo121 commented Oct 30, 2024

nukeop commented Oct 30, 2024

nukeop commented Nov 12, 2024

		// @ts-expect-error NuclearMeta is not a descendant of Track, but Track.artists should be available here anyway
		'xesam:artist': track.artists ?? [track.artist]

Replace Track.artist and Track.extraArtists with Track.artists #1681

Are you sure you want to change the base?

Replace Track.artist and Track.extraArtists with Track.artists #1681

Conversation

Lucki commented Aug 21, 2024

nukeop commented Aug 22, 2024

Lucki commented Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nukeop commented Aug 28, 2024

nukeop commented Oct 15, 2024 • edited Loading

nukeop commented Oct 21, 2024

GearKite commented Oct 21, 2024

nukeop commented Oct 21, 2024

nukeop commented Oct 21, 2024

nukeop commented Oct 28, 2024

nukeop commented Oct 29, 2024

GearKite commented Oct 30, 2024

waldo121 commented Oct 30, 2024

nukeop commented Oct 30, 2024

nukeop commented Nov 12, 2024

Replace `Track.artist` and `Track.extraArtists` with `Track.artists` #1681

Replace `Track.artist` and `Track.extraArtists` with `Track.artists` #1681

Lucki commented Aug 22, 2024 •

edited

Loading

nukeop commented Oct 15, 2024 •

edited

Loading