[immodule-qt] Introducing pluggable popup menu

[Lars Knoll]

Hi Yamaken,

We definitely need a way to add something to the context menu, so this is a 
very good thing.

But I do have a few comments/questions about the implementation.

In Qt 4, you can't use a QPtrList anymore (as it's defined in the compat 
library), but will have to use a QList. The semantics is is also easier to 
understand if you return a value and not a pointer. As Qt containers are 
implicitly shared this is cheaper. 

Is there a reason why you have added a menus() method instead of directly 
making the exportMenusInto() method virtual?

I would prefer if you used an enum instead of the "bool insertSeparator" and 
renamed exportMenusInto to appendMenusTo. The reason to use an enum is that 
later on the code written is a lot clearer and it can be extended more 
easily.

So I would propose

enum IMMenuAction {
 InsertSeparator,
 NoSeparator
};
virtual void addIMMenus(QMenu *menu, IMMenuAction = InsertSeparator);

Also your current implementation limits you to adding submenus. Is this good 
enough or would one sometimes also need to just add a single item to the 
menu?

Another question I have is about QMultiInputContext. Why do we need this class 
at all? Wouldn't it be better to build the support for IM switching directly 
into Qt?

Best regards,
Lars


On Wednesday 04 August 2004 16:28, YamaKen wrote:
> At Wed, 04 Aug 2004 23:14:07 +0900,
>
> yamaken at bp.iij4u.or.jp wrote:
> > [1  <text/plain; US-ASCII (7bit)>]
> > Hi all, I've implemented pluggable popup menu feature. Please
> > review the design and implementation of attached patch.
>
> Oops, I had sent older patch. Refer new one in this mail.