[Discussion] Absolute Unit Refactor

[xxxserxxx]

The branch xxxserxxx contains a massive refactoring aimed to address a number of fundamental design issues in stmp (inherited by stmps). I intend this issue to be a discussion thread about it.

1. stmp blithely interchanged Subsonic's filesystem-based Directory model with the metadata-based Author/Artist model. With some servers (Navidrome, for example) this was usually not noticable because IDs were fungible between the two models. With other servers (such as gonic) this often caused confusing behaviors when browsing. In all cases, it was possible to get stmp(s) into an odd situation, such as navigating into an album in the Entity list and then repeatedly navigating back up the tree via [..] until the Entity column looked like the Artists column.
2. The browser columns were blatant lies. The Artist column only contained artists if the music directory filesystem strictly followed a layout where artists were at the top, and every artist contained albums, and albums contained only songs. If there was any variation, you could see albums in the Artists column or artists in the Albums column. And the code could never know what, exactly, it was looking at.
3. The code itself followed some bad conventions; in particular, all of the stucts in the subsonic library were named with the convention SubsonicXXXX; this is considered poor Go form, where type and method names are not supposed to repeat the code structure.
4. The subsonic library had a bad habit of not being type safe: every method returned a SubsonicResponse pointer, which could be any one of several subtypes depending on the data from which it was unmarshalled. In Go, this sort of situation is hard to solve and requires either much code and data duplication or type-unsafeness. It is not solvable with generics (most problems generics are designed to address are not addressable with Go generics). Regardless, the library exacerbated this by having all methods, regardless of subtype, return SubsonicResponse structs, leaving type checking to the caller. This was error-prone.
5. Because of the first and second issues, as code based on metadata was changed to add features, bugs related to the lack of consistency between use of Directory and Artist/Album APIs were starting to appear. Navidrome would hide these issues as (e.g.) Artist IDs were also valid Directory IDs, but under other servers (e.g. gonic), this would cause items to suddenly disappear in the UI, and other odd behavior.

Thus, the xxxserxxx branch. The very first commit addressed issues 1-3:

• The subsonic library was refactored to remove all Subsonic prefixes from types; the unnecessarily repetitive subsonic.SubsonicArtist became subsonic.Artist.
• Almost all methods now return more strict types. GetArtist() returns an Artist; GetAlbum() returns an Album.
• BrowserPage now operates entirely on the metadata methods, eschewing where possible the Directory and Index functions. This enforces consistency across the application. Almost all of the rest of the pages and code already used the metadata functions, so the biggest impact was on BrowserPage.

Following commits are of four sorts:

• Because there were many PRs stacking up and a mess of difficult merges, all of my PRs have been merged onto this branch:
  • fix: issue #88 #91
  • adds: ability to toggle song info panel on queue page via i #90 (code provided by @mahmed2000; I just created the branch and submitted the PR)
  • Synced lyrics support #87
  • Feat 52 search by genre #80
• Bug fixes related to the merge; tweaks to make stmps behave consistently whether the server is Navidrome or gonic; and performance enhancements such as the removal of redundant Sort calls.
• Bug fixes from the issue list (this is a sort of subset of the first item, because there are at least two unmerged PRs that only deal with fixing user-reported bugs)
• Merges of other people's PRs, and future enhancements

At the point of commit 3359cc5, changes fall into the first two categories. My next activities on this branch are the following -- the first two items are my first priority; the rest are in no particular order:

