D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Matthieu Gallien]

mgallien added a comment.


  This patch breaks compatibility with Qt5.11 where Qt Quick Controls2 is in 
version 2.4.
  I noticed it after Debian upgraded the package and I am unable to let Elisa 
run with it. I will try to work on a fix.

REPOSITORY
  R169 Kirigami

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

To: camiloh, #kirigami, mart
Cc: mgallien, astippich, apol, plasma-devel, fbampaloukas, GB_2, domson, 
dkardarakos, davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

This revision was automatically updated to reflect the committed changes.
Closed by commit R169:fd8fec5b8923: Allow usage of QQC2 actions on Kirigami 
components and now make K.Action based… (authored by camiloh).

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22974?vs=63727=63728

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

AFFECTED FILES
  src/controls/Action.qml
  src/controls/ActionToolBar.qml
  src/controls/private/ActionButton.qml
  src/controls/private/ActionsMenu.qml
  src/controls/private/GlobalDrawerActionItem.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/private/globaltoolbar/ToolBarPageHeader.qml

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh updated this revision to Diff 63727.
camiloh added a comment.


  clean up code, remove comments and documentation fixes

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22974?vs=63726=63727

BRANCH
  k-action-to-qqc2-action (branched from master)

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

AFFECTED FILES
  src/controls/Action.qml
  src/controls/ActionToolBar.qml
  src/controls/private/ActionButton.qml
  src/controls/private/ActionsMenu.qml
  src/controls/private/GlobalDrawerActionItem.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/private/globaltoolbar/ToolBarPageHeader.qml

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Marco Martin]

mart added inline comments.

INLINE COMMENTS

> Action.qml:27
>   *
>   * @inherit QtObject
>   */

@inherit QtQuick.Controls.Action

> Action.qml:76
>   */
> -property ActionIconGroup icon: ActionIconGroup {
> -id: iconGroup
> -}
> +// property ActionIconGroup icon: ActionIconGroup {
> +// id: iconGroup

remove dead code

> Action.qml:152
>  
> -property Shortcut __shortcut: Shortcut {
> -property bool checked: false
> -id: shortcutItem
> -enabled: root.enabled
> -onActivated: root.trigger();
> -}
> -function trigger(source) {
> -if (!enabled) {
> -return;
> -}
> -root.triggered(source);
> -if (root.checkable) {
> -root.checked = !root.checked;
> -root.toggled(root.checked);
> -}
> -}
> -
> -onCheckedChanged: root.toggled(root.checked);
> +// property Shortcut __shortcut: Shortcut {
> +// property bool checked: false

remove dead code

REPOSITORY
  R169 Kirigami

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

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh updated this revision to Diff 63726.
camiloh added a comment.


  remove debugging line

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22974?vs=63725=63726

BRANCH
  k-action-to-qqc2-action (branched from master)

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

AFFECTED FILES
  src/controls/Action.qml
  src/controls/ActionToolBar.qml
  src/controls/private/ActionButton.qml
  src/controls/private/ActionsMenu.qml
  src/controls/private/GlobalDrawerActionItem.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/private/globaltoolbar/ToolBarPageHeader.qml

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh added a comment.


  In D22974#508067 , @mart wrote:
  
  > good direction, unfortunately we can't remove the custom toolbutton 
contentitem yet, so for now let's keep the custom icon group
  
  
  The solution here was to add back the kirigamiAction property to the 
PrivateActionToolButton, so it avoids the QQC2.ToolButton action prop

REPOSITORY
  R169 Kirigami

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

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh updated this revision to Diff 63725.
camiloh added a comment.


  fallback to custom kirigamiAction property for PrivateActionToolButton
  
  - useage of the fallback property due to: if we use QQC2.ToolButton default 
action property then the style will draw the content, and since this component 
draws the dropDown icon it still needs to be able to draw its own content in 
place, so the kirigamiAction hides the action prop and then avoids duplicating 
of icons.
  
  Still, an ideal solution would be to be able to let the style draw the 
content, but there was not an easy way to anchors the dropDown icon in a nice 
manner by relaying on the style.

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22974?vs=63242=63725

