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

Refactor and improve song bottom sheet #1198

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

Lurux
Copy link
Contributor

@Lurux Lurux commented Jan 10, 2024

Intended changes:

  • Reorder menu items to be in a more logical order
  • Unify content of online, local, and player sheets
  • Move some advanced player option to a submenu
  • Use disabled buttons when artists or albums are unavailable instead of hiding them, so the menu stays consistent to improve muscle memory

Side effects:

When doing this I noticed the SongMenu, YoutubeSongMenu, and PlayerMenu were all essentially the same. They were refactored and merged into a single menu that takes a MediaMetadata as parameter.

The parts of the menu related to the current playback (aka. volume, pitch, etc) are displayed only in the player base on the inPlayer parameter.

This will reduce code complexity and ensure these menus are consistent moving forwards.

Side note: I only did the song menu for now because I don't love refactoring, but you can expect me to do stuff like this when I work on your project. I hope this helps !

@nvllz
Copy link
Contributor

nvllz commented Jan 12, 2024

Hi. What's the "Related" thing about? It's grayed out for all the songs I checked. I also miss a "Start radio" in the menu grid inside the player.

This PR would fix a mess I couldn't figure out for hours, when tried to map songs without any result..

I think making a different .kt file for a player song menu would be better than checking the state of "inPlayer" variable. I would then get a new menu based on metadata (man I spent literal hours and couldn't map songs basing on this.. eh) for queue items, and stitch it with my Queue Improvements PR, which would allow for a much better menu than the actual one (see my latest comment there).

For anyone wondering, the new song menu inside player looks as follows:
Screenshot_20240112_125006_InnerTune Debug

@Lurux
Copy link
Contributor Author

Lurux commented Jan 12, 2024

Hi. What's the "Related" thing about? It's grayed out for all the songs I checked. I also miss a "Start radio" in the menu grid inside the player.

Oh yeah sorry I forgot about that, it's related to another feature that I'm working on. Basically when showing the app around people complained it didn't have Youtube-style recommendations, so I intend this to take the results from "start radio", but show them as page that people can explore (similar to a playlist) instead of putting them in the queue.

Anyways, I had already added it to highlight the logic of the new button order when they show up as three columns, but I forgot to remove it before this PR.

As for the start radio button in the player, I didn't include it here because having it at the start would change the layout when opening the menu in the player, which is something I explicitly didn't want to do.

We can add it back to the hidden icons though, and/or this may change anyways if we decide to revamp the player at some point and we need to put more stuff in the menu (this is actually an idea that had been on my mind recently, as the current player seems a bit disorganized to me).

This PR would fix a mess I couldn't figure out for hours, when tried to map songs without any result..

When searching around while trying to understand the codebase, I stumbled upon some conversion functions in the models/MediaMetadata.kt file, but I have to admit I don't understand why there are have so many ways to represent videos, haha 😅

I think making a different .kt file for a player song menu would be better than checking the state of "inPlayer" variable. I would then get a new menu based on metadata (man I spent literal hours and couldn't map songs basing on this.. eh) for queue items, and stitch it with my Queue Improvements PR, which would allow for a much better menu than the actual one (see my latest comment there).

I think it's better to have at least the common part of the menu in a single file, that will avoid code duplication and potential inconsistencies in the future. I agree it would be better to separate the "normal" and "player" specialization parts though, I just didn't bother doing it here because I don't know if it's going to be merged.

@nvllz
Copy link
Contributor

nvllz commented Jan 12, 2024

I think it's better to have at least the common part of the menu in a single file, that will avoid code duplication and potential inconsistencies in the future. I agree it would be better to separate the "normal" and "player" specialization parts though, I just didn't bother doing it here because I don't know if it's going to be merged.

Sticking to this may only limit the possibilities. For queue song menu I'd like to have a button to play next, start radio would be useful too. This is really a PR that would save me like 10 hours total if you came up with this a month ago, hahah.. I can't imagine it won't even be considered merging. But I think the menu needs to be split among multiple files for beter handling. Having multiple 'if' clauses for player, playlist, queue and other sources of calling it will make the code not as readable as it may be when tailored to each menu needs. Or maybe there's some easier way to deal with multiple different layouts in a single file?

It will look so bad when applied all the if (isPlayer || isQueue && !isPlaylist) multiple times in different states, and we still need "Play next" when called from the queue, but not in the player ;/

@Lurux
Copy link
Contributor Author

Lurux commented Jan 12, 2024

Uh, I think the items in the queue can just use the same menu as in the library, can't they ?
Also, yeah, I'll find a better way to handle the modes.

@nvllz
Copy link
Contributor

nvllz commented Jan 12, 2024

For a song menu in a queue I'd prefer to also have the "Play Next" and "Start Radio" items. And a "Delete" for a song inside a playlist.

@Lurux
Copy link
Contributor Author

Lurux commented Jan 12, 2024

(I mean, the library menu do have those... (?))
Good point for the delete button in playlists, I didn't realize it was missing xD

@Lurux
Copy link
Contributor Author

Lurux commented Jan 22, 2024

Just so you know, I'm currently improving the menu so the code is much cleaner and the UI is further simplified, will convert to draft for now

@Lurux Lurux marked this pull request as draft January 22, 2024 21:41
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.

2 participants