Code review comment for lp:~zsombi/ubuntu-ui-toolkit/contextual_actions

Revision history for this message
Cris Dywan (kalikiana) wrote :

> > * ActionContext drives the Action objects declared within the ActionContext
> > * through the \l actions list. Beside that the ActionContext drives the
> > activability
> > * of Action objects' shortcuts declared in a hierarchy. In the following
> > * example the ActionContext drives the underlaying \c action1 and \c
> action2
> > * shortcuts:
> >
> > Brought to you by redundancy department of redundancy ;-)
>
> LOL, completely agree... it's a bit of a cumbersome description. However
> there's something right in it :)
>
> >
> > How about something simpler:
> >
> > ActionContext drives the state of its \l actions. Shortcuts and mnemonics
> > are only registered if the context is active.
>
> The second sentence is not true. The shortcuts are registered but the action
> to which they are registered will be activated only if the action is in an
> active ActionContext or assigned to an ActionItem which is declared as a child
> to an item which has an active ActionContext, and all its ancestors who have
> ActionContext declared are active... huh... we need a better description of
> this...

By registered I meant "will invoke its action when pressed".

How about:

ActionContext drives the state of its \l actions. Shortcuts and mnemonics are only registered if the context is active or if the action is assigned to an \l ActionItem all of whose parent contexts are active.

> > But I find the example a bit confusing. There's a rootContext with an action
> > responding to ^A but at the end of the example you're explaining how this
> > cannot work... and the buttons are not using the context at all - how is
> > this an example of using the context?
>
> So, in the example the Rectangle has an active ActionContext. The Buttons
> within have Actions assigned to the ActionItem, so the first requirement (the
> Action is assigned to an ActionItem) is fulfilled. When the shortcut triggers
> in either of the Button actions, the selection logic checks whether the
> ActionItem's ancestors have all ActionContexts active. If yes, it marks
> selected, so it can trigger. Then the first button can deactivate the context,
> in which case neither of the actions will be selected for the registered
> shortcut.
>
> To ease the detection, ActionContext - upon component creation - attaches an
> object to its parent, so we can detect which parent Item has an ActionContext
> object declared.
>
> Summarising: An Action can be triggered by a shortcut (or mnemonic) if a) it
> is declared in an active context action (ActionContext.actions) or b) it is
> assigned to an ActionItem (or derivate) which has (at least one) ancestor
> Items which have active ActionContexts.

So you are disagreeing with the explanation given in the example? Which says "rootContext will never be triggered through the \c {Ctrl+A} shortcut". But you are saying the rootContext can be used because the action is declared as a child. Which logically has to mean that its shortcut can work as well - the shortcut should be active whenever its Action is effectively active.

> > + bool m_effectiveActive:1;
> >
> > effectivelyActive
>
> I followed QtQuick naming, they use effectiveVisible, effectiveLayoutMirror,
> effectiveEnabled, etc

I guess that's a fair reason.

review: Needs Fixing

« Back to merge proposal