jmshrv / finamp

A Jellyfin music client for mobile
Mozilla Public License 2.0
1.67k stars 117 forks source link

marquee text #754

Open Decimate1405 opened 1 month ago

Decimate1405 commented 1 month ago

Implemented marquee text in song menu for overflow, instead of two lines

Chaphasilor commented 4 weeks ago

Hey, thanks for the PR. Sorry for not responding sooner, but I'll take a look now.
I'm not a huge fan of scrolling text myself but I'm happy with making this configurable via a setting.
Is there a particular reason why you didn't use an existing package, like marquee? There is also an existing solution (without a PR so far) at https://github.com/Ivanf1/finamp/tree/marquee that implements this for the player screen as a fallback if the two lines are not enough.

If you could update this PR to behave in a similar way (using up to two lines by default, and then switching to scrolling text) that would be nice. If you don't like the line break and would like to always keep it at a single line, that could be added through a setting!

Also, it seems like you removed the track title on the player screen?

Decimate1405 commented 4 weeks ago

Hey, thank you for the response. I tried using marquee package but it was giving an error during import (some conflict with fading_edge_scrollview package) that's why I implemented it myself. I think adding it as a setting would be really nice as it would accommodate for everyone's needs. Regarding the title being removed from the player screen, looks like I accidentally forgot to remove the comment.

Decimate1405 commented 4 weeks ago

added a toggle to switch between 1 or 2 lines of song title, defaults to 2. its under Player settings.

Chaphasilor commented 4 weeks ago

Okay, thanks for the changes. There's still a lot of commented-out code, that should be removed.
Could you also try integrating the marquee package again, instead of using the custom implementation? Or is there a specific benefit to your implementation?

Decimate1405 commented 4 weeks ago

There's no specific benefit to using my implementation. I couldn't fix the errors i was getting while using the marquee package so i got fed up and implemented it myself. I'll give it another go

Decimate1405 commented 4 weeks ago

This is the error I keep getting when I use the marquee package in the now playing bar.

Screenshot 2024-05-31 at 2 47 03 PM
Chaphasilor commented 4 weeks ago

Try adding the override from https://github.com/MarcelGarus/marquee/issues/98#issuecomment-2119314341 in pubspec.yaml :)

Decimate1405 commented 4 weeks ago

changed it to use the marquee package, still defaults to 2 lines

Chaphasilor commented 4 weeks ago

Appreciate the effort! Those changes sound good.

Let me know when I should try this out!

Decimate1405 commented 3 weeks ago

its working, you can try it now :)

Chaphasilor commented 3 weeks ago

Okay, took a look at the changes just now, here's what I noticed:

Marquee text color on the now playing bar doesn't adapt to the theme: image

If a marquee isn't needed, the fallback isn't using the BalancedText widget: image Here's what it should look like: image

With the one line marquee enabled, even non-overflowing text uses a marquee on the player screen: image

The marquee should probably also be used in the queue list: image (code is very similar, so it should be easy to add)

And maybe also in the track menu: image

And finally, one suggestion. Do you think you could add a mask to the marquee on the player screen, so that it fades out at the sides instead of simply being cut off? You can take a look at the headers within the queue list as an example. Here's what I mean: image

Overall, the change is nice, and once the kinks are ironed out, I believe this will be an awesome new feature!
I'll probably add a commit that updated the settings title and descriptions to be a bit clearer, but that's it :)

Decimate1405 commented 3 weeks ago

thanks for the feedback. i'll work on the changes

Decimate1405 commented 3 weeks ago

made all the changes mentioned:

  1. Marquee text color on the now playing bar adapts to the theme

    Screenshot 2024-06-03 at 7 23 03 AM
  2. If a marquee isn't needed, the fallback uses the BalancedText widget

    Screenshot 2024-06-03 at 7 24 08 AM
  3. Fixed the bug where with the one line marquee enabled, even non-overflowing text uses a marquee on the player screen

    Screenshot 2024-06-03 at 7 29 15 AM
  4. Marquee added in queue list

    Screenshot 2024-06-03 at 7 27 15 AM
  5. Marquee added in track/album menu

    Screenshot 2024-06-03 at 7 28 04 AM
Chaphasilor commented 3 weeks ago

Taking a look now, I missed your comment, sorry about that.

