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

marquee text #754

Open
wants to merge 27 commits into
base: redesign
Choose a base branch
from
Open

marquee text #754

wants to merge 27 commits into from

Conversation

Decimate1405
Copy link

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

@Chaphasilor
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

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

@Chaphasilor
Copy link
Collaborator

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
Copy link
Author

Decimate1405 commented May 31, 2024

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
Copy link
Author

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
Copy link
Collaborator

Try adding the override from MarcelGarus/marquee#98 (comment) in pubspec.yaml :)

…that's a temp fix for marquee after flutter 3.2.2
@Decimate1405
Copy link
Author

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

@Chaphasilor
Copy link
Collaborator

Appreciate the effort! Those changes sound good.

Let me know when I should try this out!

@Decimate1405
Copy link
Author

its working, you can try it now :)

@Chaphasilor
Copy link
Collaborator

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
Copy link
Author

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
  1. If a marquee isn't needed, the fallback uses the BalancedText widget
Screenshot 2024-06-03 at 7 24 08 AM
  1. 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
  1. Marquee added in queue list
Screenshot 2024-06-03 at 7 27 15 AM
  1. Marquee added in track/album menu
Screenshot 2024-06-03 at 7 28 04 AM

@Chaphasilor
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

Chaphasilor commented Jun 4, 2024

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
Copy link
Collaborator

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
Copy link
Author

Decimate1405 commented Jun 4, 2024

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
Copy link
Author

Decimate1405 commented Jun 4, 2024

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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

2024-06-05-00-01-38.mp4

@Decimate1405
Copy link
Author

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
Copy link
Author

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

@Chaphasilor
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@Decimate1405
Copy link
Author

Decimate1405 commented Jun 13, 2024

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
Copy link
Collaborator

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

  • Make sure marquee text looks the same (font, vertical position, etc.) as a single-line title (i.e. this comment)
  • Add a new toggle in customization settings that controls if marquees should be used at all (can default to true). If disabled, overflowing text should be ellipsized like before
  • Move the existing "one line marquee" setting to the customization settings. That only has an effect if the previous setting is enabled. Maybe reword it as "allow one line break before using scrolling text" or something similar

Hopefully nothing major :)

…ttings and renamed it "Allow one line break before using scrolling text"
@Chaphasilor
Copy link
Collaborator

Hey, just saw you added commits to this. Is this ready to re-review? If so, would you mind resolving the merge conflicts? :)

@Chaphasilor
Copy link
Collaborator

@Decimate1405 I can resolve the conflicts and merge the PRs if you're busy. Just need your OK!

@Decimate1405
Copy link
Author

Hey @Chaphasilor ! Sorry I have been busy with things in personal life that I haven't been able to complete it, I still have to implement the default ellipses functionality and fix the centering a bit. I can complete it in the next 2 weeks and try to solve the merge conflicts as well

@Chaphasilor
Copy link
Collaborator

@Decimate1405 no worries! I just wanted to make sure the work you put into this will end up being used 😁
If you can finish it (at some point) that would be nice, but if you can't get to it, just let me know ^^

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