KActionCollection: it's a jungle

[Andreas Hartmetz]

Am Montag, 19. März 2007 12:26:34 schrieb 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).
>
What kind breakage do you expect? That has to be the most important question 
here...

> > 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.