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

List:       kde-core-devel
Subject:    Re: PATCH KGuiItem
From:       Martijn Klingens <mklingens () ism ! nl>
Date:       2001-11-13 12:34:09
[Download RAW message or body]

On Monday 12 November 2001 23:33, you wrote:
> the KGuiItem is on the Todo List. Actually I created the KGuiItem class and
> changed KAction to use it.  So basicly I don't know if everything necessary
> is in this class so speak up if you need some more.

Well... there is a bit too *much* in KAction now :-)

You duplicated the methods to get/set the fields that KGuiItem provides. That 
somehow sounds a bit redundant to me, since your modified KAction inherits 
the methods from KGuiItem.

Also, I think you should have a d pointer in KGuiItem and KAction. Removing 
those as you do now is a very bad idea IMO for kdelibs code.

> I'm going to patch KDialogBase and KmessageBox to make use of this KGuiItem
> class as soon as it is accepted

Somehow I have the feeling you make me unemployed with regards to what I 
wanted to do on KDE with the little spare time I have ;-)

> Further steps could be to add these things to KPopupMenu, KPushButton,
> KComboBox...

KPopupMenu is a bit questionable, since you can also plug KActions in menus 
(and even in *Q* popup menus, instead of the 'K' version), but KPushButton 
definitely could benefit from this.

> TODO: I'm going to add signals to KGuiItem to do the following,
> KGuiItem *item = new KStdAction::ok();
> item->setText(i18n("&Ok!"));
> KPushButton *button = new KPushButton(item );
> item->setText(i18n("&Save") );
> So with these signals I'm able to "autoupdate" the items.

Yes! I already dearly missed them in KDE 2.x's KAction, aand with KGuiItem 
they are really needed IMO.
I don't know what the general consensus is, but I'd favour the signal to emit 
a pointer to itself as last parameter:

emit( textChanged( m_text, this ) );

Qt's connect allows to connect with discarding the second parameter and in a 
number of applications it might be very useful to have this pointer at hand 
(share a single slot, optionally, to handle multiple actions/gui items).

Martijn

PS: I didn't have time to look at your code too closely, so I might find more 
issues that I'd like to discuss, but on the whole your patch looks pretty 
good already. Nice work!

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