Highlight feature for WidgetGroup #1068

daschuer · 2016-12-09T22:21:43Z

This PR introduce a new WidgetGroup, that allows to change the style according to a binary ControlObject, or by comparing an Index value.

The Shade skin demonstrates this behavior by using the [Microphone],show_microphone CO to change the effect background.

This should help to finish #1062

Usage details you find in src/widget/whighlightinggroup.h

timrae · 2016-12-10T01:39:38Z

src/skin/legacyskinparser.cpp

+}
+
+QWidget* LegacySkinParser::parseWidgetGroup(const QDomElement& node) {
+    WWidgetGroup* pGroup = new WWidgetGroup(m_pParent);


Why don't you use unique_ptr? It's generally considered bad practice to use naked new

We must not mix smart pointers with the Qt object tree, which manages the widget lifetime just right.

This widget is added to the object tree here:
https://github.com/mixxxdj/mixxx/pull/1068/files/9b7ac9a01c34812922b73f48ec328eba820b0433#diff-c2c84548a4801a7ca8649f1095e0bf36L601

Some time ago we have tested the object tree performance versus smart pointer solutions.
The object tree has won.

Thanks I didn't know about that

Be-ing · 2016-12-11T20:08:40Z

res/skins/Shade/effectunit.xml

-						<Size>170,90</Size>
-						<Layout>vertical</Layout>
+					<HighlightingGroup>
+      <Id>1</Id>


tabs and spaces are mixed, throwing off alignment on GitHub's web view

Be-ing

Thanks for implementing this. It generally looks good, although I am doubtful that changing a background image is how this should generally be used. What I had in mind was changing a border to indicate highlighting. It looks like this should be possible by using QSS with this.

Be-ing · 2016-12-11T20:15:15Z

src/widget/whighlightinggroup.h

+    Q_PROPERTY(int highlight READ getHighlight WRITE setHighlight NOTIFY highlightChanged)
+
+    // The highlightedId property is used to select the highlighted widget
+    // group by it's Id, which can be set from skin by <Id>4</Id>


typo in comment: "its"

Be-ing · 2016-12-11T20:19:55Z

src/widget/whighlightinggroup.cpp

+    WWidgetGroup::setup(node, context);
+
+    // Set background pixmap for the highlighted state
+    QDomElement backPathNodehighlighted =


capitalize "H" (backPathNodeHighlighted rather than backPathNodehighlighted)

daschuer · 2016-12-11T22:57:23Z

Thanks for implementing this. It generally looks good, although I am doubtful that changing a background image is how this should generally be used. What I had in mind was changing a border to indicate highlighting. It looks like this should be possible by using QSS with this.

Yes, changing the background in Shade is just the proof of concept that this works, even if color schemes are used. Using #MyGroup[highlight="1"] { } you can change any style-able property.

Be-ing · 2016-12-15T20:42:04Z

res/skins/Shade/effectunit.xml

-						<Pos>9,5</Pos>
-						<Size>170,90</Size>
-						<Layout>vertical</Layout>
+					<HighlightingGroup>


I find it confusing that HighlightingGroups are children of ordinary WidgetGroups. I think there should be a different name for the parent, maybe HighlightingGroupContainer?

Also, HighlightingGroup isn't a great name considering any QSS property could be changed. How about SelectionGroup and SelectionGroupContainer?

This case here is somehow random. A HighlightingGroup Group is just a Widget that can be highlighted. It can be placed at any place where you can place a WidgetGroup as well. It is just a WidgetGroup with highlighting features.
There is also no need to place other HighlightingGroup as siblings into a WidgetGroup. You can spread them all over the skin.

The word "selected" has already a Qt css meaning. I did not fell in love with the word "highlight", but I picked it, because it is not used in any default Qt style pseudo states, so it should be less confusing. We can pick any other name if you know a better unused one.

Be-ing · 2016-12-15T20:55:22Z

src/widget/whighlightinggroup.h

+
+    // The highlightedId property is used to select the highlighted widget
+    // group by its Id, which can be set from skin by <Id>4</Id>
+    Q_PROPERTY(int highlightedId READ getHighlight WRITE setHighlightedId NOTIFY highlightChanged)


highlightedId should be a property of the parent of the groups that could be highlighted. The groups that can be highlighted should have their own binary highlighted property.

Currently widgets, which are controlled by the same CO, are grouped by the same connection to its highlightedId property, not by its parent widget. This allows much more flexibility for the skinner since he can place highligted widget wherever he likes it.

Yes, that provides more flexibility, but to me it does not really fit the idea of selecting one member of a group. What do we really want to provide with this? A general way for skins to apply different styles in response to changes in COs? I like that idea, but I don't think the concept of a highlightedId makes much sense with it. An ID implies that it is one member of a group. Perhaps we should have a general way for a QSS-accessible property to mirror the state of a CO. For example:

in skin XML:

<WidgetGroup> <ObjectName>ExampleWidgetGroup</ObjectName> <Children> whatever widgets here </Children> </WidgetGroup> <Connection> <Connection>[Channel1],play</Connection> <BindProperty>connection</BindProperty> </Connection>

in skin QSS:

#ExampleWidgetGroup[connection="1"] { style to apply when [Channel1],play is 1 }

This will allow different styles to be applied for each state of a CO, not just 0 and 1.

If it was possible to change whether WidgetGroups are visible as well as their sizeHint and SizePolicy through QSS, that would be great.

Your example is already possible with this PR. Except that the property is not "connection" but "highlightedId"

I can easily think of examples where the HighligtingGroup can be used and the the cannot be in a common parent widget. Think of highlighting an Deck switch indicator or something .. or a solution where you can hide effect 3 + 4 or have a splitter between effect 1 + 2 and 3 + 4 or ...

Is there a confusion due to the ID member? Maybe the name is wrong. It is the common ID of all widgets that are highlighted if the connected CO has the ID value.

If you have two HighligtingGroups with ID=4 both are highlighted if the connected control has the value 4.

Yes, the choice of the name <Id> was confusing. I think that element is unnecessary and limiting. The way it is, the QSS can only react to a binary condition of whether the CO is at a specified value. The value of the bound CO should be accessible through QSS so the QSS can easily handle COs with more than 2 states. I think it would make more sense to make this a general capability of <WidgetGroup> rather than create a new element type.

Since is optional, you can use the ID, as shown in this PR or not like you describe.

The use-case for the ID value works like that:

Creat n HighligtingGroups and name them EffectBackground. Set the ID of the first HighligtingGroup to 1 and up to the n'th HighligtingGroup to n. Bind the highlightedId property of each to the same CO, wich can have the values 0 ... n

Now you can style all these groups with a single style like that:

#EffectBackground { /*default style*/ } #EffectBackground[highlight="1"] { /*higlighted style*/ }

Without that ID feature, each of the EffectBackgrounds need an individual name and an individual style. The color schema changing background will similar hard to use since you have to define a background for each of the N states.

I have made this feature separate, because it is more attracting for the user and a bit resource saving RAM/CPU. Remember that the majority of groups are happy with the default QT states.
But If you think it is a benefit to merge HighligtingGroups and WidgetGroups I can do it.
Should I?

Be-ing · 2016-12-15T20:57:16Z

src/widget/whighlightinggroup.h

+//  <Connection>
+//   <ConfigKey>[EffectRack1_EffectUnit1],single_effect_focus</ConfigKey>
+//   <BindProperty>highlightedId</BindProperty>
+//  </Connection>


The Connection should be on the parent, not the children.

Since each HighlightingGroup is independent, it needs its own connection.

rryan · 2016-12-16T09:05:39Z

I would prefer that we not introduce a dedicated widget for this -- I think we can already make it work without introducing new concepts. What about doing this with Transform and BindProperty? Set the highlight property with a BindProperty / Transform to massage the CO value into a boolean (e.g. matching against the selected effect) and then use QSS attribute selectors for the rest. WDYT?

daschuer · 2016-12-16T09:18:20Z

That would also work, but we need a new transformer type.
How about this:

<Transform>
    <Is>5</Is>
</Transform>

Be-ing · 2016-12-16T14:49:53Z

I think that's a good way to accomplish what we're seeking here.

We're treading close to pseudo-scripting-with-XML territory here. It seems there was a branch for QML skins years ago that was never merged. Are there any intentions to move to QML in the future?

rryan · 2016-12-16T17:12:32Z

Sounds good to me -- they aren't invertible so maybe we could record in the transform when it's not invertible and then print a skin warning if you use it in a bidirectional situation?

Would also be nice to have some more comparison operators e.g. Greater[Equal]/Less[Equal]

rryan · 2016-12-16T18:16:59Z

Are there any intentions to move to QML in the future?

From my perspective, definitely -- I have code that depends on #941 that introduces QML skins (and I've even gotten a very stripped down Mixxx running on Android).

All the features we added to the skin system in the past year or so (templates, resizing, etc.) were mostly to allow us to keep limping along and get the features we needed for 2.0 done. I'd like to ditch the qt-widgets skin system at the first opportunity :).

Be-ing · 2016-12-16T18:21:07Z

Cool, I've been thinking QML would be helpful for controller mappings to control how off screen rendering for controller screens could work. FWIW, that is what Traktor does.

daschuer · 2016-12-17T16:02:39Z

The transformer based solution is done.

A GreaterThan transformer and friend is not required, since we have the Add transformer, and the x > 0.0 boolean conversion schema which should be sufficient for these potential use-cases.

ywwg · 2016-12-17T16:15:24Z

QML skins

VERSION THREE POINT ZERO :)

