KActionCollection: it's a jungle

[Hamish Rodda]

On Monday 19 March 2007 20:43, Andreas Hartmetz wrote:
> Am Montag, 19. März 2007 08:27:06 schrieb Hamish Rodda:
> > On Sunday 18 March 2007 10:16, Andreas Hartmetz wrote:
> > <snip>
> >
> > > Therefore, I propose to remove all of these:
> > >
> > > * addDocCollection
> > >  a)huh?, b) used in one case in KHelpCenter. Can't be that useful.
> >
> > Discussed already in the thread.
> >
> > > * setDefaultShortcutContext:
> > >  used three times, to set it to the default each time...
> > > * applyDefaultShortcutContext:
> > >  a)wtf?, b) unused
> > > * defaultShortcutContext:
> > >  then obsolete
> >
> > These functions are to prevent having to loop over each action and change
> > its shortcut context, or to set each by hand when setting up your
> > actions. I exposed them because they were needed by the internal api
> > (*AssociatedWidget*) and seemed to me like useful functions for 3rd party
> > code to access.
> >
> > However I don't mind their being made private if you feel so moved.
>
> It really doesn't hurt to have some more bytes in the library, but utility
> functions that save one line of code (direct call vs. foreach) are IMHO not
> worth the increased mental effort when reading the documentation.
> Partly because of increased length, partly because you start thinking "why
> is it there, does it really only replace a foreach loop, what did I miss?".
> A foreach here and there adds more bytes to the applications, and for me it
> makes sense to add tiny utility functions if and only if they will be used
> *really* often.
> lxr did not find any use so I just don't think they reach a level of use to
> justify them.

I've been contacted a while back about a problem which could be fixed by using 
this function (or its inline equivalent), I think it's used in kdegames.  If 
that is ported I'm happy for it to be removed and the necessary changes made 
so that setAssociatedWidget etc. still works (it still needs to change the 
shortcut context to Qt::WidgetContext automatically).

> > > * forgetEnabled:
> > >  a)wtf?, b)unused.
> > > I'm unsure about setEnabled/isEnabled... it's hard to find them because
> > > the name occurs so often and I'm still investigating with a massive
> > > grep orgy.
> >
> > I think I added these, sounded like a useful feature again to me.  They
> > could go, if you're happy to change all uses of them.  I don't see the
> > need to remove them, personally.
>
> <strikeout>
> They are going to stay, one reason being that I can't think of a reliable
> way to find all uses of them without using the "see if anything breaks in
> any KDE module when they are removed" approach.
> The other reason is that they are useful anytime you are in the process of
> building a collection with an occasional run of the event loop in between.
> Large applications can work that way.
> forgetEnabled - IMHO it should be called stopEnforcingEnabled or similar.
> The current name sounds like it does the opposite of what it actually does.
> Is this a non-native speaker's problem?
> </strikeout>
> ------------------
> No wait, a better solution would be doDisableOnAdd(bool) /
> doesDisableOnAdd() / allSetEnabledTo(bool enable). Actions are enabled by
> default (correct?), so no reason to have them enabled when adding them.
> doesDisableOnAdd() would probably only help with debugging.
>
> This set of function could do essentially the same, but in a *much* less
> confusing way. isEnabled() would have to be dropped - what does it mean
> anyway for a collection? At least one is enabled? All are enabled? None is
> very useful, because single actions can differ for various reasons.
> Are the function names OK with native speakers?

I agree the names are not great.  I could see this being an enum... perhaps 
enum EnabledStatus { Unchanged, Enabled, Disabled }; and setEnabledStatus() / 
enabledStatus().  But on the other hand I can see it being removed, and just 
fixing breakage where it happens (it will be quite rare I'd say).

> Hello Hamish! I hope that you didn't take the criticism too personally...
> the class looked to me like like a straight port / merge with any old sins
> still in there.

Of course not, constructive criticism + taken as so :)

Cheers,
Hamish.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070319/980bf1e9/attachment.sig>