Conversation
Chaphasilor
added
feature
|
Wow, looks really good 😍 Will there be a toggle to turn of the display of the server name? And shouldn't it be "Track Mix" instead of "Song Mix"? ;) All in all: It looks very nice 🙌🏻 Thank you! |
|
What happens in Offline-Mode? So msybe, when you don't wwnt to display the server name, you could just let it say "Connected to Server"? |
|
Why would you want to hide the server name? There's no other use for this space at the moment. And yes, the text will show "Offline Mode" when that's enabled. And the "Song Mix" thing was because I simply implemented the design mockup. It's a hard-coded string at the moment, so it will be replaced with the proper track-based name of course :) |
Don't get me wrong 😊 I think its good to have it there and I personally will keep the server name there. |
|
Okay. If I receive that feedback by more people, I'll add it. Your reasons sound compelling. But right now it doesn't have priority, and is easy to add later on :D |
|
Hey ! Firstly : huge thanks for your work, this app is amazing, and your investment in it is really cool. second : a few questions
thirdly : I agree with Lukas on the server name, even though I lean more towards hiding it fourthly : thank you again ! I intend to learn and try to help as soon as possible ! this app is really cool and I would be proud to participate (not English native, sorry if I made some stupid mistakes) |
|
Thanks!
|
' |
|
@e-v-o-l-v-e so for the shuffle button all that's needed is to uncomment this section here (and the lines below): Just do that and open a PR? 😁 |
|
I'm firmly against the navigation changes introduced here. For one, the usage of multiple floating bottom bars just seems kind of silly? I also don't like the packaging of all the apps current main screens into a single subcategory. I have basically no use case for either the home and search screen, so this is just hiding away everything I actually care about, and adding a constantly present menu bar pointing to them to boot. This bar is also a bit irritating in places like the album screen, which already feel a bit cramped on smaller screen sizes. My preference would probably be something closer to just adding the home and search screens to something closer to the existing tab bar? Anyway, a couple small oddities I noticed:
|
|
This is still a draft for a reason :)
I'm very sorry to hear that you hate the proposed changes. I do have to say that a lot of users have expressed their excitement about a "proper" home screen in the recent weeks, so I don't think we should ditch it. Selecting a default tab wouldn't be a problem, but the additional nav bar using up screen estate is (for people like you who are focused on the library at least). The search overhaul is long overdue, having to switch between tabs to find something is strange and inefficient. And the way the search bar hides other controls also isn't ideal. A separate screen seems to be how most other apps do it. If we find a way to get rid of the navbar we could theoretically move the search button to the top, but with the navbar we have enough space to include it there. About the home screen, are there any sections or other functionality that would make the home screen more worth-while to you? It's meant as a quick access to things you listen to frequently, and not as a burden or inconvenience. I'd like to know how you usually start playing music in Finamp, and how you find/select it! |
|
I do understand the desire for the home and search pages, even though I don't actually care about them myself. The existing per-tab search definitely sounds clunky, and I have seen quite a few messages across this PR and the various issues asking for a home screen. This PR includes both the addition of the home and search screens as new ways to access your music, and also a initial step towards an app navigation overhaul, and its the latter that I'm mostly concerned about. I think I basically have three concerns - the demotion of the music screen, the space usage and mental model of the navbar, and the future of the sidebar. With the current app design, on startup you are immediately presented with five ways to browse your music - by album, by genre, and the other main tabs - all of which are immediately available and can be freely reordered or even hidden. This PR presents two new ways to browse - via the new centralized search, which elevates search from a mere filter of the tabs to its own independent browsing method, and via the home screen. But then these two methods are put directly onto the navbar, which is more omnipresent than any other UI element we have, while every existing browsing method is crammed into the library section, promoting the home screen as the main way to browse with all others secondary. I strongly disagree with this, and feel that the home and search screens should be treated as equal to the existing tabs. This has two aspects, navigation accessibility and control availability. For navigation accessibility, the most straightforward way to implement this would be to have all seven browsing methods availible on a single bar. If this is considered unwieldy and you want something more like your navbar implementation that never allows scrolling, one idea would be to extend the current tab customizability so that the navbar has two or three 'highlighted' sections of the user's choosing, which would default to home/search but could be changed to any of the existing tabs, and then the library section would be an overflow section for all the remaining browsing methods, potentially including the home screen or search if the customization dictates. By control accessibility, I basically mean that I'm concerned about the home screen gaining important control functionality unrelated to its use as a browsing hub which would be absent from the existing tabs and necessitate switching to that screen for non-browsing reasons. I'm also not really that enthused about the navbar from a space usage perspective. This would be alleviated if the more advanced customization scheme I mentioned above was implemented, but I really do not want to be dealing with two separate navigation bars taking up space on my most used screen in the app. We already have the fairly large now playing bar taking up space in addition to the music-screen specific controls and navigation. I have a similar space concern about the album screen and its like. On a multi-disc album, adding another bar would bring me down to eight visible tracks, which is certainly usable but its still low enough that every additional one counts. On my old phone, losing a song to the navbar would have taken 20% of the available space and brought me down to four visible tracks. Showing the navbar on screens like this also brings up an additional concern, which is how it interacts with the stack page route model. If I've drilled down from the library to a genre to an album, and then I hit home on the navbar, what happens to this stack? Is it dumped, or preserved in the other tab, or is the home screen now on top of it? All of these options seem strange, and would cause me to lean towards only showing the navbar on the main browsing screens to avoid mixing the tab and stack models like this. Shrinking these bars down would at least partly help with these space concerns, although I'm not sure how far we can really shrink them and still keep everything readable and clickable. Making them non-floating would reduce some padding, and also avoid the fact that I think stacked floating bottom bars just looks a bit dumb. I'm already not a fan of the way we have the floating instant mix button over the floating now playing bar, and if that stays then we would end up with three floating elements, which is absurd. My final big concern is with whatever is planned with the sidebar design. I don't think I'm strictly opposed to replacing it with an equivalent bottom sheet or pop-over, although with the popover option I'm a bit concerned about the border padding reducing usable UI space on small screens. I'm not entirely sure what you mean by sidebars being outdated, and I don't know how it might be interfering with gesture navigation, but whatever. A large part of my concern in this area is functionality getting removed from the sidebar, which is easily accessible from all the music screen tabs, and being consigned to the home screen instead, or other less accessible locations. All the functionality should remain quickly accessible from the music screen tabs, and if you're messing with the access maybe even some of the other screens, if that can be fit in. Also, as I mentioned before the way you access it on the homescreen does not seem intuitive as all. I don't think I ever would have tried clicking it if I hadn't seen its functionality in your description, and even if it looked more button-like I wouldn't have guessed its functionality. It would also be nice if we kept a gesture-based way of accessing the new sidebar, because the upper left corner is just out of reach on my new phone and there's a lot of important functionality in there. Other minor points:
|
|
I think, Komodo mentioned a few valid points there. While I still like the idea of having a home screen and I like the way you structured the screen itself, the navigation and type of integration in the screen flow bugs me a little bit as well... especially the bottom navbar with Home, Search and Library, that takes a lot of space that most of the time is unused and just blocking the view. Also, I don't know why the side drawer is an oddity in iOS. What's the problem with it? 🤔
When I'm searching, I dont care about the sort order, I want the results that match most, for quick access. Maybe the only thing that would be nice to have access to is the favorite filter...
Sure, there are things that are not ideal, but in general: What are the features that it was never designed for? I actually like it, that you can very quickly swipe between item types and have everything at your fingertips. Sure, maybe we could redesign the styles a little bit, and maybe update the sortBy at some point (I don't know how you can sort genres or playlists by AlbumArtist or PremiereDate :P And what about that Community and Critic Rating? Isn't that a film/series library thing? Is actually someone using this for music?), but in general, I like the structure. Suggestion:
What do you think? 😊 |
|
In the Mockup, I added all of the sidedrawer options (except logs, you don't need that unless you have a bug) on the top right instead of having the drawer button on the left. Regarding the ServerName: When you have multiple libraries, I think it makes sense to default to displaying the current library name instead (or was your Home Screen concept planning with a screen that combines all libs?). |
|
Something like that: |
|
Regarding the "Listen Again": Unfortunately, SortBy.datePlayed seems not to work on items other than tracks - We could still include that section on the HomeScreen but with tracks instead of albums, or we could load limit x tracks but then extract the albums and only use the latest 3 to display. However, we don't know how big limit x has to be in order to really get at least 3 albums, because there could be an album with 20-30 tracks or more, or 5 albums with only 5 tracks or something like that. |
|
Two more ideas. Idea A) Idea B) |
|
I don't think a dropdown to select libraries works from a UI/UX perspective . Many people likely have their music libraries simply named "music", and a dropdown in a music player app that simply says "music" could mean literally anything. I prefer the current design on the redesign branch over the dropdown |
The Library-Selection-Dropdown is actually completely irrelevant to my idea. It was just a suggestion to have some more space in the drawer and split the functions (because the other menu options are not a selection but navigation to a Screen). We can also keep the current library selection there and make the side-drawer-menu scrollable, or find a different place for the library selection that is as accessible as the current one. We could also not display the current Library Name there but call it The important part of that idea is to put |
|
As you can probably tell, I've been procrastinating taking another look at this issue for a while now. But now that some time has passed and I'm not as attached to the proposed design any more, it's about time to get on with this. Thanks for all your honest feedback and suggestions. I was honestly a bit scared when I saw your reply @Komodo5197 - so long that it didn't fully fit on my laptop's screen without scrolling. But of course, it was (as always!) well thought-out and sensible, and I'll try to address it below. The initial idea for the revised navigation stemmed, as so many other ideas, from other popular music streaming apps. Those usually divide navigation in some kind of browse view (usually a home screen) and a library view. Examples for this include Spotify, YouTube Music, Deezer, and even Jellyfin clients like Jellyfin Web/Android or Symfonium. What I'm trying to say is you're right, there's far less need for a home screen for personal music streaming. There's no new music to discover, so "feed" of new releases or posts that you need to catch up with. If you've bought the music, you're already aware of it. But let's be more concrete: Regarding selecting items for playback - do you have any heuristic for how you decide what to play? What's your preferred sort order? Do you use the favorite filter? Or is your library small enough to be browsable in its entirely quickly enough? I'll try to brainstorm a few more ideas this weekend. Thanks for the suggestions (from everyone)! |
Good that you are back on it now <3
Understandable :P
I know that you're now already convinced that that bottom menu is bad, but just as a side-note to add my personal opinion on the Jellyfin Web/Mobile thing as it triggers me since I use the app :P
Nice 😊
I personally don't have to go to the settings that regularly that I would miss that option when I'm "down a stack" aka. in an album via Artist Screen or something.
And then, we have 4 slots on the top right of the app Bar, and you can customize these up to 4 buttons yourself, from the options of the menu. So if you only care about search and Downloads, you can only display those two. And when you are on an Artist Screen or something, we could add a shortcut button to open the entire menu, if thats important.
My personal listening habits / decisions: Most of the time: Sometimes: I rarely use the "Tracks" Tab. And the favorite filter I only use when I play favorited tracks of a genre. My Curated Item Sections on Genres are: Artist Top Tracks: |
Chaphasilor
left a comment
Chaphasilor
left a comment
There was a problem hiding this comment.
no idea why GitHub considered my replies to be part of a review, but whatever...
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| FinampSettings? finampSettings = ref.watch(finampSettingsProvider).value; |
There was a problem hiding this comment.
the settings value wasn't used anyway. I'm guessing it caused rebuilds on settings change? If so, we should probably listen for relevant settings with a single .select query?
| // items = sortItems(items, SortBy.datePlayed, SortOrder.descending); | ||
| // break; | ||
| case HomeScreenSectionType.tabView: | ||
| //FIXME this seems to also return metadata-only albums which don't have any downloaded children |
There was a problem hiding this comment.
I think what this showed were albums that belonged to an artists or genre, but weren't downloaded? Like the grayed-out tracks that are shown for non-downloaded album tracks?
But I might have to take another look.
But regardless, I don't see the bottom sheet sidebar as appbreaking or anything, but I'd still prefer it unchanged. I believe my opposition is mainly stemming from the lack of 'top-level menu' feeling I mentioned earlier. With a drawer, even though it's not what you first see by default, it really feels like it's the 'root' view of the app, where everything fans out from. With a bottom sheet, it instead feels subordinate, leaving the root to the Home/Music screen. And I feel this 'root' being a menu versus a content screen, especially one that by default serves content semi-unpredictably, just gives a very different vibe. This is regardless of the fact that the default view is the content either way, which is definitely correct. So in the end, I think this is just an extension of my objection to the conceptual design of the home screen as the app root. It's possible I'm just reading too much into all this, but I'm pretty sure this is where my instinctual displeasure with this change is coming from. Compounding on this objection is the fact that I regard any non-trivial UI changes to be strongly net-negative by default, unless backed by concrete reasons why the user experience will benefit, and I just haven't been sold on how this is a tangible improvement yet. But as I said, although I feel relatively strongly about this, I'm not sure the outcome of this decision actually matters that much, and I feel I may already have had an outsized influence forcing my mental conception of the app as a whole over yours, so if I've failed to convince you then you can leave the change in and I'll try to cease complaining. |
| if (sectionInfo.type == HomeScreenSectionType.tabView && sectionInfo.contentType == TabContentType.tracks) | ||
| //TODO use similar logic to [loadChildTracksFromShuffledGenreAlbums] for loading tracks from other tab types | ||
| //TODO for collections, try to recursively load tracks directly, Jellyfin can do that | ||
| ...[ |
There was a problem hiding this comment.
We might be able to add some of the downloads which are currently confined to the settings onto the corresponding home section if present, such as the favorites download.
There was a problem hiding this comment.
Ah you mean like adding an additional button to the section header?
That is definitely a nice idea, but probably won't replace having this in the settings somewhere, right? People that disable the home screen might still want to download their favorites.
There was a problem hiding this comment.
Yeah, we definitely still need them present elsewhere, but this could make them actually discoverable.
There was a problem hiding this comment.
Thinking about this some more, it seems like this would only work well for playlists and recently added albums?
The "favorites" special download we have includes all item types, and there's currently no way to show this on the home screen.
There was a problem hiding this comment.
A lot of it doesn't align perfectly. For example, the home screen section for latest is showing all items sorted by date added, but the download needs to be specifically targeting the X latest. But having the downloads attached is still probably better than leaving all this hidden in settings. Maybe we could just have a variant of the download options screen we currently pop up that has the full details? I guess theoretically we could just move these to the downloads screen in some manner instead, if the misalignment seems too great, but I feel like that's still a bit hidden, and if someone is just clicking download on a list of latest albums or favorite tracks, the relevant special download does probably match what they'd prefer?
There was a problem hiding this comment.
So I don't think that limiting the amount of e.g. recent albums really changes the semantics here, so that seems like a decent solutions and I'd definitely connect the section to the download.
Regarding the favorites, in the end the current favorite download collection would at least contain everything they wanted to download. I'm not sure if everyone will be fine with additional things being downloaded, but it doesn't seem too dramatic.
Although, how hard would it be to introduce subtypes of the favorite collection, i.e. adding the option to download "favorite tracks", "favorite albums", etc.? Seems like that would solve most of these problems, and could help if the device is low on storage. If that's reasonably easy to add, I think I'd make sense.
I agree about moving the special download to the downloads screen. That's actually still always the first place I go when trying to download my favorites, before I remember that I need to go into settings. That is pretty unrelated to the home screen sections though, since I'd always keep all special collections available somewhere (either settings or downloads screen), and then additionally add download buttons to specific settings. I that what you were thinking of too?
There was a problem hiding this comment.
Yeah, the special downloads need to remain somewhere, because wanting to use them is not directly tied to wanting to use the approximate home screen section. Regarding separating the favorites downloads, I guess theoretically it wouldn't be to crazy split them up and then maybe make the favorites download require the components, although I think the repair might actually complain about the structure of that? And that would break up our single unified request for non-artist favorites. I'm not sure how useful that break up would actually be though? When would someone be picky enough to only want favorited tracks or something, but not favorited albums, while still not wanting control enough to be doing individual downloads of items?
There was a problem hiding this comment.
I was thinking that favoriting genres and artists could be useful for filtering while browsing, but then people might only actually care about albums and tracks since those appear to be most often listened to as a unit. Could also be useful for downloading favorite tracks/albums in lossless quality and transcode everything else?
Just an idea though that could solve our current "problem". This could be done at a later point too.
| // Use small initial page to potentially reuse existing items | ||
| // TODO maybe check exists instead? Make home screen provider use full size pages? | ||
| // Does using small pages actually decrease home screen loading times? | ||
| int pageSize = i == 0 ? homeScreenSectionItemLimit : musicScreenPageSize; |
There was a problem hiding this comment.
I see where you're coming from with the question of using regular page sizes on the home screen. I do think it makes an impact, especially if there are many sections and the server is slow, so I would like to keep the smaller page size.
Since we need to rename the HomeScreenSectionConfiguration anyway, we could introduce a generic class (ItemListingSource or something similar), and then extend that with HomeScreenSectionConfiguration to be able to differentiate the two types of requests that way? Content-wise they're identical, but semantically they'd be different types.
There was a problem hiding this comment.
Or we just stick a bool in it or something. So yeah, we could make it so the config has a default size but it isn't included in equals so we immediately get a items from the other screen if possible. This would also require loadHomeSectionItemsProvider to return the actual used limit to determine if we've run out of items or just got a cached home screen page.
| // TODO optimize for fast response, like play all on home screen? | ||
| // Maybe we add a followup Future to PlayableSlice, and if we already have the starting item in cache (we should) | ||
| // then immediately return a slice with the rest in that future for the caller to add to queue later. |
There was a problem hiding this comment.
we should think about how much we want to do from this side vs. from the treadmill queue.
I guess these section item providers already allow keeping track of where we're playing from in what order and at which position, so I guess the queue could request additional items if needed, instead of adding just one more more page?
This could then also be applied to playlists, which sometimes run into queue or fetch item limits and such.
There was a problem hiding this comment.
Yeah, eventually we'd want all the queue methods to take generator objects, but all the treadmill queue stuff seems decently far off. You already have two stage loading for the home screen, so doing it for the track lists didn't seem too crazy, because this can take a couple seconds. Also, I didn't know playlist requests could actually fail due to being oversized? How big do they need to be for that to happen?
There was a problem hiding this comment.
Yeah we can absolutely do the two stage loading here, we should just keep in mind that eventually we want to make this even more general and detached from the original replaceQueue call.
Regarding playlist, it's not really about the request failing (although it could take many seconds), but more about the queue being too long for the native player to handle, especially on iOS. I've heard from several users that their playlists exceed 2k tracks and more. This can lead to crashes, and it seems like a virtual queue is the only remedy.
| }); | ||
| }, | ||
| ); | ||
| sortAndFilterController = SortAndFilterController.trackSettings(tabType: null); |
There was a problem hiding this comment.
Thanks for cleaning this up so well. I should've looked into how to properly use ValueNotifiers.
Here, I'm wondering if it would be better to have separate constructors for e.g. trackSettingsForTab(TabType) and trackSettingsForPlaylists()? Using null to differentiate them seems hard to read and not really extendable (e.g. for ordering list of playlists in the "Add to playlist" menu, where using an order that's different from the tab order and actual playlist tracks order seems useful). Should I update this to use an enum internally instead?
There was a problem hiding this comment.
Making the two groups of settings more explicit seems fine, if you prefer. Regarding extendability, if the order doesn't match the tabs or playlists, wouldn't you just use a non-tracking controller by default? Unless you've also added a separate sortOrder setting just for add to playlist, which I guess isn't completly unjustified, but feels potentially overbuilt. But anyway, feel free to tweak things - I havn't put a crazy amount of thought into stuff and mostly just wanted to rip more of the logic and special casing out of the UI widgets. You'll note that the requirement to separately provide a reactive isOffline bool to get the correct fallback is a bit awkward, and I also considered more directly using the sort controller on the playlist screen but was unable to due to the provider structure making it a bit awkward. I'm also wondering if we could achieve a similar functionality using a riverpod notifier in a way that integrates slightly more cleanly with the rest of the app. Although with the widget-specific nature of a controller, maybe that's just a counterproductive idea. But anyway, the point is feel free to mess around, because the optimal design is probably more tied the exact nature of how you want to handle sorting and saving the sortConfig than anything I'm considering.
|
Another thought I had a while ago about the fixed-width covers: That seems too obvious though, so I'm guessing there's something preventing us from doing that? Re-adding fixed-width grid tiles make sense regardless, especially for the horizontally-scrolling home screen sections. |
|
Debouncing in time is probably possible, and might be a reasonable solution. There's a related concept that I believe wasn't implemented when the fixed size tiles were first added, which is that the album images get clamped to a set of limited sizes. This could theoretically make images slightly less clear by making the image size and widget not precisely line up, but I don't know how visible this is in practice. This is currently used app-wide whenever you are in grid mode, fixed size tiles are off, and we expect resizes due to being on desktop or in splitscreen. The original fixed-size mode can look a bit awkward due to rescaling the gaps instead of the items, and was more of a quick technical solution than a feature, so it might be best to lean on either debouching logic or size brackets and just use the currently implemented, more standard scaling. The actual feature attached to the toggle is the change from count-based sizing to a set of sizing options, so I think we'd probably be fine just using the minor-rescaling logic for the size-based mode. I think the count-based logic does still make sense on fixed size devices to enable more precise control, so that does need to be reimplemented. You also need to actually connect up the size setting when in size-based layout mode. Regarding image limits, I'd first try out enabling the size clamping with the current logic and see if the effect is noticeable and if it can successfully limit resizes to a reasonable amount across resizes first, and then only if it seems insufficent investigate time-based debounce. I believe debounce would be easy to implement though, although it would also mean that every resize would still lead to at least one full image refresh, so go ahead and add it to the album image widget if the clamping seems insufficent for such a key usecase. If the scaling is fine but we're getting uneeded resizes with the clamping algorithm, we could also try a variant where we specifically tell the album image widget we're a size based grid tile, so it can just request an image at the average size for that size setting without relying on the dynamic clamping logic. |
- quick actions already implemented, section presets not yet - add adaptive grid for quick actions area
|
@Komodo5197 I thought about the fixed-size grid tile problem again. Essentially I had the following conclusions: At the same time, cross-axis count / "columns" is a very approachable way to reason about the size of mostly square grid tiles. It's very intuitive that showing 5 tiles next to each other on a phone screen will lead to pretty small tiles, while showing just 1 will lead to gigantic tiles. However, this of course only works for static screen sizes (mainly phones, also tablets if split-screen / floating app modes are rarely used). Using this cross-axis count for a resizable desktop app is pretty much useless. Regarding the dynamic resizing and cover re-feching issue that was up until now addressed by the fixed-size grid tiles: So all in all, I'd advocate for only keeping the "Grid Tile Size" setting, and removing the two other settings (fixed size, cross-axis count) in favor of that. There also wouldn't be a need for a landscape-specific settings anymore, since - as mentioned above - people most likely care about the size, not the count, and that size simply stays consistent between orientations. What do you think about that? Is there anything I haven't considered here? Do you think implementing this (i.e., passing the current tile size preset to the |
|
I do pretty much agree with this. I think the most notable loss from the proposed design would be the ability to have different album sizes in horizontal and vertical - is that something anyone would actually care about? Adding text with current cross-axis count would be basically as good as the current system, and would actually improve the desktop experience as well, because currently you just have to try all the sizes out to see what they actually mean. This new size setting would have to be much more granular to match the specificity of the current cross-axis settings. I think having a slider with no particular units where the user can just judge off the current cross-axis count makes sense. We might need some wacky math to have the slider cover a reasonable range of cover counts though. It might also make sense to let users directly type a value into the cross-axis preview field if it's still obvious the size is the persistent setting. Regarding actual implementing image caching, I think passing the size setting to the relevant AlbumImages in the grid makes sense. Then any album image with a size setting could save the physicalWidth and physicalHeight it calculates into its state, and just reuse them on rebuilds instead of updating them. And then we would add didUpdateWidget which would check if the size setting in the old and new widget differ and clear the cached sizing if needed. I don't think the setting getting changed out in the redesign vs stable really matters much, myself. If the new setting design is better we want it, and if it isn't we don't. Everything's getting pushed to stable eventually, and we need to implement a migration regardless. If it were up to me, I would have pushed the redesign to stable over a year ago, with the beta being an actual beta whose upgrades quickly flow through to main, and that wouldn't have affected my decisions on any of the changes since. This change needs to happen now because otherwise this PR contains a regression, and and I don't care about the timing otherwise. |
|
If it turns out that different sizes for horizontal/vertical orientations really is something people want (although I really can't see a reason for that), then we could just duplicate the setting for horizontal layouts later on, that should be trivial to do. It's a regression I'm willing to introduce here. As for preset sizes, there is a natural lower (upper?) boundary, which is one column / the cover taking up the entire horizontal width. In the other direction I'd probably make the slider values not too dynamic, because having different options between smaller and larger devices seems a bit odd. And I feel like smaller screens showing a slightly smaller UI in general is very common. I'll try to get this implemented now :) |
|
So this actually was simpler than expected (in part due to your pointers), and seems to work well. We'll probably need to manage tile sizes of the home screen separately, same as already planned for the grid text. This is because people might want a wall of tiny covers on the album tab, but probably not on the home screen. Edit: One thing I wasn't sure about yet is how to calculate the preset -> size mappings while considering the device/window size. Do we need to convert the current function into a widget, or manually pass device constraints into the function? |
|
I was imagining we would have the setting widget use a mediaQuery to watch the screen width, and then the saved value would just be an int pixel size without any processing needed. You currently seem to be going for some sort of fixed-size column-count hybrid setting based on how extreme the setting is, which seems like a strange design? |
| // If using grid music screen view without fixed size tiles, and if the view is resizable due | ||
| // to being on desktop and using split screen, then clamp album size to reduce server requests when resizing. | ||
| // only re-clamp if the image size has changed (state vars set to null), to avoid re-requesting images with slightly different resolutions | ||
| //TODO these conditions can probably be simplified to apply more universally |
There was a problem hiding this comment.
I'm not sure this clamping is actually needed at all anymore. It's basically only trying to handle the music screen grid, and we have a better method now.
| int? currentPhysicalHeight; | ||
|
|
||
| @override | ||
| void didChangeDependencies() { |
There was a problem hiding this comment.
I believe didUpdateWidget is more correct and also gives us the old widget so we don't need to save its preset ourself.
| double calculateItemCollectionCardWidth(BuildContext context, BaseItemDtoType itemType) { | ||
| return _itemCollectionCardCoverSize; | ||
| // return _itemCollectionCardCoverSize; | ||
| return switch (FinampSettingsHelper.finampSettings.gridImageSize) { |
There was a problem hiding this comment.
This should probably be using the provider.
There was a problem hiding this comment.
Via the ProviderContainer? Or should I be passing a ref?
There was a problem hiding this comment.
I'm passing the ref now (since otherwise this won't be reactive anyway), but that requires using a Consumer in some other widget. Not a big deal presumably?
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| ref.watch(finampSettingsProvider.gridImageSize); |
There was a problem hiding this comment.
I don't think this line does anything?
There was a problem hiding this comment.
I adopted it from the bitrate selector. Assumed it was so that if the setting should change externally for any reason, the widget will update?
I can get rid of it in both widgets though.
| static const volumeNormalizationMode = VolumeNormalizationMode.hybrid; | ||
| static const contentViewType = ContentViewType.list; | ||
| static const playbackSpeedVisibility = PlaybackSpeedVisibility.automatic; | ||
| static const contentGridViewCrossAxisCountPortrait = 2; |
There was a problem hiding this comment.
We're going to need some of these values if we want a migration instead of just resetting everybody to col5.
| /// This then corresponds to a specific amount of colums of the grid for the current screen/window size, which can be shown to the user | ||
| /// This is only a base resolution, allowing for some slight variations in window size that lead to scaling of images (without re-fetching from the server), to keep using the entire horizontal space. | ||
| /// If the scaling would exceed a certain threshold, the scaling would be inverted (e.g. from upper scaling threshold to lower scaling threshold), and the column count is adjusted. This ensures images always have roughly the configured size. | ||
| enum GridImageSizePresets { |
There was a problem hiding this comment.
I'm not sure this is better than just saving an int for pixel size. The col names are based off the max album size instead of the screen width, which I feel just makes the names confusing?
There was a problem hiding this comment.
So regarding this and your other comment about saving pixel values: My idea was that if the screen size is changed (mostly on desktop), then we should dynamically calculate a new size that exactly fits the horizontal space. If we save a pixel value and then make the window slightly wider, we'd either always fetch the wrong resolution and scale up, or we'd use the pixel value to calculate another pixel value that's close but fills the available space. So I'm not sure if that really offers a benefit?
Enums might be a bit easier to translate as well (if that is a concern at all).
I also don't see why the names here should be based off of the screen width. Are you saying they should all just equal some column count?
I like the idea of using the settings widget to fit the layout builder, although we'll probably need another layout builder somewhere else for the migration.
That layout builder should then replace the fixed base resolutions in the switch statement.
There was a problem hiding this comment.
The scaling is already fine. calculateItemCollectionCardWidth uses the setting enum and spits out a pixel value without any other input, so from the perspective of the widgets and image loading saving a pixel value would be equivalent. It would just allow us to save a more granular setting and have less intermediate code/concepts. For the enum names, I'm complaining that the use of names like col2 or col7 basically implies use of screen width, because what else would they mean? But actually, they're cols in an arbitrary 500px base length. So a name like width120 would make more sense, but at that point just store an int.
There was a problem hiding this comment.
I see. Let me give it a try!
| @@ -227,7 +228,15 @@ class HomeScreenQueueTile extends ConsumerWidget { | |||
|
|
|||
| /// This might calculate the width base on the device width in the future, or something similar | |||
| double calculateItemCollectionCardWidth(BuildContext context, BaseItemDtoType itemType) { | |||
There was a problem hiding this comment.
What's with these args? It doesn't look like we use either.
5 participants
Finally had the time to work on this. Finamp will soon have an actual home screen!
This closes #612.
The home screen is based the design mockup proposed in #220 (comment).
It will be user-configurable content-wise. The two rows of buttons ("1-click play actions" - top row, and "library links" - bottom row) will be configurable, as well as the "sections" below the buttons.
1-click play actions start playing something right-away (possibly after choosing an option, like the decade to play).
Library links open a place in the library. They are not meant for linking to specific items (e.g. albums), but it's not off the table yet.
The sections will contain a dynamic list of items. The items within a section will not be explicitly user-configurable, they will always be powered by some kind of request to the server (or downloads). The sections do however support displaying collections, which could be used to "pin" specific items if desired, while integrating neatly with existing Jellyfin features and allowing to sync the content across devices.
The basic home screen already works, only thing not functional yet are the "show more"/
>buttons and the "Decade Mix".Customization is not there yet, but will most likely be added before this PR is merged.
The new navbar is used on (almost) all other screens, it already works but the navigation experience still needs improvements.
Feedback and suggestions welcome!
Screenshots:
TODO:
Optional:
allow having separate home screens per library? or combining content from multiple libraries on the home screen (although that probably won't work with Finamp's reliance on "currentView")?maybe in a later update