rryan · 2016-12-17T17:03:04Z

Nice, thanks for the changes! code looks good to me

Be-ing

To clarify, the transform is not necessary, correct? If I understand correctly, without that, the new highlight property will be set to whatever the value of the bound <ConfigKey> is.

Please document this on the wiki and note it is new in 2.1

@rryan : there's a TODO for you on the wiki to document <Connection>

VERSION THREE POINT ZERO :)

+3!

Be-ing · 2016-12-17T17:35:55Z

src/widget/wwidgetgroup.h

@@ -35,17 +35,44 @@ class WWidgetGroup : public QFrame, public WBaseWidget {
    Q_PROPERTY(QRect layoutContentsMargins READ layoutContentsMargins WRITE setLayoutContentsMargins DESIGNABLE true);
    Q_PROPERTY(Qt::Alignment layoutAlignment READ layoutAlignment WRITE setLayoutAlignment DESIGNABLE true);

+    // A WWidgetGroup can be also used to highlight a group of widgets by


nit: word order should be "can also be used"

daschuer · 2016-12-17T19:03:20Z

To clarify, the transform is not necessary, correct? If I understand correctly, without that, the new highlight property will be set to whatever the value of the bound is.

Correct!

daschuer · 2016-12-17T19:49:20Z

Thank you for review!

Be-ing · 2016-12-17T19:50:09Z

res/skins/Shade/effectunit.xml

@@ -60,7 +59,6 @@
 							</Template>
 						</Children>
 						<Connection>
-							<ConfigKey>[Microphone],show_microphone</ConfigKey>
 							<BindProperty>highlight</BindProperty>


Remove the whole <Connection> element if it isn't doing anything.

It is prepared to take the new CO once your work is merged.

Oh, okay. LGTM then as long as it's documented on the wiki. 👍

daschuer · 2016-12-17T21:23:10Z

Wiki is updated

Be-ing · 2016-12-19T03:01:14Z

I added support for this to my branch for new Deere effects units in #1063 but it is not working. The highlight="1" style is only applied for the effect that has its focused CO set to 1 when the skin is loaded. Changing the focused CO after loading the skin does not update the styles. You can test by building my personal branch that has #1063 and #1062 merged into it.

daschuer · 2016-12-19T12:34:04Z

Oh Sorry, I see there is

    style()->polish(this);

missing in WWidgetGroup::setHighlight()

daschuer added 5 commits December 8, 2016 22:49

Put controlls of on effect into a common widget goup

a169ac0

Added WHighligtingGroup

80dba57

Use the new HighlightingGroup in Shade

21861de

Make use of HighlightingGroup in Shade's Effects

eef17ad

More comments

9b7ac9a

timrae reviewed Dec 10, 2016

View reviewed changes

Be-ing reviewed Dec 11, 2016

View reviewed changes

daschuer added 2 commits December 11, 2016 23:49

Fix indentation

fb98db3

Fix typos

a3b86ce

fix more indentation

4aa9d9a

This was referenced Dec 11, 2016

Dj compact #918

Merged

Reloop terminal mix2 cleanup #1076

Merged

Be-ing suggested changes Dec 15, 2016

View reviewed changes

daschuer added 2 commits December 17, 2016 14:59

Merged WHighlightingGroup with WWidgetGroup, dropped the Id concept

05488db

Added a Trasformer IsEqual and tested it in Shade.

cd58c7d

daschuer changed the title ~~HighlightingGroup for effects~~ Highlight feature for WidgetGroup Dec 17, 2016

Be-ing reviewed Dec 17, 2016

View reviewed changes

Remove test connection and fix word order in comment

b811d38

Be-ing reviewed Dec 17, 2016

View reviewed changes

daschuer merged commit 2ba33ad into mixxxdj:master Dec 17, 2016

Be-ing mentioned this pull request Dec 21, 2016

fix WidgetGroup highlighting #1087

Merged

daschuer deleted the single_effect_focused branch September 7, 2021 21:06

Highlight feature for WidgetGroup #1068

Highlight feature for WidgetGroup #1068

Conversation

daschuer commented Dec 9, 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

Be-ing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Dec 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer Dec 15, 2016 • edited Loading

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

Choose a reason for hiding this comment

Be-ing Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

daschuer Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rryan commented Dec 16, 2016

daschuer commented Dec 16, 2016

Be-ing commented Dec 16, 2016 • edited Loading

rryan commented Dec 16, 2016 • edited Loading

rryan commented Dec 16, 2016

Be-ing commented Dec 16, 2016

daschuer commented Dec 17, 2016

ywwg commented Dec 17, 2016

rryan commented Dec 17, 2016

Be-ing left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Dec 17, 2016

daschuer commented Dec 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Dec 17, 2016

Be-ing commented Dec 19, 2016

daschuer commented Dec 19, 2016

daschuer Dec 15, 2016 •

edited

Loading

daschuer Dec 15, 2016 •

edited

Loading

Be-ing Dec 16, 2016 •

edited

Loading

daschuer Dec 16, 2016 •

edited

Loading

Be-ing commented Dec 16, 2016 •

edited

Loading

rryan commented Dec 16, 2016 •

edited

Loading

Be-ing left a comment •

edited

Loading