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

fixed broken 21pink LCD display #6914

Merged
merged 3 commits into from
Oct 15, 2023
Merged

Conversation

Rossmaxx
Copy link
Contributor

@Rossmaxx Rossmaxx commented Oct 2, 2023

fixes #6454

Apparently, the "pink" lcd on the default theme is purple and it is barely visible with the dark background. I changed it to use an actual pink color.

before :
image

after :
image

fix suggested by @DanielKauss

@Veratil
Copy link
Contributor

Veratil commented Oct 2, 2023

@RebeccaDeField

@RebeccaDeField
Copy link
Contributor

@Veratil I don't think having a pink color to improve the contrast is a problem. There was a time that we spent way too many thread messages and issues trying to get the purple light enough, so given that it's still blending into the background, it's probably not going to get bright enough as purple.

I haven't used LMMS in a while so what needs to be tested is making sure that every area this effects is taken into account in case the color change will also modify colors unelated to the LCD. We will also need to see how this looks on several plugins.

Lastly, I don't know if we still take into account general public opinion because small tweaks can cause some strong reactions, so it might not be a bad idea (or might be) to get a few thoughts from a few other users about a particular color and how it blends in with the rest of the program, especially if they are more familiar with the program than I am atm.

My thought is that if it gets the job done and takes away one contrast issue, then go for it 👍

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Oct 3, 2023

We will also need to see how this looks on several plugins.

Afaik, the 21pink lcd is used only in sf2 player (and SlicerT once it gets merged). Correct me if i miss out any plugin.

don't know if we still take into account general public opinion

That's still the case but things get merged pretty fast these days because we tend to agree on stuff more compared to the past.

The more eyes the better.

@superpaik
Copy link
Contributor

I like it better now. I think it's more in line with the "green" lcd

@tresf
Copy link
Member

tresf commented Oct 8, 2023

When did change from green?

image

@Veratil
Copy link
Contributor

Veratil commented Oct 8, 2023

@tresf
Copy link
Member

tresf commented Oct 8, 2023

f7b160d#diff-13236858ad549aea6331ae6486fdaa11dbd9d73d03be0e628abf632415d13ac7

No, in that commit it's pink, not purple and using the older LCD style artwork.

@Veratil
Copy link
Contributor

Veratil commented Oct 8, 2023

f7b160d#diff-13236858ad549aea6331ae6486fdaa11dbd9d73d03be0e628abf632415d13ac7

No, in that commit it's pink, not purple and using the older LCD style artwork.

Oops, you're right. Here it is. 3f2e17d#diff-4d6d01e3874566c5ebc59ff14cf5c36e0a4a456eb44277d6e5a6a83ed6945c10

@tresf
Copy link
Member

tresf commented Oct 8, 2023

Yeah but it hasn't been working. My screenshot is from stable.

@Veratil
Copy link
Contributor

Veratil commented Oct 8, 2023

Yeah but it hasn't been working. My screenshot is from stable.

You're right. I just checked 1.2.1 as well and it's using green there, too.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Oct 8, 2023

It's ok to change to green, but imo, the current pink suits the ui better than green.

@tresf
Copy link
Member

tresf commented Oct 8, 2023

My guess is that it was inadvertently fixed in #5865. Yeah, the pink is fine, it's just an odd one to have been broken for so long and then resurrected and then turned into a bug report.

@tresf
Copy link
Member

tresf commented Oct 8, 2023

We will also need to see how this looks on several plugins.

@RebeccaDeField Only SF2 and GigPlayer uses this particular "pink" LED style.

Here's GigPlayer with this PR:

image

My thought is that if it gets the job done and takes away one contrast issue, then go for it 👍

I sort of agree. I feel that GigPlayer doesn't really have enough of it's own visual style to warrant NOT merging.

@sakertooth
Copy link
Contributor

If we are all happy with the contrast change, I say merge?

@zonkmachine
Copy link
Member

If we are all happy with the contrast change, I say merge?

Sure. That's a whole lotta pink. I actually prefer the original green but if people want pink then I guess pink it is.

@RebeccaDeField
Copy link
Contributor

I prefer to consolidate into one green if it's only used in two instances, but pink also works if it contracts well enough. Is it possible to consolidate even the separated definition for these LCDs and point it to just use the normal color or is that a pretty complex rabbit hole? From my perspective it's better if we have less unique colors to deal with.

I'm not strongly opinionated on this as the pink still has good contrast and is not offensive in any way.

@messmerd
Copy link
Member

messmerd commented Oct 14, 2023

If we go with pink, I think I'd prefer if the pink of the LCD display and the pink of the file open icon matched. Right now they are slightly different shades of pink and it bothers me a little.

Or maybe if they were more obviously and intentionally distinct from each other while both still being pink, that could also work? I'm not really sure - just putting ideas out there.

@RebeccaDeField
Copy link
Contributor

@messmerd You're right on that point. So I think we have two options and this can be merged upon one direction being completed:

  • Change icon to match and merge
  • Change icon to be a neutral color and LCD to be green and merge

@Rossmaxx
Copy link
Contributor Author

From this discussion, I just noticed that the pink shade in lcd box and the pink in file dialog is different. Will fix it now.
Also, using green is ok but I don't feel like it matches the theme of sf2player. So I feel it's better to use pink here.

@zonkmachine zonkmachine merged commit 732e5cc into LMMS:master Oct 15, 2023
@Rossmaxx Rossmaxx deleted the fix-21pink branch October 15, 2023 13:06
@RoxasKH RoxasKH mentioned this pull request Oct 19, 2023
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.

Bad selection of colors on SF2 UI
8 participants