Refactor the drawing of TCO's; Get rid of hardcoded colors in TCOs; Even out the color scheme #2574

Umcaruje · 2016-02-16T21:53:27Z

Per HDDigitizerMusic#8

Fixes #2148

This PR does several things:

Fixes incorrect rendering of TCO's: they was a one pixel gap between the grid and the TCOs. Also fix the incorect rendering of the bar-lines on Instrument pattern TCO's:
Before & after
Adds a Boolean qproperty-gradient that allows themers to get rid of the hardcoded gradients on TCO's.
Removes all of the hard coded colors, so now you can change the color of a muted and selected TCO, as well as the shadow of the text.
Evens out the muted and selected colors, so they all are the same.
Implements missing muted foreground color for automation patterns.
All pattern text stays the same color when the pattern is muted, for better readability.

Comparison:

Old

New

New with gradients off

IvanMaldonado · 2016-02-17T00:38:17Z

@Umcaruje I don't have words to express my gratitude on the work you have done so far.
So many ideas for new themes with the new posibilites I can't wait for 1.2 to come out! Thanks!

Umcaruje · 2016-02-19T10:02:31Z

@zonkmachine @tresf @michaelgregorius can I get some testing/review on this one?

tresf · 2016-02-19T14:04:51Z

data/themes/default/style.css

 	qproperty-fgColor: rgb( 187, 227, 236 );
+	qproperty-fgMutedColor: rgb( 128, 128, 128 );


Of no implication to you... I'm not a fan of the fgFoo tag, personally because normally in css fg is synonymous with foreground which is synonymous with color which is synonymous with textColor. In this case, it's the gradient/overlay, right? Long term, I think we need to cleanup our naming conventions a bit, but you've at least kept it consistent. 👍

no, this is the color of the drawn notes/automation/waveform when a pattern is muted.

I was thinking about this. Maybe we can avoid a lot of this by using the background-color property in CSS. So, fgColor becomes color and color becomes background-color.

Also we could avoid the muted and selected properties by adding custom css pseudo class selectors like
AutomationPatternView:muted or AutomationPatternView:selected

Is :muted a valid selector? If so, that may certainly help. I'm glad you're thinking about it thought because we do need to cleanup and standardize the style.css eventually, and you've quickly become the most active dev looking at that file. 👍

tresf · 2016-02-19T14:31:34Z

Although you didn't write it, this draw code is bloated in my opinion. Although not fair to pick on you about it, this may be our best chance to clean it up and make it consistent. I left an example of what I mean here: #2574 (comment) but just about every draw function could benefit from some cleanup.

tresf · 2016-02-19T15:05:23Z

src/gui/AutomationPatternView.cpp