By "track menu", I meant the menu that opens when tapping the "hamburger icon" or three dot icon, with options like "add to playlist", "go to artist", etc. From your screenshot it seems like you added the marquee to the track list within albums and playlists, which is probably a bit overkill...

Decimate1405 commented 3 weeks ago

oh, I thought you meant track/album menu which did seem a bit off to me. i can move it to the hamburger menu

Chaphasilor commented 3 weeks ago

Also, the text color on the now playing bar is still wrong in light mode. It should be white there as well, as before. So in a way you could say it should not adapt to the theme :P

image

Edit: same issue on the queue list for the current track: image

Chaphasilor commented 3 weeks ago

I also think there's no need to add marquees for all the other tracks in the queue list. Just the currently playing track should be enough, otherwise there's too much movement on the screen. But I'd like to hear your opinion.

Decimate1405 commented 3 weeks ago

i do agree, when i was testing it out having marquee in the queue list seemed very off because none of the tracks are synced because of their title length. I only implemented it because I thought that's the change you were referring to

Decimate1405 commented 3 weeks ago

Just to confirm, this text should always be white regardless of system theme right? Dark theme

Screenshot 2024-06-04 at 5 49 17 PM Screenshot 2024-06-04 at 5 50 40 PM

Light Theme

Screenshot 2024-06-04 at 5 49 52 PM Screenshot 2024-06-04 at 5 50 18 PM

and kept marquee only in current playing track in queue list

Screenshot 2024-06-04 at 5 51 28 PM
Chaphasilor commented 3 weeks ago

Yes. That's how it should be :)

I also noticed that the height required by marquee text and two-line wrapped text isn't equal. Previously I made sure that one line and two line titles had the same height (single line was centered vertically), so that so layout shift between tracks happens.
Can you make the marquee on the player screen have the same height?

(tried to record a video of the problem, but my screen recorder doesn't like me)

Decimate1405 commented 3 weeks ago

oh ok. yea I intentionally changed the height between one line and two line because i thought it would look cleaner. but i can keep it the same

Chaphasilor commented 3 weeks ago

Also, please add a "Scroll text instead of truncating" setting to the customization settings at, it can default to be enabled. You might also wanna move the other setting over there, since it doesn't only affect the player screen anymore :)

Chaphasilor commented 3 weeks ago

Okay, here's the layout shift I was talking about:

https://github.com/jmshrv/finamp/assets/18015147/7bf035cc-33bc-4082-99dd-0d6b59cd4a37

Decimate1405 commented 3 weeks ago

added marquee in hamburger menu track title (kept the same text style as before)

Screenshot 2024-06-04 at 6 03 46 PM

Also, please add a "Scroll text instead of truncating" setting to the customization settings at, it can default to be enabled. You might also wanna move the other setting over there, since it doesn't only affect the player screen anymore :)

do you mean I rename "One Line Marquee" to "Scroll text instead of truncating" and move from Player screen settings to Customization settings?

Decimate1405 commented 3 weeks ago

is this layout fix good or should i change something? Screen_recording_20240604_181544.webm

Chaphasilor commented 3 weeks ago

do you mean I rename "One Line Marquee" to "Scroll text instead of truncating" and move from Player screen settings to Customization settings?

No, add a new setting that controls if a marquee is used at all (or if the previous behavior with the ellipsis is always used) and move the other setting

Or you could try to combine those two settings in a select/dropdown list.
That could be "Handle too-long text by..." and then three options: "truncating" (old behavior), "scrolling" (one-line marquee), "wrapping and scrolling" (two-line marquee).
But I'm not sure if that's really a good idea, the two separate settings seem easier for now :)

Chaphasilor commented 3 weeks ago

Layout change is almost perfect, but the marquee text should be at the same position as the non-marquee single-line text. Here you can see they are not quite at the same height:

Chaphasilor commented 2 weeks ago

@Decimate1405 are you still willing to work on this? I can handle the merge conflict for you, if that helps?

Decimate1405 commented 2 weeks ago

Yes, last few days have been quite busy but i can continue working on it this weekend. Could you just summarize what other features are needed. The last change I made was the layout fix

Chaphasilor commented 2 weeks ago

That's great to hear!
The changes I'd love to see are:

Hopefully nothing major :)