BRANCH
  k-action-to-qqc2-action (branched from master)

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

AFFECTED FILES
  src/controls/Action.qml
  src/controls/ActionToolBar.qml
  src/controls/private/ActionButton.qml
  src/controls/private/ActionsMenu.qml
  src/controls/private/GlobalDrawerActionItem.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/private/globaltoolbar/ToolBarPageHeader.qml

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Marco Martin]

mart requested changes to this revision.
mart added a comment.
This revision now requires changes to proceed.


  good direction, unfortunately we can't remove the custom toolbutton 
contentitem yet, so for now let's keep the custom icon group

INLINE COMMENTS

> camiloh wrote in Action.qml:76
> from Kirigami actionIconGroup:
> string name
> string source
> int width
> int height
> color color
> 
> from QQC2 icon property:
> icon.name
> icon.source
> icon.width
> icon.height
> icon.color

ok, please remove all code that you commented out

REPOSITORY
  R169 Kirigami

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

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh updated this revision to Diff 63242.
camiloh added a comment.


  remove todo line

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22974?vs=63241=63242

BRANCH
  k-action-to-qqc2-action (branched from master)

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

AFFECTED FILES
  src/controls/Action.qml
  src/controls/ActionToolBar.qml
  src/controls/GlobalDrawer.qml
  src/controls/ToolBarApplicationHeader.qml
  src/controls/private/ActionButton.qml
  src/controls/private/ActionsMenu.qml
  src/controls/private/GlobalDrawerActionItem.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/private/globaltoolbar/ToolBarPageHeader.qml

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh updated this revision to Diff 63241.
camiloh added a comment.


  Correct possible undefined result pointed out by Aleix

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22974?vs=63240=63241

BRANCH
  k-action-to-qqc2-action (branched from master)

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

AFFECTED FILES
  src/controls/Action.qml
  src/controls/ActionToolBar.qml
  src/controls/GlobalDrawer.qml
  src/controls/ToolBarApplicationHeader.qml
  src/controls/private/ActionButton.qml
  src/controls/private/ActionsMenu.qml
  src/controls/private/GlobalDrawerActionItem.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/private/globaltoolbar/ToolBarPageHeader.qml

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh added inline comments.

INLINE COMMENTS

> camiloh wrote in ActionToolBar.qml:184
> I think it then gets marked as visible = true

You were right, I have now fixed it.

REPOSITORY
  R169 Kirigami

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

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh updated this revision to Diff 63240.
camiloh marked an inline comment as done.
camiloh added a comment.


  further changes to make components work with new Kirigami.Action based on 
QQC2.Action, and proposed solution to draw the PrivateActionToolButton

REPOSITORY
  R169 Kirigami

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D22974?vs=63219=63240

BRANCH
  k-action-to-qqc2-action (branched from master)

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

AFFECTED FILES
  src/controls/Action.qml
  src/controls/ActionToolBar.qml
  src/controls/GlobalDrawer.qml
  src/controls/ToolBarApplicationHeader.qml
  src/controls/private/ActionButton.qml
  src/controls/private/ActionsMenu.qml
  src/controls/private/GlobalDrawerActionItem.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/private/globaltoolbar/ToolBarPageHeader.qml

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh marked 2 inline comments as done.
camiloh added a comment.


  I have worked on the duplicating icons, by not using the contentItem to draw 
the icons and label, and instead use the style implementation. The only thing 
missing would be the dropdown icon for actions with submenus, I'm anchoring 
that icon to the right con the contentItem. I will update this patch.
  I will push my proposal for duplicating icons, it can be reviewed. And then I 
can split this patch into different parts as suggested by Aleix.

INLINE COMMENTS

> apol wrote in Action.qml:76
> Can you check that we're not missing some API? is ActionIconGroup a subset of 
> what Qt offers?

from Kirigami actionIconGroup:
string name
string source
int width
int height
color color

from QQC2 icon property:
icon.name
icon.source
icon.width
icon.height
icon.color

> apol wrote in ActionToolBar.qml:184
> If `ourAction` doesn't have a visible property, won't `ourAction.visible` be 
> undefined?

I think it then gets marked as visible = true

> apol wrote in PrivateActionToolButton.qml:89
> Wouldn't it be control.action.icon.name?

yes. fixed now on new commit

REPOSITORY
  R169 Kirigami

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

To: camiloh, #kirigami, mart
Cc: astippich, apol, plasma-devel, fbampaloukas, domson, dkardarakos, 
davidedmundson, mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Aleix Pol Gonzalez]

apol added a comment.


  WRT the two icons, maybe it would make sense to populate only the bits we 
want instead of using the action property? i.e. we can keep a kirigamiAction 
property for now. This patch is far too big as is already. I would even suggest 
doing the Kirigami.Action rebase first and then do the other one in different 
commits.

INLINE COMMENTS

> Action.qml:76
>   */
> -property ActionIconGroup icon: ActionIconGroup {
> -id: iconGroup
> -}
> +// property ActionIconGroup icon: ActionIconGroup {
> +// id: iconGroup

Can you check that we're not missing some API? is ActionIconGroup a subset of 
what Qt offers?

> ActionToolBar.qml:184
>  itemDelegate: ActionMenuItem {
> -visible: 
> actionsLayout.findIndex(actionsLayout.overflowSet, function(act) {return act 
> === ourAction}) > -1 && (ourAction.visible === undefined || ourAction.visible)
> + visible: 
> actionsLayout.findIndex(actionsLayout.overflowSet, function(act) {return act 
> === ourAction}) > -1 &&  (!ourAction.hasOwnProperty("visible") || 
> ourAction.visible === undefined || ourAction.visible)
>  }

If `ourAction` doesn't have a visible property, won't `ourAction.visible` be 
undefined?

> PrivateActionToolButton.qml:42
> +flat: !control.action || !control.action.icon.color.a
>  //TODO: replace with upstream action when we depend on Qt 5.10
> +// property Action action

Remove TODO?

> PrivateActionToolButton.qml:89
> +source: control.action ? (control.action.icon ? 
> control.action.icon.name : control.action.iconName) : ""
> +visible: control.action && control.action.iconName != "" && 
> control.display != Controls.ToolButton.TextOnly
> +color: control.flat && control.action && control.action.icon 
> && control.action.icon.color.a > 0 ? control.action.icon.color : label.color

Wouldn't it be control.action.icon.name?

REPOSITORY
  R169 Kirigami

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

To: camiloh, #kirigami, mart
Cc: apol, plasma-devel, fbampaloukas, domson, dkardarakos, davidedmundson, 
mart, hein

D22974: Allow usage of QQC2 actions on Kirigami components and now make K.Action based on QQC2.Action

[Camilo Higuita]

camiloh created this revision.
camiloh added a reviewer: Kirigami.
Herald added a project: Kirigami.
Herald added a subscriber: plasma-devel.
camiloh requested review of this revision.

REVISION SUMMARY
  The ideal is to be able to add regular qqc2 actions to the actiontoolbar, 
pageheaders, globaldrawers and different components, and make kirigami.action 
based on qqc2.action, so implementations are not duplicated.

TEST PLAN
  So far i have tested this with different kirigami components, such as 
pageheadertoolbar, globaldrawers, kirigamiactiontoolbar, and it is working.
  There is an issue now with the PrivateActionToolButton, since K.Actions is 
based on QQC2.Action, it make sthe control to draw two icons, one from 
PrivateActionToolBar own contentItem implementation and another one from the 
style background
  
  I will continue testing this, and pls let me know how i could proceed better 
with the PrivateActionToolButton issue.

REPOSITORY
  R169 Kirigami

BRANCH
  k-action-to-qqc2-action (branched from master)

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

AFFECTED FILES
  src/controls/Action.qml
  src/controls/ActionToolBar.qml
  src/controls/GlobalDrawer.qml
  src/controls/private/GlobalDrawerActionItem.qml
  src/controls/private/PrivateActionToolButton.qml
  src/controls/private/globaltoolbar/ToolBarPageHeader.qml

To: camiloh, #kirigami
Cc: plasma-devel, fbampaloukas, domson, dkardarakos, apol, davidedmundson, 
mart, hein