-
-			p.fillRect( QRectF( x1, 0.0f, x2-x1, it.value() ),
-								lin2grad );
+			if ( gradient() ) {


Although I quite prefer if () { to if () <newline> {, we should try to be consistent with the styling of the current file.

Ah, ok, will fix.

Umcaruje · 2016-02-20T00:31:02Z

@tresf I simplified the code, and also refactored it, so now all patterns draw in the same way/order. I also fixed the problem where the text was getting drawn over the pattern borders. I also commented the code in some places.

Inspired by #2579, I will implement static text drawing on the patterns, since their text doesn't change so often, which should speed up the gui.

tresf · 2016-02-20T15:37:00Z

src/gui/AutomationPatternView.cpp

@@ -21,12 +21,12 @@
 * Boston, MA 02110-1301 USA.
 *
 */
+#include "AutomationPatternView.h"


@Umcaruje moving this up like this... is this just a "THIS IS MY HEADER!!" convention? This is new to me.

Its in https://github.com/LMMS/lmms/wiki/Coding-conventions

The exception is that in a source file, the first included file should always be its header.

tresf · 2016-02-20T15:48:35Z

@tresf I simplified the code, and also refactored it, so now all patterns draw in the same way/order. I also fixed the problem where the text was getting drawn over the pattern borders. I also commented the code in some places.

I think this is in a very good state now. Since we use similar methods to draw it simplifies the code as well as the maintenance of it.

Inspired by #2579, I will implement static text drawing on the patterns, since their text doesn't change so often, which should speed up the gui.

Good catch.

Have we put any thought into consolidation? If we can consolidate the paint-event code, we may be able to make subclass-specific paint functions that pertain exactly to that particular class and handle everything else in the inherited class. Likely not worth it this time around, but I wanted to mention it.... if we can draw all Track objects the same and then tweak the drawing of each subclass for its own particular type, it would further simplify this. I've never done this in C++, so what I'm saying it purely hypothetical. 🍻

Umcaruje · 2016-02-20T15:59:54Z

Have we put any thought into consolidation? If we can consolidate the paint-event code, we may be able to make subclass-specific paint functions that pertain exactly to that particular class and handle everything else in the inherited class. Likely not worth it this time around, but I wanted to mention it.... if we can draw all Track objects the same and then tweak the drawing of each subclass for its own particular type, it would further simplify this. I've never done this in C++, so what I'm saying it purely hypothetical.

The thing is, in PatternView we don't just draw the pattern, we also draw the BB editor (which is bad imo, but I won't be fixing that). Also the drawing of the bar-lines also differs from pattern to pattern, and the only thing connecting them all is the drawing of the fill, borders and text. It might be a good idea to consolidate, but that is out of the scope of this PR.

tresf · 2016-02-20T16:01:06Z

👍

tresf · 2016-02-20T16:24:28Z

src/tracks/SampleTrack.cpp

 	{
 		p.setFont( pointSize<7>( p.font() ) );

-		p.setPen( QColor( 0, 0, 0 ) );
+		p.setPen( textShadowColor() );


👍 for fixing future code too.

Umcaruje · 2016-02-26T22:49:37Z

Update: QStaticText implemented. I also turned on antialiasing on the pattern text, which should make it a lot clearer (cc @musikBear). I also made the B&B patterns render to a pixmap like the other ones.
Also aligned up the sample tracks so they render perfectly in the center.

Comparison between old and new text:

What's left to do: refactor the bar-line drawing code and also make sample tracks render to a pixmap.

Umcaruje · 2016-02-26T23:54:48Z

@LMMS/developers I need a little help here. I reworked the BBTCOView to paint to a QPixmap, like PatternView and AutomationPatternView do, but I ran into a problem:

When I add a new TCO, it gets grayed out, as if some values are not initialized. The TCO gets normal color when I resize it, select it, or do any of the actions in the context menu. I made the BBTCOView behave the same way PatternView does (m_needsUpdate becomes true in the same spots), but it just won't behave the same way (maybe some signals and slots are missing).

Anyways, if someone here with a greater Qt Knowledge than me could help out, that'd be great.

tresf · 2016-02-27T01:54:06Z

Is there a chance your new code checks m_needsUpdate before it's ever initialized? It looks like the first draw may be getting missed because it's never set after first paint, but that's just a quick glance.

Fastigium · 2016-02-28T16:41:43Z

@Umcaruje To fix #2148, try adding this to the constructor of BBTCOView:

connect( _tco->getTrack(), SIGNAL( dataChanged() ), this, SLOT( update() ) );

Signals/slots after all, but not between Pattern and BBTCOView. Turns out Pattern's updateBBTrack() method I mentioned makes the BBTrack that the Pattern belongs to emit a dataChanged() signal, and it's no problem to connect a BBTCOView to its track. I was a bit misled because there was no emit keyword, but today I learned signals always emit when called, with or without emit before them 😊

Umcaruje · 2016-02-28T18:38:54Z

@Fastigium it works! Thanks a lot.

tresf · 2016-02-29T17:15:24Z

The diffs are outdated, so I'll reply in the PR comments directly...

virtual void is like that in Pattern.cpp as well

Sorry, my own misunderstanding of it. This article explains it: http://www.qtcentre.org/threads/864-diffrence-between-void-and-virtual-void, but it's fine as-is.

I don't quite understand. I should define a private member variable m_needsUpdate in TrackContentObjectView, and then make functions that allow the child classes to access that variable?

Yes, exactly. But, from a design perspective this would also assume you have a use for it inside TCOV by consolidating your common paintEvent code (well, assuming you encapsulate the variable... you could also simply make it public and expose it to all sub-classes).... and then each variation to paintEvent can first call the super-class first, then subsequent code (foo() would first call super::foo()) and then add any additional logic afterward. Of course, this conversation really is a duplicate of #2574 (comment), so feel free to tell me to stop any any time. 😄

Umcaruje · 2016-02-29T17:48:28Z

Well, I implemented it so that m_needsUpdate is now a member variable of TrackContentObjectView. I implemented getter and setter functions needsUpdate() and setNeedsUpdate().

This is pretty much where I draw the line. I'd want to work on some new issues now and this code is in a much better state than before. As I said previously, complete consolidation of the code is currently out of the scope of this PR :)

tresf · 2016-02-29T18:18:17Z

@Umcaruje well I don't think it harms to have an external API to force the redraw and agreed, out of scope.

I don't have a chance to compile and test this for a few days, so if someone else can give it the OK, that would be great.

tresf · 2016-03-01T21:09:21Z

Things look good from here. Good to merge. These Piano roll "notches" in the BBEditor... are they a new feature? I don't remember them before.

Also, unrelated to your PR, and I think already reported, but the clone pattern button doesn't impact the last BB track.

Umcaruje · 2016-03-01T22:51:36Z

These Piano roll "notches" in the BBEditor... are they a new feature? I don't remember them before.

They exist on the LMMS 1.1.3 I have installed on my PC:

Umcaruje · 2016-03-01T22:54:24Z

Oh, don't merge yet: I just noticed the mute icons are misaligned from your pic:

I'll fix that tommorow, and then this will be ready to merge :)

Umcaruje · 2016-03-02T20:54:09Z

The 'muted' pixmaps are now properly aligned, and also stay in place when you resize the track width. I also reduced their size to 14x14 so they don't clash with the text.

This is good to merge now 👍

@Fastigium

…ake TCO gradient configurable; Even out the color scheme Thanks to @Fastigium for helping with the BB Pattern redraw problem

tresf · 2016-03-02T21:02:01Z

👍

Refactor the drawing of TCO's; Get rid of hardcoded colors in TCOs; Even out the color scheme

IvanMaldonado · 2016-03-03T22:36:04Z

I know this is not the place to post this, but I didn't want to open a issue to inform a minor problem.

I noticed there is a property in the style.css that should be moved.

Umcaruje · 2016-03-03T22:48:48Z

I noticed there is a property in the style.css that should be moved

Why?

IvanMaldonado · 2016-03-03T23:11:28Z

I take every comment as a section, and the graphics that are changed by QToolButton are the same graphics changed by the properties under "all tool buttons" and it is currently under "smaller toolbars" besides QToolBar's properties.

Red: QToolButton
Blue: QToolBar

Umcaruje · 2016-03-03T23:16:32Z

Then the comment should be moved, and not the property. Cuz, you see, we are overriding the colors on hover and also the border and other stuff. If that property was moved where you suggest, the look of the buttons wouldn't change.

IvanMaldonado · 2016-03-03T23:27:10Z

My bad, I meant to say "the object", not "the property".

This is how it should look:

Umcaruje · 2016-03-03T23:30:41Z

No it shouldn't look like that, you just made the play and stop buttons square again...

Umcaruje · 2016-03-03T23:31:40Z

Anyways, this conversation is entirely pointless on this PR and has 0 relevance to it.

Issue a Pull Request or open an issue about this if you want, I'm locking this.

Umcaruje force-pushed the gradients branch 3 times, most recently from 4788c49 to 7d874fc Compare February 16, 2016 22:29

Umcaruje mentioned this pull request Feb 16, 2016

Remove Track Fade HDDigitizerMusic/lmms-alt-theme#8

Closed

tresf reviewed Feb 19, 2016
View reviewed changes

Umcaruje force-pushed the gradients branch 2 times, most recently from 0fe2e8f to d226c71 Compare February 19, 2016 21:01

Umcaruje added the in progress label Feb 19, 2016

Umcaruje force-pushed the gradients branch from d226c71 to 57df63d Compare February 20, 2016 00:24

Umcaruje changed the title ~~Get rid of hardcoded colors in TCOs; Make TCO gradient configurable; Even out the color scheme~~ Refactor the drawing of TCO's; Get rid of hardcoded colors in TCOs; Even out the color scheme Feb 20, 2016

tresf reviewed Feb 20, 2016
View reviewed changes

Umcaruje force-pushed the gradients branch from 57df63d to 08a726c Compare February 20, 2016 17:14

Umcaruje force-pushed the gradients branch 3 times, most recently from 6d7eb28 to d22cc82 Compare February 26, 2016 23:10

Umcaruje force-pushed the gradients branch 2 times, most recently from aa73d52 to db46bed Compare February 28, 2016 10:49

Umcaruje force-pushed the gradients branch 4 times, most recently from e545bfb to f96a4ea Compare February 29, 2016 15:51

Umcaruje force-pushed the gradients branch from f96a4ea to d1803d0 Compare March 2, 2016 20:52

Refactor the drawing of TCO's; Get rid of hardcoded colors in TCOs; M…

f136ba3

…ake TCO gradient configurable; Even out the color scheme Thanks to @Fastigium for helping with the BB Pattern redraw problem

Umcaruje force-pushed the gradients branch from d1803d0 to f136ba3 Compare March 2, 2016 20:58

Umcaruje added a commit that referenced this pull request Mar 3, 2016

Merge pull request #2574 from Umcaruje/gradients

020b4dd

Refactor the drawing of TCO's; Get rid of hardcoded colors in TCOs; Even out the color scheme

Umcaruje merged commit 020b4dd into LMMS:master Mar 3, 2016

Umcaruje deleted the gradients branch March 3, 2016 22:48

LMMS locked and limited conversation to collaborators Mar 3, 2016

		qproperty-fgColor: rgb( 187, 227, 236 );
		qproperty-fgMutedColor: rgb( 128, 128, 128 );

Refactor the drawing of TCO's; Get rid of hardcoded colors in TCOs; Even out the color scheme #2574

Refactor the drawing of TCO's; Get rid of hardcoded colors in TCOs; Even out the color scheme #2574

Conversation

Umcaruje commented Feb 16, 2016

IvanMaldonado commented Feb 17, 2016

Umcaruje commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf commented Feb 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Umcaruje commented Feb 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf commented Feb 20, 2016

Umcaruje commented Feb 20, 2016

tresf commented Feb 20, 2016

Choose a reason for hiding this comment

Umcaruje commented Feb 26, 2016

Umcaruje commented Feb 26, 2016

tresf commented Feb 27, 2016

Fastigium commented Feb 28, 2016

Umcaruje commented Feb 28, 2016

tresf commented Feb 29, 2016

Umcaruje commented Feb 29, 2016

tresf commented Feb 29, 2016

tresf commented Mar 1, 2016

Umcaruje commented Mar 1, 2016

Umcaruje commented Mar 1, 2016

Umcaruje commented Mar 2, 2016

tresf commented Mar 2, 2016

IvanMaldonado commented Mar 3, 2016

Umcaruje commented Mar 3, 2016

IvanMaldonado commented Mar 3, 2016

Umcaruje commented Mar 3, 2016

IvanMaldonado commented Mar 3, 2016

Umcaruje commented Mar 3, 2016

Umcaruje commented Mar 3, 2016