Albums start showing up as blank after clicking through other ones

[ptrcnull]

i don't really know how to properly describe this issue, so here's a screen recording:

recording.mp4

bisected it down to be a regression from 0754fdf, sadly

[mahmed2000]

I can reproduce this with a gonic server. Its not just after going through other albums, going [..] one level and back in triggers it. Removing the if check from:

stmps/subsonic/api.go

Lines 341 to 343 in 4a8428b

	if cachedResponse.Album.Name != "" {
	return &cachedResponse, nil
	}

makes the problem go away, but that's going to cause problems for whatever case necessitated that in the first place.

As for what's causing this:

• The page_browser fetches a music directory with songs (non-directories) in it upon selection for the first time (handleEntitySelected). The cache for id al-xx gets populated.
• The loop over .Directory.Entities hits a song, which runs makeSongHandler.
• That function retrieves the Album of the song's parent. With gonic (idk if its specific to it), the id of the directory is the same as the id of the album, al-xx. Because of the check failing, the cache is ignored and overwritten. The request is successful and the cached SubsonicResponse now has a populated Album struct instead of a SubsonicDirectory. All the songs are in the .Album.Song slice, instead of .Directory.Entities which is now empty.
• Going up and down fetches the music directory for id al-xx again and hits the cache. Because the response now has an empty SubsonicDirectory, nothing gets rendered. Since the .Directory.Parent is also empty, the [..] entry also doesn't get added.

[xxxserxxx]

I think this is a dupe of #82. I'm working on it.

It's ultimately related to the fact that Subsonic -- and stmps in turn -- look at music in two conflicting ways: as a filesystem, and as a tree of things organized by metadata. As you've found, some API calls return incomplete data if the filesystem structure doesn't match the tags.

I can replicate this, and it'll be resolved when I fix #82.

[mahmed2000]

This would be a dupe, yeah. But just to clarify, its not incomplete data, just the Response type is different. Matching tags wouldn't affect anything since its a completely different object. The song entities are there, just in a different substruct. Logging the value of the response.Album.Song.Len() in handleEntitySelected gives you the expected number of entities. Committing to either representation will break anything relying on the other. Unless they're merged somehow which has its own headaches.

Would just changing how the cache indexes responses help, since the key is already a string? Combining the id with some identifier of the endpoint might resolve this. Everything else should keep working as is, and any case where the server might respond with 2 different views, the object would be saved according to the endpoint. So /getAlbum+al-1 and /getMusicDirectory+al-1 (or equivalent) would store different responses for the same id, instead of GetAlbum overriding GetMusicDirectory. Anything needing the album's metadata can use that response without messing with the response for directory navigation. Could expand it to cache parameterized calls as well if needed.

[xxxserxxx]

First, this is a good observation, and it changes how I'm thinking about the solution. It comes at a good time, too, because I dropped this for other Life Events a couple of months ago and have forgotten where I was with this.

Second, let me say issues like this are why we might prioritize #39. The bespoke Subsonic client in stmps is really kludgey, stuffing a bunch of different responses into one struct in a union-type-but-not-really thing. It's not particularly type safe, and error prone (tomAto, tomAHto).

Third, as I mention in #82, there's also a disturbing amount of indirectly linked information in stmps, which you have also observed. Better encapsulation might help, but it'd be a large refactoring, and there's already one going on. I think I'm not going to try to address that in this issue, and will simply try to implement the least invasive fix in the hopes of a quick merge.

Right now, I'm thinking that if we make a fresh call and get empty information that would overwrite non-empty data, we don't do that. This may necessitate a dirty or source bit, to handle the edge case where information really is removed on the server side, but I suspect that's so edge we can ignore it for now.

[xxxserxxx]

So, there's a lot to unpack here. First, there's this concept of a "parent ID" which is not clearly defined in the API. Is it a directory ID? An album ID? Could be either! Compounded by the fact that we shove every response into the same struct type, it leads to confusion -- and, in particular, something I did in conflating the IDs in the directoryCache with album IDs.

So I'm starting to clean this up; GetAlbum() won't return a SubsonicResponse, but an Album. We do, after all, have an Album type. As we do have Directory type, and all of the other specific types for which we currently return SubsonicResponses when we Get() them. The only truly ugly thing here is SubsonicEntity, which the OpenSubsonic API uses as a generic catch-all for "things on the server".

However, I'm increasingly thinking we need to bite the bullet and implement #39. The bespoke Subsonic API we have really is a mess. I'm going to see if I can fix this with minimum fuss, but any further bugs or API changes, honestly, we just need to switch to a specialized library.

@spezifisch I know you're busy with other things, but what are your thoughts here?

[mahmed2000]

I would assume "parent" means the parent directory, given the "Child" object also has an "albumId" variable. But the opensubsonic docs state the parent is "The id of the parent (folder/album)". The worst part is the "parent" attribute is optional for the response.

[xxxserxxx]

I've come to the conclusion that the UI is a lie, and we need to rethink the model.

The Browser tab says it's listing Artists in the first column, and either Songs or Albums in the second. But that's not what is going on in the program; it's working on Directories, which can be anything but a song. It's not built from the music metadata, but from the directory structure. A directory can be a folder with some loose songs and a subdirectory with an album, and in the UI, this is what was being shown. If that's what it said was being shown, that would be fine, but it isn't: stmps claims what's in the left column is Artists, and what's in the right is Songs or Albums (but never both). Over time, as features have been added - mostly the ones I've added, which tend to focus on the metadata - this issue has been exacerbated and now the model is breaking.

There are two options here:

1. Roll things back and have Browser only deal with directories, and stop lying about what we're showing the users. On the left is Directories, and on the right are ... Directories, or Entries, or both, possibly at the same time. I can still get at my metadata for the Queue tab. However, browsing the file structure is IMO a last resort for broken metadata, so I'd prefer to:
2. Have Browser show information built from metadata, and add a hotkey to switch it to the Directory view. Or maybe add another tab; it's getting a little crowded there at the bottom.

In case 2, instead of fetching Directories as stmps currently does, it'd fetch Artists, and put those in column 1. When an Artist is selected, it'd fetch the Artist's Albums and put that in column 2. No loose files -- songs are always under albums. Selecting an Album shows Songs. Then the titles are true, and we can always know what state we're in, which is impossible with Subsonic Directories (which are, after all, dumb folders).

We still need a Directory browser, because no library of any size has entirely clean metadata; there may be songs without Artist or Album set, in which case, they are invisible. But I argue this should be a fallback, not the default, which is what stmps (and stmp before it) does.

[xxxserxxx]

I would assume "parent" means the parent directory, given the "Child" object also has an "albumId" variable. But the opensubsonic docs state the parent is "The id of the parent (folder/album)". The worst part is the "parent" attribute is optional for the response.

Your comment came in as I was writting my essay.

To be clear, Subsonic Directories have no semantics. They're folders. They have a parent ID, the root of which tree is just the root; in gonic, it's "al-1". Entities in Directories are also not typed; there's a flag that says whether it's itself a directory or not, and if not, we can assume it's a song, but if you fetch directory "al-1" from gonic, you get a bunch of other directories and any loose songs, and there's nothing to tell you whether any child is an album or artist.

BTW, pre-fork stmp behaves the same way. Browse and select an artist and then click "[..]", and you get the same content that's in the "artist" column. This has been broken since before @spezifisch forked the project, because it's based on browsing directories and pretending it's browsing metadata.

[xxxserxxx]

I pushed a new branch, based off main, that has a major refactoring which changes the browser to be entirely metadata-based. While I believe that Navidrome users may not see a difference in visible content, gonic users will. Gonic draws a clear distinction between "Directories" and "Artists"/"Albums". Directories, and the related API calls, reflect the filesystem structure; Artists & Album API calls reflect the music metadata tags. The OpenSubsonic documentation itself conflates Directories and Artists/Albums: an Artist ID is also a valid Directory ID, and Navidrome's implementation is therefore more "correct." The author of gonic, disagrees with this and the implementation behaves consequently differently.

For example, consider a filesystem:

Audio/
  Amy Grant/
    Black Sabbath/
      Iron Man.opus

where the song "Iron Man" is correctly tagged with artist and album. Navidrome and gonic will present two different views of this.

This has been a source of some odd behaviors in stmp (and then stmps, which inherited the model). stmp's "Browser" page calls the left column "artist" and the right "song" or "album"; however, underneath it is accessing all of the content through the Index and Directory API, which means in gonic the left column is merely the top-level folders of the filesystem, and the right is whatever subdirectory filesystem is being navigated (with the context being determined by whether the current directory has a "parent" or not, an unreliable measure). Navidrome is a little more consistent with the API documentation which claims that "Index" returns "Authors" but also in the "Directory" calls claims that IDs returned by Index can be used to reference filesystem entries.

