Virtual methods in KLineEdit

[Simon Hausmann]

On Saturday 27 January 2007 14:53:32 Christian Schaarschmidt wrote:
> Am Dienstag, 23. Januar 2007 22:58 schrieb Olivier Goffart:
> > Le mardi 23 janvier 2007 19:38, Christian Schaarschmidt a écrit :
> > > Hi,
> > >
> > > I have recently commited a change to trunk/.../KLineEdit (bug 137430)
> > > and after that I had a discussion with Simon concerning virtual
> > > methods. Since we could not agree upon a solution it would be nice to
> > > get some other opinions...
> > >
> > > Problem: Allow subclasses of KLineEdit to extend context menu
> > > The patch solves the issue by moving the code to
> > > createStandardContextMenu(), remains the question: should KLineEdit be
> > > used as base for a hierarchy? How comfortable should it be?
> >
> > If you choose to have the virtual method,  i think that
> > createStandardContextMenu is not a good name for the following reason:
> >  - It's not really standard for the sublass (i mean that
> > KFooApplicationSuperLineEdit cannot say his menu will be standard)
> >  - It's confusing with the Qt method which is not virtual
> >
> > So i suggest simply "createContextMenu"
>
> this sounds like a reasonable suggestion.
> since there were no more comments on this issue I'll do that.

Please please reconsider the use of a virtual function here. I mentioned it in 
private mail already but I'll do it here again since it wasn't part of your 
summary:

It is very very much an exception for an application to sub-class 
(K/Q)LineEdit for a customized context menu. I don't think your case of a 
sub-sub-class is realistic. Mechanisms like QWidget's context menu policy 
make it easy to implement custom context menus without the need to subclass. 
And even in the very rare case that an application would have to re-implement 
contextMenuEvent() of KLineEdit it is absolutely trivial code:

QMenu *menu = createStandardContextMenu();
[ ... modify it ... ]
menu->exec();
delete menu;

I think it makes perfect sense to shadow QLineEdit's 
createStandardContextMenu() implementation with one that provides a KDE style 
menu. That keeps the API 100% consistent and people don't have to learn yet 
another mechanism for dealing with context menus that is specific to 
KLineEdit only.

Let's please not make the same mistake as in Qt 3 where every third function 
was virtual just because one can. Almost every property pair there had a 
virtual setter which was also a slot. There is a price to it and I think we 
should just be a bit more careful in kdelibs4, too.

Simon
-------------- 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/20070128/ea18063b/attachment.sig>