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

Authored by camiloh on Tue, Aug 6, 4:58 PM.

Details

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.

Diff Detail

Repository
R169 Kirigami
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
camiloh created this revision.Tue, Aug 6, 4:58 PM
Restricted Application added a project: Kirigami. · View Herald TranscriptTue, Aug 6, 4:58 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
camiloh requested review of this revision.Tue, Aug 6, 4:58 PM
camiloh added a reviewer: mart.Tue, Aug 6, 4:59 PM
apol added a subscriber: apol.Tue, Aug 6, 5:22 PM

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.

src/controls/Action.qml
51–52

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

src/controls/ActionToolBar.qml
184

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

src/controls/private/PrivateActionToolButton.qml
25–26

Remove TODO?

26

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

camiloh marked 2 inline comments as done.Tue, Aug 6, 8:49 PM

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.

src/controls/Action.qml
51–52

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

src/controls/ActionToolBar.qml
184

I think it then gets marked as visible = true

src/controls/private/PrivateActionToolButton.qml
26

yes. fixed now on new commit

camiloh updated this revision to Diff 63240.EditedTue, Aug 6, 10:44 PM
camiloh marked an inline comment as done.

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

The dropDown icon for PrivateActionToolButton is not yet perfect. I'm trying new things to make it fit the actual ToolButton background.

I've tried to be super careful with the changes, and have tested them over the kirigami examples, kirigami-gallery and Maui apps, and it all seems to work okay, except, the dropDown icon overlapping.

camiloh updated this revision to Diff 63241.Tue, Aug 6, 11:03 PM

Correct possible undefined result pointed out by Aleix

camiloh added inline comments.Tue, Aug 6, 11:03 PM
src/controls/ActionToolBar.qml
184

You were right, I have now fixed it.

camiloh marked 3 inline comments as done.Tue, Aug 6, 11:05 PM
camiloh updated this revision to Diff 63242.Tue, Aug 6, 11:23 PM

remove todo line

mart requested changes to this revision.Wed, Aug 7, 1:50 PM

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

src/controls/Action.qml
51–52

ok, please remove all code that you commented out

This revision now requires changes to proceed.Wed, Aug 7, 1:50 PM
camiloh updated this revision to Diff 63725.Wed, Aug 14, 2:22 PM

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.

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

camiloh updated this revision to Diff 63726.Wed, Aug 14, 2:28 PM

remove debugging line

camiloh marked 2 inline comments as done.Wed, Aug 14, 2:29 PM
mart added inline comments.Wed, Aug 14, 2:31 PM
src/controls/Action.qml
27

@inherit QtQuick.Controls.Action

51–52

remove dead code

116–117

remove dead code

camiloh updated this revision to Diff 63727.Wed, Aug 14, 2:35 PM

clean up code, remove comments and documentation fixes

mart accepted this revision.Wed, Aug 14, 2:36 PM
This revision is now accepted and ready to land.Wed, Aug 14, 2:36 PM
This revision was automatically updated to reflect the committed changes.