Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix flutter_downloader and redesign the Menu #434

Merged
merged 27 commits into from
Dec 20, 2023

Conversation

Y0ngg4n
Copy link
Contributor

@Y0ngg4n Y0ngg4n commented Apr 14, 2023

@jmshrv it seems like

flutter_downloader switched the repository and the DownloadCallback works with an int now.
I hope this fixes the issues.

Edit: Also this Pull Request redesigns the Menu.
It now looks like this:

MenuRedesign.mp4

@Y0ngg4n Y0ngg4n changed the title Fix flutter_downloader and DownloadCallback Fix flutter_downloader and redesign the Menu Apr 16, 2023
@jmshrv
Copy link
Owner

jmshrv commented Apr 16, 2023

The download callback stuff should be fixed in main, I need to rebase redesign but once that's done it should fix the callback.

As for the design, I like the idea of a modal sheet. The current layout (in main) is getting a bit too big due to the amount of options

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Apr 16, 2023

@jmshrv yes was just not able to deploy without fixing it.

Ok nice. Wasn´t that easy to implement it :)

@Chaphasilor
Copy link
Collaborator

So should we go for the slide-up modal instead of the pop-over? I could adapt the current design to this style, but I'm not sure if we would gain a lot. The menu on the player page, with the added playback controls and (possibly) share button takes up a lot of space anyway.

Also, @Y0ngg4n does this also convert the menu on the Now Playing view, or only in the song list? Because the menu for the Now Playing view is the important one for the beta :)

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Apr 16, 2023

@Chaphasilor i am not a big friend of modal dialogs. so in my opinion the bottom sheet is better. Currently this is only for the menu in the list view. but it is no big deal to implement it for the player view too. I am currently working on the queue but the bezier curve button at the bottom of your design is driving me crazy XD

@Chaphasilor
Copy link
Collaborator

okay, no worries. You can leave out the curved shape for now, there were a lot of people who didn't really like it. Just add the button, and I'll think about it some more ^^

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Apr 17, 2023

@Chaphasilor ok so yesterday evening i have worked on implementing the menu to the player screen. It is more difficult than i thougt. I had to rewrite the state managment in the playerscreen. Currently it works, but it does not update the screen. So i think there is some callback currently not working. Have to check.

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented May 6, 2023

@jmshrv maybe could you check this PR i am a little bit stuck why the player screen does not get updated

@jmshrv
Copy link
Owner

jmshrv commented May 10, 2023

I'll look into it in a bit, I've been very busy recently so I haven't kept up on Finamp issues

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented May 10, 2023

@jmshrv yeah no problem. just being stuck.

@jmshrv
Copy link
Owner

jmshrv commented May 26, 2023

Had a quick look through the code, it's mostly good, just a few things:

  • The song info header seems to be rebuilt every frame
  • The imageProviderCallback in _SongInfo seems to be copied from the player screen, which sets the providers that should be for the player screen only. Since we only need the colour for the header itself, no provider stuff should be required
  • Your widgets appear to be backwards (properties and constructors after build). Minor issue, just threw me off a bit lol

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Jun 1, 2023

@jmshrv will check it the next weeks

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Jun 16, 2023

@jmshrv so i have removed the providers from the album image. How can i fix it, that song info does not get rendered every frame?
(Also fixed the backwards widgets)
Any idea why the menu in the player screen does not update?

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Jun 18, 2023

Ok so as i was to dumb to commit last time.... I had to rewrite everything again.
@jmshrv i have seen you changed the color generation. currently when you long press on the song_list_tile, the color stays white. i couldnt figure out how to initialize the imageprovider.
Maybe you can check it. I added a todo.

@jmshrv
Copy link
Owner

jmshrv commented Jun 18, 2023

Ah yeah that code is temporary, I've submitted a PR to palette_generator to do the multithreading within the package (flutter/packages#4132), so that mess can get removed from song_info.dart.

I'll have a look at your other questions at some point (as well as getting that PR merged). I'm on holiday right now so I don't have much time to be doing Finamp stuff, but thanks for being active on the project today :)

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Jun 18, 2023

Now it looks like this:

Menu.mp4

@Chaphasilor
Copy link
Collaborator

Nice, that's a good baseline to continue development from I think. On the player page I'm not a huge fan of another bottom sheet, because that's already used for the queue and would result in confusing interactions (does swiping up open the queue or the menu?). Right now there's also an unnecessary swipe up that's needed in order to show all options.
I was thinking about using the modal, but collapsing away options that aren't frequently used (customizable).

But I'm interested to hear why you're not a fan of modals dialogs? What are the issues with them? Maybe I'm missing something that makes bottom sheets (or something similar) more suitable :)

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Jun 19, 2023

@Chaphasilor yes i know i would like a popup button menu more too. It was just easier to implement it this way. If it works i will change it to popupbutton menu.

The nice thing of the bottom sheet is that you have this header where you can providr information about the currently selected item. i find that very useful in the song list.

@Chaphasilor
Copy link
Collaborator

Okay, nice. Not sure if I ever posted this, but the popover menu would also show the selected item:

image

(this is for an album, but it would be almost the exact same for a song :))

And we could think about adding swipe up/down gestures for closing the menu, in addition to the X-button or clicking outside the modal

@jmshrv
Copy link
Owner

jmshrv commented Jul 2, 2023

Ah ok, a lot of apps get that wrong lol. A more "elegant" solution is to make it so that half of the next item is out of view, so that it's obvious that it can be scrolled. Apple does a very good job of this in their apps:

Apple Maps screenshot, showing an option half-hidden

It's just about working in this PR, I'll test on some other devices.

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Jul 2, 2023

@jmshrv yes i know i tried it, but i did not get it to work. the problem here is that the buttons have no boundaries, if they would be elevated or something it would be far more remarkable that there is something cut of.

@Chaphasilor
Copy link
Collaborator

@Y0ngg4n could you resolve the conflicts so we can get this merged? Would like to bring this up to speed for the beta :D

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Dec 11, 2023

@Chaphasilor i can try

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Dec 12, 2023

@Chaphasilor i hope i didn´t messed the merge up

@Chaphasilor
Copy link
Collaborator

I'll take a look at it later. It's just some types that are wrong or missing parameters, not big deal hopefully. But thanks for doing the heavy lifting here! :D

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Dec 12, 2023

@Chaphasilor i can try to fix it

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Dec 13, 2023

Managed to fix it. There were some changes that got lost while merging and I tried my best to restore those :)
I'll do some more testing and try to merge it asap!

Edit: flutter_downloader is still giving some trouble, but worst case I just need to pin the version...

- looks like a button, overlays menu items and looks a bit out of place
- if people have trouble figuring out that the sheet can be scrolled, maybe an actual scrollbar like in the queue would help
- added blurred album cover to menu
- updated colors
- updated layout
@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Dec 16, 2023

@Chaphasilor you are the best :)

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Dec 17, 2023

Only thing missing now is adding some extra buttons when the menu is shown on the player screen, otherwise we lose the sleep timer :)
And the menu looks a bit different now...

Edit: buttons like "go to album" also seem to be missing (artist, genre). "Add to favorites" should probably be more at the top

@Chaphasilor
Copy link
Collaborator

Okay, this should be done now :)
@Y0ngg4n can you take a brief look at it to confirm that everything is working?

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Dec 19, 2023

@Chaphasilor looks good should work :) Thank you for your help :)

@Chaphasilor Chaphasilor merged commit 03d39af into jmshrv:redesign Dec 20, 2023
2 checks passed
@Chaphasilor
Copy link
Collaborator

Alright, it's merged. Thanks for all your work!

@Y0ngg4n
Copy link
Contributor Author

Y0ngg4n commented Dec 21, 2023

@Chaphasilor i have to thank you ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants