A KSelectAction dedicated to QTextCodec selection ?

[Michel Hermier]

Hi,

2006/10/20, David Faure <faure at kde.org>:
> Hello!
>
> On Friday, October 20, 2006 01:19:05 PM Michel Hermier wrote:
> > I noticed while preparing the changes locally, that I need one of my
> > local change for KSelectAction (well only a return change, but the
> > patch adds a little more).
> > So I delay the commit for now.
> >
> > Can someone maintaining KSelectAction review the patch.
> > What changed with this patch:
> > - bool setCurrentAction(QAction *, DeselectionMode mode) should be
> > more safe, checking that the action really belongs to the action group
> > before activating the action. Also added an extra parameter with
> > default value to mimic the old behaviour. This extra parameter allow
> > to not deselect the previous action in case of falure to select the
> > action.
>
> Why does this need to be configurable? Deselection the previous action
> and not selecting any new one instead looks like a bug, not a feature.
> Did you make it configurable "just in case", or is there a real use case for this?

I think it's needed to be configurable if we allow to have multiple
selection enabled.
Because in this case you have to look at all the actions to search to
know wich ones are enabled, since the API don't offer to get all the
selected items (directly).

And yes there is a real need to allow multiple selection, since
usually these groups are quite large (see the font and QTextCodec). So
we need an option to configure the removal some of them, so we need to
select multiple items.

> We need "set this one as current" and "set none as current".
> Why do we need "setCurrentAction(0,LetSelectedMode)" which does -nothing-

I know it's stupid, but it's necessary to allow setCurrentAction(fooaction);
Unless you propose to make the SelectionMode configured by a method,
it's still stupid to make
setSelectionMode(LetSelectedMode);
setCurrentAction(0);

> and setCurrentAction(unrelatedAction,DeselectMode) which has the effect of unchecking the current action (!)
> and setCurrentAction(unrelatedAction,LetSelectedMode) which, well, does nothing too.

Hmmm you are rigth I don't thougth about this, maybe now I can remove
these cases ;)

> Either I'm missing something, or this would be a very confusing API.
>
> setCurrentAction(unrelatedAction) should do nothing IMHO, and that's it.

Sure, here is the patch for this ;)

Maybe, I should also add some checks to check that the actions are
visible, enabled and checkable.

Michel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kselectaction.diff
Type: application/octet-stream
Size: 5533 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20061020/b2f17608/attachment.obj>