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

Added automatic dark note on light background for midi clips. #6539

Merged
merged 5 commits into from
Sep 17, 2023

Conversation

aakers24
Copy link
Contributor

Closes #6526

@Veratil
Copy link
Contributor

Veratil commented Oct 11, 2022

Thanks! Can you share a screenshot of it working for various colors both light and dark?

@aakers24
Copy link
Contributor Author

MidiClipScreenshot

@Veratil
Copy link
Contributor

Veratil commented Oct 11, 2022

I feel the muted clips contrast on grey isn't as good as it could be. Not sure it's a problem, but what would happen if we tried to make those notes get slightly lighter? It would have an effect on the rest, but curious how much.

@aakers24
Copy link
Contributor Author

image
How does this look? I feel like it's more visible, but less obvious that they're muted. Maybe this is a happy medium? I played around with it and much more than this makes it hard for me to tell which are muted or not.

@allejok96
Copy link
Contributor

This is great. Personally I don't see an issue with the contrast, but if we change it, it should be done inside style.css and not hardcoded.

IMHO (not necessarily related to this PR) muted clips should always turn grey, no matter what color they have. Also when selecting clips they should always turn blue. Currently custom colored clips are just darkened when selected or muted... see ClipView::getColorForDisplay

@zonkmachine
Copy link
Member

I stressed through the review yesterday and had a second look.

The demo track Greippi - Krem Kaakkuja has colored tracks so it lends itself to comparing before/after this fix.
Here are eight screenshots side by side. As you can see from the bottom row, the unmuted notes also changes by this approach. I'd prefer the old old unmuted look and the new muted. Can we have both?

Top row ------- default theme - unmuted/muted, classic theme - unmuted/muted
Bottom row -- default theme - unmuted/muted, classic theme - unmuted/muted

guimutedoldclassicmuted

Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment #6539 (comment)

In b224f4d the colors of the unmuted tracks are now darker/less visible.

src/gui/clips/MidiClipView.cpp Outdated Show resolved Hide resolved
aakers24 and others added 2 commits September 16, 2023 18:56
@zonkmachine
Copy link
Member

Looks good to go! Caveat, I haven't tried it in any other themes than the ones we ship but this looks like a nice change to me.

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine, and works with the default themes. A more thorough implementation could use the luminance of the colour rather than the HSL lightness, and compare it to the luminance of the note colour rather than a fixed constant.

@zonkmachine zonkmachine merged commit 4348038 into LMMS:master Sep 17, 2023
9 checks passed
consolegrl pushed a commit to consolegrl/lmms that referenced this pull request Sep 27, 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.

Notes is barely visible if the Track color is light-colored
5 participants