Redesign Remaining Menus

[Chaphasilor]

This PR will make the menus for all remaining item types consistent with the track menu.
The design is the same, and the options are as similar as possible.

I'm also planning to add a proper "Play" option to the menu, as this came up a few times now.

TODOs:

• Port all menus
• Add options for play/shuffle
• Make sure all old menu options are still available (and possibly new ones)
• Figure out a way to improve code sharing
  • This could work well by just having an enum for the different options, and then having a custom list consisting of these enums for each type of menu, along with custom handlers for the options (maybe combined into one as a map?). Just the player screen menu might be a bit more complicated due to the playback actions

[Komodo5197]

My thoughts on the potential for code sharing - I'd just turn all the options into widgets. So we have a PlayNextMenuOption(BaseItemDto), and an AddToQueueMenuOption(BaseItemDto), and a DownloadMenuOption(BaseItemDto) and so on. You'll want a helper method that takes a baseItemDto and spits out the list of child songs from either the server or downloads as appropriate. Also, the buildWrapper option for the themed bottom sheets is mostly needed due to the complications of the track menu. I think most of the others could use the buildSlivers option where you just hand it a sliver list.

[Chaphasilor]

Missed your comments, got lost in notifications. I'd never have considered using widgets for menu items, but it makes a lot of sense for sure. It's also a great way to keep the functionality/logic tied the option, and allows easily extending the functionality for specific options (showing favorite state, handling long presses, etc.).
So I'll probably do just that.

For the helper method, I'll look into it. But I'm not sure we need the same list of tracks in all cases, so the logic might become more complicated.

I'll also take a look at replacing the buildWrapper. I'm guessing you'll let me know if I break something there...

[Chaphasilor]

I also think I understand your comment about the helper function now. We add one function per menu entry widget, and that function handles the various BaseItemDto types?
I'll try to do that, but maybe handling the cases right within the onTap function works even better.

[Komodo5197]

I was imagining a single global helper function. All the play options should be doing the same thing. So the play, add to queue, and add to next up options for, say, an artist always fetch the same set of tracks using the same logic. They just add it to the queue differently. I can't think of a reason we would want a different set of tracks for the same item - that would be really confusing and unexpected for the user? Even the play/shuffle options on the artist page itself should want the same set of tracks, I'm not sure those could be easily or usefully unified. But I was imagining a single function which switch between multiple sets of logic depending on the given BaseItemDto type. And then all the menu option widgets just use that function and feed the returned tracks into the correct queue calls in their tap handler. At that point, the difference between the same menu entry, such as play next, across the different item types is hopefully fairly minimal?

[Chaphasilor]

Yeah I think it could work. Although essentially this is was JellyfinApiHelper.getItems() is supposed to be, isn't it? But maybe this helper function could be slightly less abstract, and pre-apply some filters based on the current settings (i.e. global favorite filter and such).

[Chaphasilor]

@Komodo5197 I've moved a good chunk of the menu into reusable widget that can be composed into the different menus.
I'm guessing I could squeeze out a bit more code sharing, but I don't know if that makes a lot of sense? We could have a generic showModalMenu() function that accepts the content from all specific instances of the menu. It would make things less flexible, but maybe that's what we want to improve maintainability? Would appreciate your thoughts on the matter :)

Oh and I made the item request helper method (loadChildTracks in item_helper.dart) a provider. Doesn't add much complexity, and I think network requests are really what providers are made for? Is there any downside to doing it like this?

[Komodo5197]

I don't think a showModalMenu function is needed, it feels like the code reuse balance is in a pretty good place as-is. Although I'm questioning why either argument to PlaybackActionRow exists. Why would we ever want to show a different set of options? That would also open up more avenues to make the section more adaptive and/or customizable. The other code reuse thought is to just see how much of the track menu can be built out of the new widgets. And it might make more sense to pack all the download menu options into one widget that returns the correct state rather than having to always add all three to the menu.

For loadChildTracks, I don't think putting it in a provider is really that great in this scenario. providers are great whenever you need to make a network request while building a widget, but they aren't really helpful when making a network request from a tap handler. All they do is add the possibility of accidentally not waiting for the future to complete when you need its value. I'd make it a regular function unless you have a specific goal in mind that needs the caching or rebuilding.

[Chaphasilor]