• Add a mode where BrowserPage can be used as a directory browser, using Indexes and Directory API calls. These calls are reflections of the filesystem, and operate without looking at metadata; they represent artist/album hierarchy only so far as the users have been consistent in their directory structure. Users who don't put albums under artists will see a completely different view of their data than the metadata view. This view, while silly, is necessary to catch music which might be mis-tagged or missing tags.
• Merges of other people's PRs.
• Continued fixes of reported issues
• Merge the concurrent cover art enhancement
• Add LRU control over albums and cover art, to prevent stmps memory caching from slowly eating all system memory. The code for this exists; I just haven't hooked it into anything yet.
• Performance enhancements; in particular some areas could benefit from binary search instead of the current linear search (e.g. synchronized lyrics)
• Re-add the log-to-disk function that was part of, and then redacted out of, an unrelated PR.
• Support for log levels, to control verbosity
• Add a mocking library and start filling in unit tests
• Factor out the proactive caching of playlist data. Playlists should behave like all other tree data: load the children when the user navigates to it. There's discussion about this decision in Albums start showing up as blank after clicking through other ones #85
• Expand the search/filtering function. / should work in the Entities column, and also in the playlists, search, and genre search pages
• Update screenshots
• Improve the command line arguments; they're poorly documented and kinda ugly. I'd kind of like to replace viper with claptrap, but I'm biased. Changing that would be low priority, but improving the CLI help text is valuable.

I want a way to make managing these merges easier. @spezifisch is often busy for periods of time, and I have spurts of productivity, and it's easy for me to pile up several non-trivial PRs that become overwhelming and hard to merge. I've been using jujutsu on my desktop, and it really does massively improve over git's notoriously terrible merging, and is nearly as easy as Mercurial to work with... but that doesn't address the fundamental procedural issue. My solution, at the moment, is to keep my own "main" -- that's this xxxserxxx branch. I still do not want to hard-fork stmps, so I'm open to suggestions.

BTW, I'd like to try to recruit @mahmed2000, who obviously has a better handle on tview than myself and has already been immensely helpful; having another contributor would be fantastic, and from what I've seen, they'd bring a lot of value.

There's a lot of discussion that includes history and observations in #85.

[xxxserxxx]

After having spent several days writing code, trying API calls, and reading the OpenSubsonic API docs, I've come to the conclusion that it's going to be very difficult to get cohesive, reliable behavior across servers. This is the result of several things I've already mentioned, but I'm going to sum up those with some additional findings into a hopefully reasonably brief summary. I'm going to call the OpenSubsonic API "OSA" for my sanity; any criticisms I may appear to level at OSA must be tempered with the knowledge that they're trying to reverse-engineer around an API that comes with a host of problems.

1. OSA defines getMusicFolders. The returned items have IDs, but in neither Navidrome nor Gonic do these IDs refer to any object that can be fetched by any other API call -- in particular, not getMusicDirectory, getArtist, or getAlbum.
2. OSA does not provide getMusicFolder (singlular)
3. OSA claims that getIndexes returns "a list of authors." In Navidrome, this is mostly true, in that the returned object IDs are indeed artist IDs; in gonic, this is not true, and the IDs returned can be used only with getMusicDirectory.
4. getMusicDirectory could be either an artist, an album, or any other folder in the filesystem

The crux of the issue is that the documentation is ambiguous about the distinction between filesystem and metadata structure, and server implementations vary on how they interpret this. Subsonic originally assumed a directory structure of

musicFolder/
  artist/
    album/
      song.ext

In reality, users could put individual song files under the music folder, and create whatever hierarchy they wanted. When browsed through the metadata APIs -- which the documentation specifies is built from tag data -- then the filesysetm structure is irrelevant. This is a valid structure:

musicFolder/
  artist/
    album/
      song.ext
  artist2-album1/
      song.ext
  artist3/
    song.ext
  song.ext
  album/
    artist/
      song.ext
  artist/
    album/
      disc 1/
        song.ext

Some of these examples may be contrived and seem silly, but some seem (to me) entirely reasonable. If I have a Beetles anthology, the last folder would seem utterly sane. There are artists who release songs only as singles and not under albums, in which case the third subfolder makes sense.

However, the view you see when you use different API calls -- file system vs metadata -- can be different and worse, can vary between servers. Mistagged songs might not be reachable at all through the metadata API calls, and wild directory structures can present lies to the use if browsed through the filesystem calls.

So, the question I've been struggling with is: how do we reconcile and present a sane representation of the user's database, using the OSA, that works across servers and doesn't misplace songs? Which view do we choose as being the canonical view? Do we even try to?

I'd dearly love ideas about this. I'm particularly concerned about mis- or incompletely-tagged songs. If a song is missing the "ARTIST", it won't show up in the metadata view since the top-level data structure there is "artists". I think gonic puts these songs under a meta "[UNKNOWN ARTIST]", and maybe Navidrome does, too. Theoretically, with the search function, users should still be able to get at these items.

I'm least interested in trying to work around the file system view, because it isn't very consistent within the API, c.f. points 1 and 2 above. getMusicDirectory returns an array of directory objects with children, which are almost filesystem nodes that can represent either a subdirectory or a song, annotated by an isDir attribute. However, because it's from the FS, there's no information about whether it's an album, artist, or some sub-directory structure. Also, getting at the root node is exceedingly hard on any server, since getMusicFolder returns no useful information because of point 1.

If I can confirm that untagged music files are somehow reachable through the metadata-oriented API calls, I'd like to completely ignore the music directory API calls in stmps and provide a browsing experience where the user can either browse hierarchically via metadata rooted at Artist, or use the Search tab. That's essentially where my xxxserxxx branch is right now, only I haven't confirmed the untagged-song behavior on both gonic and Navidrome.

I've spent several hours trying to build another browser using only getIndexes, getMusicDirectory, and getSong, but it's quite confusing and it feels a lot like trying to write a multi-panel filesystem navigator -- which is essentially what it would be.

Really happy to get some feedback and ideas on this.

As an aside, I pop out of this filesystem-browser branch to pick off bugs and odd behaviors where Navidrome and Gonic don't behave the same, and those continue to get pushed to the xxxserxxx dev branch.

[mahmed2000]

OSA defines getMusicFolders. The returned items have IDs, but in neither Navidrome nor Gonic do these IDs refer to any object that can be fetched by any other API call -- in particular, not getMusicDirectory, getArtist, or getAlbum.

Not entirely true. Its basically useless in most cases and isn't an object that can be fetched in any sense, but it is accepted by getArtists, getAlbumList, and getIndexes. MusicFolders do not correspond to subdirectories and its only purpose to restrict browsing to a single root folder if you configure the server to use multiple. To confuse things even more, there's this thread. Apparently getIndexes returns an "artist" object and is not supposed to represent actual artists in terms of metadata. Which corroborates with navidrome:

There are currently no plans to support browse-by-folder. Endpoints for this functionality (Ex: getIndexes, getMusicDirectory) returns a simulated directory tree, using the format: /Artist/Album/01 - Song.mp3.

The object is an Artist, but not an ArtistID3 like what getArtists returns. As far as I can tell, the intention is to use getIndexes and getMusicDirectory to navigate by file-view, and exclusively so. The original subsonic API docs also make distinctions between directory/metadata endpoints and even opensubsonic copies the same disclaimer:

For instance, browsing through the collection using ID3 tags should use the getArtists, getArtist and getAlbum methods. To browse using file structure you would use getIndexes and getMusicDirectory.

At the song level, using the parent as the album should work since that's basically the same compromise gonic makes for subsonic compatability. This is a gonic thing specifically, so not entirely correct to use it for metadata:

when browsing by folder, any arbitrary and nested folder layout is supported, with the following caveats:

files from the same album must all be in the same folder
all files in a folder must be from the same album

I'd probably just pretend metadata doesn't even exist in the filesystem-viewer. Its more accurate to say that an Artist from getIndexes is more akin to any directory that is a direct descendant of the music folder root, (aka "indexes"). Subdirectories are just regular directories. The actual way to get metadata in this view is /getSong, which has obvious problems with giant folders spamming requests. The above caveats should in theory work fine across servers. The only exception would be any loose files in the music root since there is no parent there.