D16658: Respect the display property of buttons

[Alexander Stippich]

astippich created this revision.
astippich added reviewers: Plasma, apol, mart.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
astippich requested review of this revision.

REVISION SUMMARY
Before, the display property of buttons introduced in Qt 5.10
was not taken into account. It allows to specifiy if the buttons
display icon or text only, or both

REPOSITORY
R858 Qt Quick Controls 2: Desktop Style

BRANCH
display_property

REVISION DETAIL
https://phabricator.kde.org/D16658

AFFECTED FILES
org.kde.desktop/Button.qml

To: astippich, #plasma, apol, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Aleix Pol Gonzalez]

apol added a comment.


I guess we can wait until we can bump to Qt 5.10 as a dependency? This looks a bit chaotic.
Also the Under/Beside setting isn't taken into account yet.

REPOSITORY
R858 Qt Quick Controls 2: Desktop Style

REVISION DETAIL
https://phabricator.kde.org/D16658

To: astippich, #plasma, apol, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Alexander Stippich]

astippich added a comment.

Post by Aleix Pol Gonzalez
I guess we can wait until we can bump to Qt 5.10 as a dependency? This looks a bit chaotic.
Also the Under/Beside setting isn't taken into account yet.

Yep, forgot that to mention, but icon below the text is not something I currently care about.

REPOSITORY
R858 Qt Quick Controls 2: Desktop Style

REVISION DETAIL
https://phabricator.kde.org/D16658

To: astippich, #plasma, apol, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Alexander Stippich]

astippich updated this revision to Diff 45424.
astippich added a comment.


- rebase
- more verbose comment about compatibility

REPOSITORY
R858 Qt Quick Controls 2: Desktop Style

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16658?vs=44827&id=45424

REVISION DETAIL
https://phabricator.kde.org/D16658

AFFECTED FILES
org.kde.desktop/Button.qml

To: astippich, #plasma, apol, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Alexander Stippich]

astippich added a comment.


Can I convince you to land this for Kf 5.53 with the promise to clean up once frameworks depends on Qt 5.10?

REPOSITORY
R858 Qt Quick Controls 2: Desktop Style

REVISION DETAIL
https://phabricator.kde.org/D16658

To: astippich, #plasma, apol, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Marco Martin]

mart added a comment.

Post by Alexander Stippich
Can I convince you to land this for Kf 5.53 with the promise to clean up once frameworks depends on Qt 5.10?

it needs at least a final release of Qt 5.12: frameworks need to depend from 2 Qt releases prior the current one.
however, i think it's possible to make it work without breaking old releases

REPOSITORY
R858 Qt Quick Controls 2: Desktop Style

REVISION DETAIL
https://phabricator.kde.org/D16658

To: astippich, #plasma, apol, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Marco Martin]

mart requested changes to this revision.
mart added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

Button.qml:44
+ * and controlRoot.display != AbstractButton.IconOnly == 0 */
+ Kirigami.MnemonicData.label: (controlRoot.display == undefined || controlRoot.display != 0) ? controlRoot.text : ""
Shortcut {

controlRoot.hasOwnProperty("display")

Button.qml:65
+ * and controlRoot.display != AbstractButton.TextOnly == 1 */
+ "icon": controlRoot.icon && (controlRoot.display == undefined || controlRoot.display != 1) ? (controlRoot.icon.name || controlRoot.icon.source) : "",
"iconColor": controlRoot.icon && controlRoot.icon.color.a > 0? controlRoot.icon.color : Kirigami.Theme.textColor,

controlRoot.display == undefined needs to become controlRoot.hasOwnProperty("display") this would be false with Qt 5.9 and less, protecting controlRoot.display != 1
also add the following comment:
//FIXME: when we can depend from Qt 5.10 do controlRoot.display !== T.AbstractButton.TextOnly

REPOSITORY
R858 Qt Quick Controls 2: Desktop Style

REVISION DETAIL
https://phabricator.kde.org/D16658

To: astippich, #plasma, apol, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Alexander Stippich]

astippich updated this revision to Diff 46372.
astippich added a comment.


- use hasOwnProperty and add fixme comment

REPOSITORY
R858 Qt Quick Controls 2: Desktop Style

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16658?vs=45424&id=46372

BRANCH
display_property

REVISION DETAIL
https://phabricator.kde.org/D16658

AFFECTED FILES
org.kde.desktop/Button.qml

To: astippich, #plasma, apol, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

[Alexander Stippich]

astippich added a comment.


ping

REPOSITORY
R858 Qt Quick Controls 2: Desktop Style

REVISION DETAIL
https://phabricator.kde.org/D16658

To: astippich, #plasma, apol, mart
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart