ubuntu-sdk-team team mailing list archive

[Zsombor Egri <zsombor.egri@xxxxxxxxxxxxx>]

Review: Needs Information

Mostly clarification I need. Also few more tests to cover the group properties please.

Diff comments:

> === modified file 'components.api'
> --- components.api	2016-09-17 05:48:25 +0000
> +++ components.api	2016-12-13 14:24:00 +0000
> @@ -45,13 +45,14 @@
>      property bool active
>      function addAction(Action action)
>      function removeAction(Action action)
> -Ubuntu.Components.ActionItem 1.0 0.1 UCActionItem: StyledItem
> +Ubuntu.Components.ActionItem 1.3 1.0 0.1 UCActionItem: StyledItem
>      property Action action
>      property string iconName
>      property url iconSource
>      signal triggered(var value)
>      function trigger(var value)
>      function trigger()
> +    readonly property ActionMnemonic mnemonic 1.3

Oh, so you;ve added a group property not an attached one. You spoke about attached, that's why I got a bit confused :)
Though adding an extra QObject to Action increases the footprint, and we may not want that to happen. Upstream is quite against using extra objects if not necessary, though grouping properties is way nicer than prefixing each of those belonging to a specific group.
But really, what is the need to expose the mnemonic? Visibility I may accept, but the rest is a bit unnecessary API.

>      property string text
>  Ubuntu.Components.Styles.ActionItemProperties 1.3: QtObject
>      property color backgroundColor
> @@ -79,6 +80,10 @@
>      function removeAction(Action action)
>      function addLocalContext(ActionContext context)
>      function removeLocalContext(ActionContext context)
> +Ubuntu.Components.ActionMnemonic 1.3: QtObject
> +    property int modifier

You don't want to set the modifier, don't you? Or you want to have this because of the sub-menu accessing, to remove the Alt modifier need?

> +    readonly property StandardKey sequence
> +    property bool visible
>  Ubuntu.Components.Popups.ActionSelectionPopover 1.0 0.1: Popover
>      property var actions
>      property Component delegate


-- 
https://code.launchpad.net/~nick-dedekind/ubuntu-ui-toolkit/actionItem-mnemonics/+merge/313115
Your team Ubuntu SDK team is subscribed to branch lp:ubuntu-ui-toolkit/staging.