Implements #2635: Draws a transparent rectangle in the loop area (plus CSS exposure) #2657
Conversation
|
Fixes #2635. |
michaelgregorius
changed the title
|
This also fixes #2259 |
|
Actually you can replace this: QColor bg_color = QApplication::palette().color( QPalette::Active,
QPalette::Background );
QLinearGradient g( 0, 0, 0, height() );
g.setColorAt( 0, bg_color.lighter( 150 ) );
g.setColorAt( 1, bg_color.darker( 150 ) );
p.fillRect( 0, 0, width(), height(), g );with this: p.fillRect( 0, 0, width(), height(), p.background() );and define the |
src/gui/TimeLineWidget.cpp
Outdated
| } | ||
|
|
||
| // Draw the loop rectangle | ||
| int loopRectMargin = 2; |
There was a problem hiding this comment.
shouldn't loopRectMargin and loopRectHeight be const too? Also, I think it would be better if the margin was 1 or 0 pixels.
|
@michaelgregorius Tact is not the correct word in English. Unfortunately it's used internally in the code, but in user facing things such as themes they should be called bars. |
|
The code looks fine, and I made some observations from the screenshots. I have yet to compile it and test it out. I actually wanted to start working on this issue today, but you ninja'd me 😉 Next time, you should assign yourself, so we don't start working on the same thing. |
|
Tested, it works for me 👍 OS: Manjaro 15.12 Capella
|
|
I've noticed we have lost some vertical space here:
|
|
Yup, that looks abnormally large... |
|
Fixed by specifying the max-height of the widget as well. I have also learned that you can specify the font size only in The height of the time line widget is still specified in |
|
@michaelgregorius Thank you for fixing it, yet I don't understand why this is like that now.
|
|
@IvanMaldonado What exact do you mean? I only see a themed version of the Piano Roll editor. |
|
@michaelgregorius Makes more sense now?
|
|
@IvanMaldonado Ah, ok. Yes, it was changed because the old Piano Roll window was too wide on smaller resolutions. Please check #2287 for more details. |
|
@michaelgregorius I see... I don't know how I never noticed it but now, I'm sorry for referencing that here. Thank you for working on this one! 👍 |
| If you want a fixed size set min and max to the same value. */ | ||
| min-height: 1.5em; | ||
| max-height: 1.5em; | ||
|
|
There was a problem hiding this comment.
You want to add this here:
background-color: qlineargradient( x1: 0, y1: 0, x2: 0, y2: 1,
stop: 0 #8796a7, stop: 1.0 #3e454e );to reinstate the old look.
Also, shouldn't
TimeLine {
font-size: 8px;
}be removed?
There was a problem hiding this comment.
I have reinstated the old look. I have to admit that I like the flat look better but that's something for theme designers I guess. :)
I have also removed the TimeLine entry from the style.css.
|
Ok, compiled and tested, and the numbers look a bit blurred on my non-high DPI monitor: Could you force hinting on them? Here's how I did it for TCO's: Lines 1177 to 1180 in 08847cc |
|
I have added hinting in 47e7b7d. |
|
Can this be merged? |
Don't rush it please, I have a couple of remarks left, just haven't had the time to write them. |
src/gui/TimeLineWidget.cpp
Outdated
| int const loopEndR = markerX( loopEnd() ) + 9; | ||
| int const loopRectWidth = loopEndR - loopStart; | ||
| p.setPen(Qt::NoPen); | ||
| p.setBrush(loopPointsEnabled() ? getActiveLoopColor() : getInactiveLoopColor()); |
There was a problem hiding this comment.
You don't have space between parentheses on these 3 lines, while the rest of the file looks differently. It'd be nice if you changed it for consistency.
|
@michaelgregorius this all looks good, but the numbers appear off-center on my machine. The numbers also still look weird to me, but that's the issue of what my default font is. Ideally, we should probably choose a font to force on the app, so its consistent across all platform, but that's a completely different issue and out of scope of this PR. Otherwise, this looks and works fine, so when you address these 2 minor issues, I'll merge 👍 |
|
Isn't the contrast a bit too low? When quickly looking at that screenshot @Umcaruje posted I can barely see a difference. |
|
@grejppi that was a screenshot with the loop points disabled. Here's how it looks with them enabled: |
|
@Umcaruje Done! Here's how it looks on my machine now: I think it's hard to get it perfect because you have to use font metrics and they might give different numbers for different fonts. For example the numbers of a font might not make use of the full ascent. |
|
@michaelgregorius @Umcaruje TBH the active colour is too dim. That would still work well as the inactive colour though. |
I believe that by removing the borders and using either #1C4933 if you want it to be more subtle or #21A14F if you want it to be brighter will work. It seems from reading over this thread that people preferred the 1px padding so I think that should be left in. I don't know if it's possible, but a bit of transparency would have a nice effect. I can mock it up to see what it would look like if need be. |
The color for the rectangle can be defined in the style CSS for the active and inactive case. Some other properties of the TimeLineWidget are now exposed through the CSS as well: - The color of the lines that are drawn at each tact - The color of the tact numbers - The font size - The minimum height of the widget It is therefore possible to change the height of the time line using the CSS. The tact numbers are drawn conditionally like the tact lines. The numbers are drawn with a constant distance to the tact line. This gives a more consistent picture at different zoom levels and also fixes the broken look at very small zoom sizes like for example 12.5%. Last but not least the order in which the elements of the time line are drawn has been adjusted. The loop markers are now drawn after the lines, numbers and the loop rectangle so that they overlay these elements.
Rename "tactLineColor" to "barLineColor" and "tactNumberColor" to "barNumberColor". Correct the loop rectangle's width which had offsets to the bar lines. Reduce loop rectangle margin to 1. Make everything const that can be made const in the paint method.
The height of the time line in the Automation Editor and Piano Roll was larger than the height in the Song Editor. This is fixed by specifying the max-height in the stylesheet as well. Also the font-size is now given in pt and the widget height in em so that the height of the whole widget will always scale proportional to the given height of the font.
Activates hinting when painting the bar numbers so that they show up less blurry on low DPI displays. Reinstates the old background for the time line widget. Removes the unused "TimeLine" from style.css.
Also some adjustments to parentheses.
Provide three properties to TimeLineWidget for the inactive and active loop mode: * A loop color: The color for the main rectangle's pen * A loop brush: The brush used to fill the main rectangle * An inner loop color: The color used for the pen that draws the inner border. Remove the pixmaps that have been used up to now to draw the loop boundaries.
Draw the inner fill first, then lines and bar numbers and finally draw the outlines (inner and outer).
Bring back the playhead marker which was removed accidentally with commit da21e36.
Add a new property loopRectangleVerticalPadding which specifies the padding used for the loop indicator rectangle. Initialize the loop rectangle members in TimeLineWidget. Document the style sheet properties for the loop indicator rectangle.
michaelgregorius
force-pushed
the
2635-loop-area-rectangle
branch
from
13c36f7 to
e8b8f66
Compare
|
@Umcaruje I have rebased against the current master. I have redone the CSS changes for the now classic theme and have also removed the loop marker bitmaps there. The current look for the new default theme is as follows: @IvanMaldonado: I'd rather have this one integrated first and then we can later check for further improvements. Otherwise these pull requests become never ending stories. |
|
@michaelgregorius The green does look a bit cloudy. My guess is that you are using a gradient that goes from white to completely transparent on top of the green? If so, the reason the color looks off is because the overlay makes the color grayer, so changing it to a gradient that is a lighter green to a dark green or removing the gradient all together might fix that.
When designing this theme, I only used gradients where I felt it was absolutely necessary and I personally didn't see a reason why all of the editors such as the song editor and the automation editor did not have gradients in the grid when the timeline did. I know these are different sections of the program, but I like to keep things consistent. When @Umcaruje removed the hard coding for the gradient, some people noted that it even looked nice with the old theme (but didn't match as that theme uses gradients on everything) and I noticed that other people are using the timeline without gradients in their themes as well. I also did some research and noted that many other DAWs don't use gradients in the timeline. I would prefer to leave the gradient out in that section, but this is Open Source and I don't mind if people make changes that I'm not entirely in agreement with as this is also the default theme. I would also like to hear what some other people in the LMMS team think? :) Thanks for working so hard on this! |
|
IMO, the flat timeline fits better with @RebeccaDeField 's theme. I think maybe it would be good to add yet another exposure to CSS that can control whether gradients are enabled. |
|
I also agree that the timeline would look a lot better when flat. Good job on the rebasing @michaelgregorius 👍 |
|
@RebeccaDeField No need to thank me. Most of the hard work for the new theme was done on your side anyway. I have adjusted the CSS to use no more gradients and borders as proposed by @RebeccaDeField. I have removed the borders and have used the less subtle I propose to merge this pull request and to do some potential remaining changes in other commits/pull requests. @liushuyu The classic theme uses a gradient for the time line so it should already be possible to use gradients via CSS. |
|
@michaelgregorius Before we merge, can we adjust the inactive color to #3B424A? The current color has a slightly different hue and saturation than the gray on the bar that is is sitting on, so it clashes a bit. A good way to get the right shade is to color-pick the main color in the windows and just change the value (or lightness/darkness.) This can always be done after the merge if need be. |
|
@RebeccaDeField I have added a new commit that changes the color of inactive loops as proposed. |
|
@michaelgregorius Thanks, I think we can make any additional changes later as you said. |
|
I will merge this one in the next few days if there are no objections. |
|
@michaelgregorius sounds fine to me 👍 Just remember to squash and merge on github (When merging the PR, you have a dropdown menu next to the merge button where you can select that). |
|
👍 |
…(plus CSS exposure) (LMMS#2657) * Draws a transparent rectangle in the loop area (plus CSS exposure) The color for the rectangle can be defined in the style CSS for the active and inactive case. The following properties of the TimeLineWidget are exposed through the CSS: - The color of the lines that are drawn for each bar - The color of the bar numbers - The font size (given in pt) - The minimum and the maximum height of the widget (given in em so that it scales with the font size). Set both to the same value to set a fixed size. - The background of the widget - A loop color: The color for the main rectangle's pen - A loop brush: The brush used to fill the main rectangle - An inner loop color: The color used for the pen that draws the inner border. - loopRectangleVerticalPadding: specifies the padding used for the loop indicator rectangle. The bar numbers are drawn conditionally like the bar lines. The numbers are drawn with a constant distance to the tact line. This gives a more consistent picture at different zoom levels and also fixes the broken look at very small zoom sizes like for example 12.5%. The bar numbers are drawn with hinting so that they show up less blurry on low DPI displays. Remove the pixmaps that have been used up to now to draw the loop boundaries. Removes the unused "TimeLine" from style.css. Document the style sheet properties for the loop indicator rectangle.
7 participants
The color for the rectangle can be defined in the style CSS for the
active and inactive case. Some other properties of the TimeLineWidget
are now exposed through the CSS as well:
It is therefore possible to change the height of the time line using the
CSS.
The tact numbers are drawn conditionally like the tact lines. The
numbers are drawn with a constant distance to the tact line. This gives
a more consistent picture at different zoom levels and also fixes the
broken look at very small zoom sizes like for example 12.5%.
Last but not least the order in which the elements of the time line are
drawn has been adjusted. The loop markers are now drawn after the lines,
numbers and the loop rectangle so that they overlay these elements.
Inactive loop:
Active loop:
Small zoom:
