[Gtk2hs-devel] [PATCH] Update signal handlers for MenuItem and Item
objects, and fix a suspected bug
From: Maxime H. <mhe...@gm...> - 2010-10-24 20:39:07
Attachments:
menuitem.patch
|
Hello all, Yet another boring patch to update signal handlers to the new way of doing things, this time for the MenuItem and Item objects :-). There is, however, something of interest in this patch. I was surprised to see that the "onToggle" and "afterToggle" signal handlers were actually bound to the signal name "toggled". Checking the GTK+ API documentation, it appears the correct signal name is really "toggle", without the final 'd' character. I suspect this code never worked before, but that noone was unlucky enough to actually hit it. Therefore, I corrected the signal name in the new signal handler as well as in the deprecated signal handlers. Finally, another question comes to mind: why is the "activate" signal bound by the "activateLeaf" signal handler? Should I take advantage of this patch to rename it to just "activate", or "menuItemActivate" if there are namespace conflicts? Cheers, Maxime |
From: Andy S. <laz...@gm...> - 2010-10-25 04:05:28
|
Hi Maxime, Maxime Henrion <mhe...@gm...> writes: > Hello all, > > > Yet another boring patch to update signal handlers to the new way of > doing things, this time for the MenuItem and Item objects :-). There > is, however, something of interest in this patch. I have apply your patches, thanks! :) > > I was surprised to see that the "onToggle" and "afterToggle" signal > handlers were actually bound to the signal name "toggled". Checking the > GTK+ API documentation, it appears the correct signal name is really > "toggle", without the final 'd' character. I suspect this code never > worked before, but that noone was unlucky enough to actually hit it. > Therefore, I corrected the signal name in the new signal handler as well > as in the deprecated signal handlers. Yes, it's a bug, thanks fix. :) > > Finally, another question comes to mind: why is the "activate" signal > bound by the "activateLeaf" signal handler? Should I take advantage of > this patch to rename it to just "activate", or "menuItemActivate" if > there are namespace conflicts? Yes, menuItemActivate is right signal name. But after we change to 'menuItemActivate' it conflict with function 'menu_item_activate' that emit 'activate' signal. So we change function 'menuItemActivate' (menu_item_activate) to 'menuItemEmitActivate' avoid conflict. Please see detail in patches i push. :) Cheers, -- Andy |
From: Maxime H. <mhe...@gm...> - 2010-10-25 14:50:27
|
Hi Andy, On Mon, Oct 25, 2010 at 5:11 AM, Andy Stewart <laz...@gm...> wrote: > Hi Maxime, > > Maxime Henrion <mhe...@gm...> writes: > >> Hello all, >> >> >> Yet another boring patch to update signal handlers to the new way of >> doing things, this time for the MenuItem and Item objects :-). There >> is, however, something of interest in this patch. > I have apply your patches, thanks! :) >> >> I was surprised to see that the "onToggle" and "afterToggle" signal >> handlers were actually bound to the signal name "toggled". Checking the >> GTK+ API documentation, it appears the correct signal name is really >> "toggle", without the final 'd' character. I suspect this code never >> worked before, but that noone was unlucky enough to actually hit it. >> Therefore, I corrected the signal name in the new signal handler as well >> as in the deprecated signal handlers. > Yes, it's a bug, thanks fix. :) >> >> Finally, another question comes to mind: why is the "activate" signal >> bound by the "activateLeaf" signal handler? Should I take advantage of >> this patch to rename it to just "activate", or "menuItemActivate" if >> there are namespace conflicts? > Yes, menuItemActivate is right signal name. > But after we change to 'menuItemActivate' it conflict with function > 'menu_item_activate' that emit 'activate' signal. > > So we change function 'menuItemActivate' (menu_item_activate) to > 'menuItemEmitActivate' avoid conflict. > > Please see detail in patches i push. :) So, you actually qualified every signal handler with the object name; I thought we were only doing this in case of namespace conflicts, which is why I didn't do it. However, there's a problem in your patch, as far as I can see: you have prefixed the "select", "deselect" and "toggle" signals with "menuItem", while they are actually signal handlers for the Item object and not the MenuItem one. So I think they should have been named "itemSelect", "itemDeselect" and "itemToggle" respectively. Cheers, Maxime |
From: Andy S. <laz...@gm...> - 2010-10-25 15:24:22
|
Hi Maxime, Maxime Henrion <mhe...@gm...> writes: >>> Finally, another question comes to mind: why is the "activate" signal >>> bound by the "activateLeaf" signal handler? Should I take advantage of >>> this patch to rename it to just "activate", or "menuItemActivate" if >>> there are namespace conflicts? >> Yes, menuItemActivate is right signal name. >> But after we change to 'menuItemActivate' it conflict with function >> 'menu_item_activate' that emit 'activate' signal. >> >> So we change function 'menuItemActivate' (menu_item_activate) to >> 'menuItemEmitActivate' avoid conflict. >> >> Please see detail in patches i push. :) > > So, you actually qualified every signal handler with the object name; > I thought we were only doing this in case of namespace conflicts, > which is why I didn't do it. However, there's a problem in your > patch, as far as I can see: you have prefixed the "select", "deselect" > and "toggle" signals with "menuItem", while they are actually signal > handlers for the Item object and not the MenuItem one. So I think > they should have been named "itemSelect", "itemDeselect" and > "itemToggle" respectively. We not just change signal name when it conflict with other module. We need add object prefix if signal name is too generic. The object prefix in signal name just a prefix to avoid conflict, not any sense for signal. When you use 'menuItemSelect', you can consider it's a signal 'select' in module 'MenuItem' If we change signal to 'itemSelect', how do you handle signal name if GTK+ add *new* module 'Item' with same signal 'select'? :) -- Andy |
From: Maxime H. <mhe...@gm...> - 2010-10-25 15:38:01
|
On Mon, Oct 25, 2010 at 5:24 PM, Andy Stewart <laz...@gm...> wrote: > Hi Maxime, > Maxime Henrion <mhe...@gm...> writes: > >>>> Finally, another question comes to mind: why is the "activate" signal >>>> bound by the "activateLeaf" signal handler? Should I take advantage of >>>> this patch to rename it to just "activate", or "menuItemActivate" if >>>> there are namespace conflicts? >>> Yes, menuItemActivate is right signal name. >>> But after we change to 'menuItemActivate' it conflict with function >>> 'menu_item_activate' that emit 'activate' signal. >>> >>> So we change function 'menuItemActivate' (menu_item_activate) to >>> 'menuItemEmitActivate' avoid conflict. >>> >>> Please see detail in patches i push. :) >> >> So, you actually qualified every signal handler with the object name; >> I thought we were only doing this in case of namespace conflicts, >> which is why I didn't do it. However, there's a problem in your >> patch, as far as I can see: you have prefixed the "select", "deselect" >> and "toggle" signals with "menuItem", while they are actually signal >> handlers for the Item object and not the MenuItem one. So I think >> they should have been named "itemSelect", "itemDeselect" and >> "itemToggle" respectively. > We not just change signal name when it conflict with other module. > We need add object prefix if signal name is too generic. > > The object prefix in signal name just a prefix to avoid conflict, not any > sense for signal. > > When you use 'menuItemSelect', you can consider it's a signal 'select' in > module 'MenuItem' Well, this is unlike every single method in the gtk2hs source which are all prefixed with the object name they apply to. Naming those signal handlers "menuItemSelect" when they actually deal with the Item object seems dangerously misleading and a lot less logical to me, plus it seems to violate the existing gtk2hs naming conventions. > If we change signal to 'itemSelect', how do you handle signal name if GTK+ add > *new* module 'Item' with same signal 'select'? :) I don't think this could ever happen given that there already is an existing GtkItem object. They would have namespace conflicts on their side before it would be a problem for us. Cheers, Maxime |
From: Axel S. <Axe...@in...> - 2010-10-25 18:00:55
|
Hi Maxime, On Oct 24, 2010, at 22:38, Maxime Henrion wrote: > Hello all, > > > Yet another boring patch to update signal handlers to the new way of > doing things, this time for the MenuItem and Item objects :-). There > is, however, something of interest in this patch. > > I was surprised to see that the "onToggle" and "afterToggle" signal > handlers were actually bound to the signal name "toggled". Checking > the > GTK+ API documentation, it appears the correct signal name is really > "toggle", without the final 'd' character. I suspect this code never > worked before, but that noone was unlucky enough to actually hit it. > Therefore, I corrected the signal name in the new signal handler as > well > as in the deprecated signal handlers. > Yes, quite possible that this code was always broken! > Finally, another question comes to mind: why is the "activate" signal > bound by the "activateLeaf" signal handler? Should I take advantage > of > this patch to rename it to just "activate", or "menuItemActivate" if > there are namespace conflicts? > I might have chosen that name to distinguish it from other occurrences of "activate". Since we now use prefixes, you should use "menuItemActivate". Thanks for fixing these, Axel > Cheers, > Maxime > < > menuitem > .patch > > > ------------------------------------------------------------------------------ > Nokia and AT&T present the 2010 Calling All Innovators-North America > contest > Create new apps & games for the Nokia N8 for consumers in U.S. and > Canada > $10 million total in prizes - $4M cash, 500 devices, nearly $6M in > marketing > Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi > Store > http://p.sf.net/sfu/nokia-dev2dev_______________________________________________ > Gtk2hs-devel mailing list > Gtk...@li... > https://lists.sourceforge.net/lists/listinfo/gtk2hs-devel |