Fundamentally, the OpenSubsonic API, and Navidrome, assume a strictly ordered filesystem layout: the audio directory contains Authors, and each Author directory contains Albums, and Albums contain songs. If your library follows this strictly, and the server assigns IDs to things under this assumption (as Navidrome does), then it doesn't matter which API calls you use. However, if your library has files at the top audio directory, or maybe directly under author directories, or maybe you have some albums that are at the top level and are not under an author directory... things get messy with Navidrome. gonic handles the latter case better, but it also does not conform to the API spec because IDs are not fungible between the Index/Directory views and the metadata (tag) Author/Album/Song view.

Personally, I believe that many libraryies of any significant size are not going to be 100% clean, and that gonic's approach is in this case better. I also believe that servers should follow the API spec, and both can't be true. Consider my "Amy Grant/Black Sabbath" example above.

So: I'm in the process of doing two things that are major renovations. The first is this new branch "metadata-based-browser", which completely removed all code accessing the Index and Directory API calls by the browser page. The columns are now populated entirely by the Artist and Album API calls, which (per the spec) are built by metadata in the song files and not by the directory structure. The downside is that files will not be accessible if the IDv3/Vorbis tags are not clean; if a file is missing an ARTIST tag, it'll never show up, because the tree now (truly) starts with Artists.

The second step will be implemeting a browser view that only uses Index and Directory API calls. It'll basically be a filesystem browser, which is really what those APIs provide. This will allow you to access those poorly-tagged files. When I do this, I'm also going to add a feature that highlights files with missing structural tags (author & album) so you can use stmps to identify files that need their metadata sanitized. I'm not going too far with this, because fixing metadata isn't stmps' job, but combined with the search function I think it'll be helpful enough.

I would appreciate any testing people are willing to do. If you grab that branch, you'll get the main branch plus my changes related to this feature. This should also address #82/#85. It may also have bugs, since I haven't thoroughly tested all features; however, since it mainly affects the browser page, in a way it's constrained and should not impact other pages.

[xxxserxxx]

There's another new branch: xxxserxxx. It contains all of my outstanding PRs, including the metadata refactor. This branch has a bug that prevents fully browsing Navidrome servers; I know why, it's just too late for me to fix it tonight.

These enhancements include:

• Fix for "Save queue as playlist" keybind is overriden by the global start scan bind #88, the global key binding for "refresh server" overrode "save playlist"; it was changed to 'c'
• Song info panel (song information, lyrics, and cover art) can be toggled with 'i' (ty @mahmed2000)
• Synced lyrics are shown, and synced with the music, if the server supports getLyricsBySongId/. Currently, a PR in gonic provides this through filesystem .lrc files. This version of gonic is in the main branch of @danielepintore's fork.
• The search tab can toggle between "search for anything" (via search3/), or search-by-genre (via getSongsByGenre/. As part of this, switching to the genre search in the search tab with 'g' also shows a list of all known genres, which can be browsed.

I'll be pulling in other people's PRs as soon as I address a couple of things, which I'll do in the next few days:

1. Add a directory browsing mode for the browser tab. This'll close the gap where poorly tagged files won't show up in the new purely metadata view modality.
2. Merge the concurrent-album-art branch, which is an outstanding PR that loads slow assets in the background and improves UI responsiveness
3. Fix the Navidrome issue, and do some more testing with Navidrome

Then I'll pull in other outstanding PRs.

Finally, in no particular order, I would like to address the following items, none of which are a huge amount of work:

• Stop deeply populating playlists. The current code (which I wrote) loads all playlists recursively in the background. It's slick, but unnecessary, and diverges from how the rest of the program browses content. Removing it will simplify the code and make the behavior more consistent. Going in the other direction, and making everything else pro-actively load content in the background is another option, but would result in stmps effectively mirroring the entire server DB in memory, which is undesireable.
• Put album art and album cache content in an LRU, to prevent bloating memory use. Caching everything is great for performance, but bad for memory use. I added an LRU a little while back, but now I need to hook it up to the heavyweight memory objects.
• Add the ability to filter playlists, a-la the search fuction on the browser page
• Possibly have the genre search function not perform a search so much as mirror how artists are navigated in the browser page: automatically show the contnts when the genre is highlighted.
• Show artist images in the browser
• Update screenshots in the README
• Add a mocking library and really flesh out the unit tests; in particular, mock out the OpenSubsonic API with the documented behavior, and ensure our bespoke API library uses it correctly
• Improve the CLI help. Viper is really pretty awful, but evenso the current --help output is especially bad and incomplete

Again, feedback on the xxxserxxx branch will be highly valued. This is also the same code that's in main on my fork, but as I don't want to become the main maintainer of yet another project, I'd prefer to keep things here under @spezifisch's fork.

[mahmed2000]

Problems I've noticed so far on your branch:

• Pressing "a" on an album in the new metadata browser doesn't populate the queue, even if the cursor jumps. Only on the artist or individual songs works.
• Adding an artist overwrites their album list with that of the next artist's. So:
  • Adding artist A, queues all songs in album Y.
  • Observe artist B, with album Z.
  • Navigate back to artist A, now has album Z instead.
  • Adding artist A adds album Z.
  • This is transitive, so for artists A, B, and C, adding B then A, removing the queue and then adding A again adds C's albums.
• Some general weirdness switching the normal/genre search. Searching in either state, and then switching modes while the stmps is still populating results gives odd behavior.
  • Searching "A" in the regular search mode, populates a few artists, songs and albums. Each "tick" adds more as expected. Hitting "g" during that, switches to the genre mode, but the pending (when switched) request's results added in. Inverse does the inverse, but same in principle.
  • The result count also does weird things, and doesn't get wiped when switching modes when not doing the above.

[xxxserxxx]

Thanks @mahmed2000. The first two are probably related to the refactoring. The last one I'm going to guess is how it behaves on the other branch: the loading happens in a thread, and there's no user interaction interruption in either branch. I've been aware of it, but haven't prioritized it.

Although I didn't mention it, I've decided that my implementation of playlist population is inconsistent with the rest of the UI: it proactively deeply populates playlist data into memory, whereas in all other cases, stmps pulls data only as users browse. In one way, it makes the user experience more snappy once the data is cached; in another, it adds complexity and uses more memory. There are three solutions:

1. Change everything so that it proactively populates data in the background; then playlists will be consistent with everything else! The upside is that the slight lags when browsing would go away; the downsides are added complexity (and, therefore, bugs), and eventually the entiry music DB would be essentially cached in memory on the client, which would mightily suck.
2. Remove the deep caching from the playlists code. It'd make playlists act like everything else, and simplify the code. The downside is that playlists will now have the same first-time-fetch lag during browsing.
3. Leave things the way they are, and accept the divergent behavior. The downside is that the playlist population code is significantly more complex and, therefore, more difficult to maintain and build on than everything else.
4. Start going down the road of building a local persistent storage, which would allow caching and faster response without sacrificing memory. This would add a lot of complexity, and is exacerbated by the fact that the OpenSubsonic API does not provide "last changed" attributes on any items, which would make it impossible to perform any sort of efficient dirty data algorithm. stmps would basically have to refresh the local cache every time it runs. This could happen in the background, but it'd still cause a lot of network traffic and would be horribly inefficient as -- most times -- very little or nothing would have changed to necessitate a refresh.

#1 is fraught. It has the same problem as #4, in that there's no way to detect server-side changes, and I worry about memory use. #4 is, I think, impractical for the reasons I listed. #3 increasingly bothers me aesthetically -- it's a drastic departure from how the rest of stmps operates, and really is a lot of added complexity. Therefore, unless a hue and cry is raised, I'm going to factor out all of that background loading, and make playlists behave like any other tree data.

[xxxserxxx]

@mahmed2000 #1 is fixed.

[xxxserxxx]

@mahmed2000 #2 is fixed

[xxxserxxx]

So, I did a better job on the threading than I thought, and user events should be interrupting the search properly. In any case, I can't get either my gonic or Navidrome to respond so slowly that I can trigger the issue you're seeing with your search issue. I believe it's happening, but I can't reproduce it. I'm going to backburner it and knock out some other more critical usability issues, such as the (now) missing directory browsing functionality. Directory browsing is probably not as important on Navidrome, but the way gonic is implemented, it's critical to get at files that aren't well-tagged.

I'm now moving this conversation to a different ticket. This set of changes is relatively large, and @spezifisch is going to be overwhelmed by them; I want to track the branch progress in a dedicated tracker.