[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: KActionCollection proposal
From:       Ellis Whitehead <kde () ellisw ! net>
Date:       2002-01-16 18:35:44
[Download RAW message or body]

On Wednesday 16 January 2002 05:42 am, David Faure wrote:
> On Wednesday 16 January 2002 10:22, Ellis Whitehead wrote:
> > [snip]
> > I.e., instead of:
> > KStdAction::selectAll(viewManager, SLOT(slotSelectAll()),
actionCollection());
> > prefer:
> > actions().insert(KStdAction::SelectAll, viewManager,
> > SLOT(slotSelectAll());
> >
> > The advantages of this include assuring proper usage by the application
> > programmer and simplifying the code to be maintained.
>
> I don't like this. What about non-standard actions ? It makes inserting
> standard actions and non-standard actions very different, whereas currently
> the API is the same for both:
> * first, get a KAction* - either by creating your custom action or by
> getting it from KStdAction
> * second, insert that action.
>
> The above insert( <enum>, ... ) only works for standard actions, doesn't
> work for actions created by the developer.

The only insertion method that would be inconsistent would be sub-classes of 
KAction that the developer created himself (a rarity if not a non-existant 
phenomenon).  Those would require the "new KMyAction(KActionCollection*,...)" 
method instead of 'insert(...)'.

Other custom actions would be done like this:

actionCollection()->insert("save_copy", i18n("Save Co&py"), ...);
actionCollection()->insertToggle("show_score", i18n("Show Score"), ...);

However, the two-step process you mentioned really needs to be done in one 
step anyway -- the collection must to be specified on creation for proper 
handling.

> > 4) Depricate KStdAccel and KStdAction and all its functions.  Move the
> > enums to KAccel or KActionCollection and use a single insertion function
> > rather than using an individual function to create each action type. 
> > This makes the
> > code centralized, smaller, easier to maintain, and avoids the evil of
> > copy&paste programming in KStdAccel/KStdAction.
>
> I'd like to point out that the primary goal of classes in kdelibs is to
> offer a nice programming interface - the API is almost more important than
> the implementation behind it (as long as that implementation works ;)
> It would also be nice if we would stop changing the API at some point.

I agree that changing APIs are aggravating, without question.  That in mind 
as a counter-argument to what I'm saying here, my interest is in creating a 
"nicer" API.  KAccel, KGlobalAccel, KStdAccel, KStdAction, and 
KActionCollection could all be merged into one class, resulting in a simpler 
and much more consistent interface.

> [snip]
> > - add KAction* KActionCollection::insert(...)
> > - modify documentation to reflect the insert() method of action creation.
>
> One insert that creates the action and one that doesn't... might be a bit
> confusing.

I made the insert() method you're refering to 'protected' on my local copy 
back a couple weeks ago, so the coder can't use it anyway.  Four reasons why:

1) it's superfluous
    it requires extra lines of code when the the collection should 
    simply be specified on action creation
2) it's inefficient
    it triggers extra internal deletions and reinsertions.   before,
    two or three QAccel objects were having the same shortcut
    added to them.
3) it's creates run-time bugs
    it places shortcuts into the application-wide accel list, regardless of
    whether they belong there or not.
4) it has no advantage
    there was not a single place in all the CVS modules I've compiled,
    as far as I'm aware, where insertions and removals from a
    KActionCollection had any additional useful function.  All of these lines
    could be simply deleted.

This breaks compilation where insert() was used (most all programs in cvs use 
the one-step method -- I haven't looked at koffice yet), but this is a 
critical issue: treating the action collection as something secondary which 
may or may not be attached to creates accelerator bugs.

---
What do you think?

Regards,
Ellis
[prev in list] [next in list] [prev in thread] [next in thread]