We will actually already have a different set of options for the artist menu, there currently is a "shuffle artist" (all tracks shuffled" and "shuffle artist albums" (albums shuffled, but tracks in album order). That's why I made it flexible.
Bundling the download options is a good idea.

The provider was mostly meant to be consistent with the home screen PR (where I responded to your comment btw), since I'm also using a provider there. But there the caching is more important too, so yeah, maybe the provider is overkill.

[Komodo5197]

If we already have variations, that does make more sense, although I'd still lean towards having that handled internally because its just based off the item type in the same way the child track fetch is. But I think part of that is because I don't actually like the precise layout of the actions, so I'm already thinking of customizations. I can see the logic of play and shuffle being the two categories, but the most used buttons are probably play and shuffle, and this puts them on seperate pages so even if we save the position there's a good chance you need to switch. It might be nice to have all the next up related items on one page, and the other options together on the other. Or maybe it could even be customizable like the filter chips or music screen tabs. There's also the idea of just showing both rows at once on larger devices. But this might all be me overthinking, I always just use the main buttons on the album screen anyway, and it's not like a swipe or two is that bad.

[lukaslindnermusic]

@Chaphasilor So, I just experimented with the childCount for Artists (regarding the displayal of the albumCount as subtitle on the artist list), as you've mentioned that you are also planning on integrating it in the new menus.

Unfortunately, I got a lot of album-artists that displayed my "?? 0" fallback of "0 Albums", which shouldn't be possible because they definitely have albums - I checked manually - and otherwise they wouldn't even show up in the list in the first place.

I think I found the answer for that oddity. The problem I discovered:
For an album, Jellyfin only increases the childCount for ONE album artist. If you have an album that has two or more albumartists assigned, the childCount for only one of those will be increased, resulting in incorrect data for the others. 🤡

Unfortunately, it seems that there is also no valid count for trackChilds songCount, which I planned to display as subtitle when "Performing Artists" is selected, and as getting both values accurately would require additional queries at the moment, I vote that we remove the albumCount for now (since it displays incorrect data) but leave the functionality in the code, commented out.
We then riot on the Jellyfin Server issue board and as soon as they fixed this server-side, we can enable it on our side again.

[Chaphasilor]

Haha yes I also had similar problems with those counts. Good catch we with the album artist counts, I guess that really is something they should be able to fix on the server :)
I guess we should wait until the next release though, since they are already behind schedule and probably super busy right now.

[lukaslindnermusic]

The actions to fetch artist and genre tracks currently are not yet correct.
I think they would include items from all libraries, not just the selected one.
And artists won't include the "Appears On" tracks, only "album artist"-tracks.
I've left some comments how I think it should work 😊

[lukaslindnermusic]

Haha yes I also had similar problems with those counts. Good catch we with the album artist counts, I guess that really is something they should be able to fix on the server :)
I guess we should wait until the next release though, since they are already behind schedule and probably super busy right now.

Ooops, I already created an issue now :P
jellyfin/jellyfin#14150

[Komodo5197]

I think you messed up when copying.

[Komodo5197]

Shouldn't this be albumId? That's what the rest of the widget uses.

[Chaphasilor]

Could be. The original track menu used the parentId check, so I simply moved that check here.
I haven't properly tested things yet, so I'm not even sure if everything still works.

[Komodo5197]

This should be the same as the other mask, 135.

[Chaphasilor]

The MenuMask stuff took me a good while to figure out, and I still don't understand it. Essentially this is what adds the fade effect and overlay behind the ItemInfoHeader, right?
So there should always only be a single instance of it?

It felt as if this should either be part of the actual header, or part of the themedBottomSheet.

[Komodo5197]

It specifically masks out any portion of its contents that gets withing X pixels of the top of the scrollview. We need to do this because the iteminfoheader is transparent and allows you to see the background, so when the other menu items are slid behind the header as you scroll you can still see them. This can't be done in the header because it doesn't have control of how the other items in the list are rendering, or at least I don't think it does. It feels overly specific to put into the themedbottom sheet. We could put all the other items into a single instance of menumask, but it should work fine with multiple, and I'm not sure if either way any performance implication. Either way is probably fine.

[Komodo5197]

Why do we have the text visible and as a tooltip?

[Chaphasilor]

I'll have to check on my laptop, but I think there was an instance where the text and tooltip were different at some point, and this is just what remains of that. I'll confirm that TalkBack treats it equally if I omit the tooltip.

[lukaslindnermusic]

We now have to go to the GenreScreen instead of the ArtistScreen ;)

And also, this currently only takes you to the first available genre. Do you have something planned that lets you select which genre you want to go to when there are more than one?