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

Fix marked semitones in the piano roll #4239

Merged
merged 5 commits into from Apr 2, 2018

Conversation

Projects
4 participants
@Umcaruje
Member

Umcaruje commented Mar 12, 2018

Fixes #3651
Went with option #2 from the issue. @musikBear @RebeccaDeField
Screenshots:
image
image

The marked semitones now draw over the whole grid and are a transparent overlay. This will break any existing user themes for 1.2 but it's as easy to fix as adding transparency to your color.

@Umcaruje Umcaruje added this to the 1.2.0 milestone Mar 12, 2018

@Umcaruje Umcaruje added this to In Progress in Release LMMS 1.2.0-RC6 Mar 12, 2018

@@ -146,7 +146,7 @@ PianoRoll {
qproperty-noteBorders: false; /* boolean property, set false to have borderless notes */
qproperty-selectedNoteColor: #006b65;
qproperty-barColor: #078f3a;
qproperty-markedSemitoneColor: #06170E;
qproperty-markedSemitoneColor: rgba(255, 255, 255, 20);

This comment has been minimized.

@zonkmachine

zonkmachine Mar 12, 2018

Member

Something like this for classic theme too?

This comment has been minimized.

@Umcaruje

Umcaruje Mar 12, 2018

Member

I always forget about that, will fix

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Mar 12, 2018

image

Updated the classic theme, thanks @zonkmachine

@zonkmachine

Tested both themes. Works fine!

I haven't looked any deeper into the code. You draw the marking after the grid because it now has alpha?

@musikBear

This comment has been minimized.

musikBear commented Mar 13, 2018

Classic is actually more 'visible' -but this is so easy to customize with css, so anyone can get a personal version.
So @Umcaruje Great work with the css options!

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Mar 13, 2018

@musikBear I can bump up the visibility in the original theme if you want

@musikBear

This comment has been minimized.

musikBear commented Mar 14, 2018

@Umcaruje Not for my sake. I have made my own version of CSS, and thats working fine for me -but its more a question what other users think. @Mark-Agent003 reacts with 👍, so i think he should chime in, and say if he feel there are visibility issues. But all that super CSS-customizing you have made possible, is a huge help! Big 👍

@tresf

This comment has been minimized.

Member

tresf commented Mar 14, 2018

@Umcaruje Not for my sake.

Yes, for your sake this is an area you can help with.

But yes @Umcaruje please increase the contrast. I have new, bright monitors yet still I have to look pretty closely to find the highlighted sections in both classic as well as default themes.

0.4.15 for comparison:

image

@musikBear

This comment has been minimized.

musikBear commented Mar 15, 2018

Yes, for your sake this is an area you can help with

well what i did was to make my own idea, with black for marked notes, and a sea-blue for selected notes.
It looks like this:
mycsspianoroll

The sea-blue is not color-blind safe, so its really only a personal choice, thats work in Rebeccas design

@tresf

This comment has been minimized.

Member

tresf commented Mar 15, 2018

@musikBear oh you want to influence the note color too? Sounds reasonable but also sounds like a different request entirely. I was more inviting feedback on existing feature contrast, so @Umcaruje can decide whether or not to merge this.

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Mar 15, 2018

Well from the discussion in #3651 Rebecca said that black doesn't provide enough contrast, and another thing is that black makes all the other semitones look marked even though it should be the other way around. I'll bump up the transparency the marked notes in the default theme.

As far as the notes go it's the discussion for the other pull request.

@musikBear

This comment has been minimized.

musikBear commented Mar 16, 2018

oh you want to influence the note color too?

@tresf No, i just thought i might as well have both my CSS changes for piano-roll in the same picture, here the issue is the marked scale, but @Umcaruje says

Rebecca said that black doesn't provide enough contrast

So even though i find it easier to use, it is not for general usage, especially because it just as Umcaruje correctly points out, confuse new users, in respect to what is marked, and what isent marked -Eg black for marked, is contra intuitive (and should for that reason not be used :p )

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Mar 19, 2018

I'm trying with some colour on the classic theme. Here's a comparison with lmms-1.1.3
I also think it looks a bit better with the colour marking behind the notes but I'm not particularly invested in anything theme related so I'm fine with merging this as is with the current grey.

lmms-1.1.3/classic theme
theme113

lmms-1.2.0/classic theme with qproperty-markedSemitoneColor: rgba( 100, 255, 255, 60 );

theme120

@musikBear

This comment has been minimized.

musikBear commented Mar 20, 2018

I also think it looks a bit better with the colour marking behind the notes

I agree, it looks more '3D' like the note in ON the grid
Whit the colour marking over the note, the '3D' edges on the notes are eradicated, and it looks 'flat'

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Mar 29, 2018

@musikBear @zonkmachine @tresf thanks for the feedback, this is what I came up with:
screenshot from 2018-03-29 14 51 18
screenshot from 2018-03-29 14 55 04
Enough contrast or you feel it needs more?

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Mar 29, 2018

The classic theme is still a bit on the dim side though. Why the change to an overlay in the first place?

@musikBear

This comment has been minimized.

musikBear commented Mar 30, 2018

Its better, but its difficult to access the ergonomic effect, without working with real thing. Maby a set of css-files could be included default. Covering subtle to extreme contrast, and then users decide for them self.
But Umcaruje, you have spend seriously long time with this design layout. Isent everything css based? In that case it could be a 'junior-job' and be done by a non-ccp-coder?

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Mar 30, 2018

But Umcaruje, you have spend seriously long time with this design layout. Isent everything css based?

I spent a lot of time with these changes because of life getting in the way. Everything is CSS based, I changed the drawing method to an overlay here though rather than a change in the background.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Mar 30, 2018

Well, it looks good. I think it's a merge if you're happy with it.

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Mar 30, 2018

Well, it looks good. I think it's a merge if you're happy with it.

I'll tackle the classic theme a bit more and then merge.

@Umcaruje

This comment has been minimized.

Member

Umcaruje commented Apr 2, 2018

image

Here's the classic theme, made it to be closer to 1.1.3 in terms of color contrast.

@zonkmachine

This comment has been minimized.

Member

zonkmachine commented Apr 2, 2018

Merge...

@Umcaruje Umcaruje merged commit ee910d3 into LMMS:stable-1.2 Apr 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zonkmachine zonkmachine moved this from In Progress to Done in Release LMMS 1.2.0-RC6 Apr 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment