Hi Matthew, I'm a bit concerned about the changed ui_lookup_widget (and hookup) functions - these are in the plugin API and can be used independently from Glade. (plugin) API function behaviour should not normally change, but it seems the owner parameter is now ignored. If we are caching lookups this is probably wrong as the widget may get destroyed.
I haven't looked at the new implementation but perhaps you could help explain the changes.
On 11-10-17 05:22 AM, Nick Treleaven wrote:
Hi Matthew, I'm a bit concerned about the changed ui_lookup_widget (and hookup) functions - these are in the plugin API and can be used independently from Glade. (plugin) API function behaviour should not normally change, but it seems the owner parameter is now ignored. If we are caching lookups this is probably wrong as the widget may get destroyed.
I haven't looked at the new implementation but perhaps you could help explain the changes.
Long story short: compatibility is the reason.
Typical short story long:
I didn't want to break all the existing code in core and plugins that were using ui_lookup_widget/ui_hookup_widget() functions, so I dropped the (now) pointless first parameter. There's no need to associate things with a parent/owner widget since all objects are required to have unique names in GtkBuilder (and the GHashTable).
I'm not 100% sure what you mean by caching lookups or widgets being destroyed. I'll try and explain how it works and hopefully we can get some more experienced developer's eyeballs on the code to review it (that's you!).
So the first function called is ui_init_builder() and what it does is to initialize the GtkBuilder so that it creates all of the GObjects. Then it iterates over all of the objects in the GtkBuilder and stores their pointers into a GHashTable (also ref'ing them) using their names as the key. This is basically a "drop-in" replacement for the old Glade 2 generated code. Replacing the hookup and lookup functions are the new ui_lookup_object() and ui_hookup_object() functions. The former just does a g_hash_table_lookup() to find the GObject with the associated key(name). The latter just inserts the object into the hash table using the name as the key (and ref'ing it).
It's my understanding that all objects that were hooked up were eternal until program close, though if this is not the case, we'll be leaking exactly 1 GObject per object that the user/plugin code destroys/unrefs since the GHashTable is holding a ref on it. It would be quite trivial to add a function to remove objects from the hash table, but existing code wouldn't be using this obviously. We could also not ref user-added object and only ref those from GtkBuilder if we wanted.
In a perfect world, we could drop the GHashTable and use GtkBuilder directly to track the objects, but it seems GtkBuilder can only add objects from an XML string. Actually, in a perfect world, we wouldn't be creating widgets outside of the GUI file at all and so we could use GtkBuilder to track all the objects.
So the basic plan I had thought up is to do these steps:
1. Convert the Glade 2 to Glade 3 (done) 2. Get rid of the old generated code with some compat code (done) 3. Separate out all the hard-coded GUI stuff and move it into the proper place, the GtkBuilder file (not even remotely done) 4. Stop adding new code that mixes UI and business logic (todo)
Only the first 2 are essential to do now to actually switch to GtkBuilder, the next two can be done gradually over time.
I hope this clears up somewhat how the GtkBuilder stuff is currently working. If you want to review the implementation, it's quite trivial really, you can get a pretty clear view of the meat of it by using:
$ git diff master..gtkbuilder src/ui_utils.c
That should show pretty much only the implementation and a few other minor changes that have happened since.
Do let me know if you have any more questions, and feel free to hack away on the code as usual. I don't think any more experienced developers have really actually reviewed the code in-depth, so I would be grateful if you get the time. It's something that will need to happen before we merge into master.
In the meanwhile, I'll keep plugging away on the odd little bugs I notice resulting from the changes.
Cheers, Matthew Brush
On 11-10-17 04:09 PM, Matthew Brush wrote:
On 11-10-17 05:22 AM, Nick Treleaven wrote:
Hi Matthew, I'm a bit concerned about the changed ui_lookup_widget (and hookup) functions - these are in the plugin API and can be used independently from Glade. (plugin) API function behaviour should not normally change, but it seems the owner parameter is now ignored. If we are caching lookups this is probably wrong as the widget may get destroyed.
I haven't looked at the new implementation but perhaps you could help explain the changes.
Long story short: compatibility is the reason.
Lex has pointed out on IRC that in my typical fashion I haven't been clear enough :) so I would highly recommend just reading the code. It's really not tricky or complicated at all and it's quite short.
Cheers, Matthew Brush
On 18/10/2011 00:09, Matthew Brush wrote:
On 11-10-17 05:22 AM, Nick Treleaven wrote:
Hi Matthew, I'm a bit concerned about the changed ui_lookup_widget (and hookup) functions - these are in the plugin API and can be used independently from Glade. (plugin) API function behaviour should not normally change, but it seems the owner parameter is now ignored. If we are caching lookups this is probably wrong as the widget may get destroyed.
I haven't looked at the new implementation but perhaps you could help explain the changes.
Long story short: compatibility is the reason.
I understand that, but see below.
Typical short story long:
I didn't want to break all the existing code in core and plugins that were using ui_lookup_widget/ui_hookup_widget() functions, so I dropped the (now) pointless first parameter. There's no need to associate things with a parent/owner widget since all objects are required to have unique names in GtkBuilder (and the GHashTable).
Here I think is the problem. As I mentioned earlier, ui_[lh]ookup_widget can be used *without* Glade or GtkBuilder! This is mentioned in the 'GUI example' for Stash (scroll down):
http://www.geany.org/manual/reference/stash_8h.html#_details
There is no reason why the owner dialog can't be destroyed, and also no reason why plugin API users have to use globally unique names for widgets.
So this change breaks existing API behaviour, and in addition I think those features are things that are good to support anyway.
So I think we need to restore this behaviour for those functions. ui_[lh]ookup_object should either be updated similarly or renamed to something like ui_builder_lookup to be clear about the difference.
I'm not 100% sure what you mean by caching lookups or widgets being destroyed.
Hopefully I've explained this above.
So the first function called is ui_init_builder() and what it does is to initialize the GtkBuilder so that it creates all of the GObjects. Then it iterates over all of the objects in the GtkBuilder and stores their pointers into a GHashTable (also ref'ing them) using their names as the key. This is basically a "drop-in" replacement for the old Glade 2 generated code. Replacing the hookup and lookup functions are the new ui_lookup_object() and ui_hookup_object() functions. The former just does a g_hash_table_lookup() to find the GObject with the associated key(name). The latter just inserts the object into the hash table using the name as the key (and ref'ing it).
It's my understanding that all objects that were hooked up were eternal until program close, though if this is not the case, we'll be leaking exactly 1 GObject per object that the user/plugin code destroys/unrefs since the GHashTable is holding a ref on it. It would be quite trivial to add a function to remove objects from the hash table, but existing code wouldn't be using this obviously. We could also not ref user-added object and only ref those from GtkBuilder if we wanted.
In a perfect world, we could drop the GHashTable and use GtkBuilder directly to track the objects, but it seems GtkBuilder can only add objects from an XML string. Actually, in a perfect world, we wouldn't be creating widgets outside of the GUI file at all and so we could use GtkBuilder to track all the objects.
So the basic plan I had thought up is to do these steps:
- Convert the Glade 2 to Glade 3 (done)
- Get rid of the old generated code with some compat code (done)
- Separate out all the hard-coded GUI stuff and move it into the proper
place, the GtkBuilder file (not even remotely done) 4. Stop adding new code that mixes UI and business logic (todo)
Only the first 2 are essential to do now to actually switch to GtkBuilder, the next two can be done gradually over time.
I hope this clears up somewhat how the GtkBuilder stuff is currently
Thanks for explaining, but I think my suspicions were correct.
working. If you want to review the implementation, it's quite trivial really, you can get a pretty clear view of the meat of it by using:
$ git diff master..gtkbuilder src/ui_utils.c
That should show pretty much only the implementation and a few other minor changes that have happened since.
Thanks for that command, I hadn't learnt how to do that yet ;-)
On 11-10-18 09:05 AM, Nick Treleaven wrote:
On 18/10/2011 00:09, Matthew Brush wrote:
I didn't want to break all the existing code in core and plugins that were using ui_lookup_widget/ui_hookup_widget() functions, so I dropped the (now) pointless first parameter. There's no need to associate things with a parent/owner widget since all objects are required to have unique names in GtkBuilder (and the GHashTable).
Here I think is the problem. As I mentioned earlier, ui_[lh]ookup_widget can be used *without* Glade or GtkBuilder! This is mentioned in the 'GUI example' for Stash (scroll down):
http://www.geany.org/manual/reference/stash_8h.html#_details
ui_[hl]ookup_object() are not used with GtkBuilder or Glade[1], they access an internal hash table. GtkBuilder is completely out of the picture by the end of the ui_init_builder()[2] function. Glade3/GtkBuilder does require unique names (as did glade 2 IIUC, since all the names were unique), but the only reason those two functions currently do also is because the names are used as the key in a hash table, and so must be unique. If you look back in the history you should see where I started out with using a GList but eventually moved to a GHashTable since it's a better data structure for the purpose.
There is no reason why the owner dialog can't be destroyed, and also no reason why plugin API users have to use globally unique names for widgets.
I still don't understand what destroying the owner dialog has to do with anything. TBH I don't even know what use the owner widget parameter had in those functions, was it just to provide a "namespace" so that widget names didn't clash between plugins? As for the globally unique names, I guess this could be a problem if some new code was using the pointless ui_[hl]ookup_object() functions to associate a completely arbitrary "name" with a GObject (not *it's* name like gtk_widget_set_name(), just *a* name - used to lookup the object in the GHashTable).
So this change breaks existing API behaviour, and in addition I think those features are things that are good to support anyway.
It might yes, especially if different plugins were calling ui_hookup_widget() with clashing names (which they don't).
So I think we need to restore this behaviour for those functions. ui_[lh]ookup_object should either be updated similarly or renamed to something like ui_builder_lookup to be clear about the difference.
It used to be called ui_add_object() but then I renamed it :) ui_builder_lookup() is misleading since it's not looking up anything in the builder, which has long since been destroyed. It's looking up in the GHashTable which is only there to provide backwards compatibility with old code.
I'm not 100% sure what you mean by caching lookups or widgets being destroyed.
Hopefully I've explained this above.
Not really (I'm kinda stupid remember :) I think I'm confused about how it used to work and you're confused about how it works now :)
I'm not too familiar with the stash library, but aside from it, what other purpose do the ui_[lh]ookup_widget() functions serve that couldn't be accomplished with the way plugins typically use widgets[3]? I know glade generated code used to have lookup/hookup functions for some reason, is it to do with that?
I looked in the Geany-Plugin code, and there's no use of ui_hookup_widget() and most uses of ui_lookup_widget() are using a geany->main_widgets->foo widget as the owner, which is fine since the widgets that came from Geany's Glade file are guaranteed to have unique names.
P.S. Are you ever on IRC or IM? It's hard to have this type of discussion in 1 message per day email communications :)
Cheers, Matthew Brush
[1] here's the lifecycle of the GtkBuilder instance: - enter the ui_init_builder() function - initialize a gtkbuilder instance - add the xml file to parse to the gtkbuilder - extract all the newly parsed/initialized gobjects - add those objects to the ghashtable - destroy the gtkbuilder instance - leave ui_init_builder() [2] https://github.com/geany/geany/blob/gtkbuilder/src/ui_utils.c#L2116 [3] that is, holding a pointer and destroying the widget when the plugin is unloaded.
On 11-10-18 01:33 PM, Matthew Brush wrote:
Not really (I'm kinda stupid remember :) I think I'm confused about how it used to work and you're confused about how it works now :)
I think I see what you're talking about, ui_hookup_widget() is attaching the widget to the owner Gobject's datalist. Ok, I think to avoid fixing all of Geany's code that uses this, I will make the new ui_hookup_object() to do this same thing as well, then we can worry more about getting rid of what's left once we port the rest of the UI to GtkBuilder.
I'll report back once I've got it working and we can go from there.
Cheers, Matthew Brush
On 18/10/2011 22:57, Matthew Brush wrote:
On 11-10-18 01:33 PM, Matthew Brush wrote:
Not really (I'm kinda stupid remember :) I think I'm confused about how it used to work and you're confused about how it works now :)
I think I see what you're talking about, ui_hookup_widget() is attaching the widget to the owner Gobject's datalist. Ok, I think to avoid fixing all of Geany's code that uses this, I will make the new ui_hookup_object() to do this same thing as well, then we can worry more about getting rid of what's left once we port the rest of the UI to GtkBuilder.
I'll report back once I've got it working and we can go from there.
I'm currently fighting off flu so can't look at the new branch yet.
Just to make sure we understand each other; basically, the ui_[l]ookup_widget changes should be reverted. Then, if ui_lookup_widget fails, for compatibility it can try your hashtable lookup.
New code that wants to lookup a widget created from geany's glade xml can use your new function to lookup an object from the hashtable. Looking up some non-geany non-global widget still needs to use ui_[lh]ookup_widget.
BTW name clashes are definitely possible if we only use your global hashtable, which is why we need the old functions to work like they used to. Even if it works with the global lookup now doesn't mean it's doing the right thing.
I don't think I misunderstood any of your explanations.
I'm not on IRC but I could use IM maybe. I'm usually online between 12pm - 6pm monday-friday UK time.
Regards, Nick
On 10/20/2011 07:53 AM, Nick Treleaven wrote:
On 18/10/2011 22:57, Matthew Brush wrote:
On 11-10-18 01:33 PM, Matthew Brush wrote:
Not really (I'm kinda stupid remember :) I think I'm confused about how it used to work and you're confused about how it works now :)
I think I see what you're talking about, ui_hookup_widget() is attaching the widget to the owner Gobject's datalist. Ok, I think to avoid fixing all of Geany's code that uses this, I will make the new ui_hookup_object() to do this same thing as well, then we can worry more about getting rid of what's left once we port the rest of the UI to GtkBuilder.
I'll report back once I've got it working and we can go from there.
I'm currently fighting off flu so can't look at the new branch yet.
Bummer. Hope you get better quickly!
Just to make sure we understand each other; basically, the ui_[l]ookup_widget changes should be reverted. Then, if ui_lookup_widget fails, for compatibility it can try your hashtable lookup.
Yep, in my gtkbuilder2[1] branch I have replaced the old functionality (same with lookup_widget() in stash.c). I didn't actually "revert" in the Git sense, but I did restore the old code for those functions.
New code that wants to lookup a widget created from geany's glade xml can use your new function to lookup an object from the hashtable. Looking up some non-geany non-global widget still needs to use ui_[lh]ookup_widget.
Nope, I dropped the hash table, now all GtkBuilder widgets get "registered" like they used to with ui_hookup_widget() that I put back. I also re-created the old Glade generated create_*() functions to provide access to the top level widgets. The hash table was only there for compatibility so if it's not going to work, no point in keeping it.
BTW name clashes are definitely possible if we only use your global hashtable, which is why we need the old functions to work like they used to. Even if it works with the global lookup now doesn't mean it's doing the right thing.
Right, though for the core, we *shouldn't* be using duplicate names for any widgets in the UI, since this isn't supported by GtkBuilder, but for now, until all the UI code gets ported into the Glade file, we do need to make sure it's supported to remain compatible.
I don't think I misunderstood any of your explanations.
Ok, wasn't sure since you said you didn't read the code and my English isn't very good :)
Cheers, Matthew Brush
On 11-10-17 05:22 AM, Nick Treleaven wrote:
Hi Matthew, I'm a bit concerned about the changed ui_lookup_widget (and hookup) functions - these are in the plugin API and can be used independently from Glade. (plugin) API function behaviour should not normally change, but it seems the owner parameter is now ignored. If we are caching lookups this is probably wrong as the widget may get destroyed.
I haven't looked at the new implementation but perhaps you could help explain the changes.
OK! I got this more or less fixed up and working like it used to. The one exception is some items from the editor menu and maybe a few other menu items. I think it's related to some hackery somewhere in the existing code that moving around menu items to share the same widgets in more than one menu.
I'm hoping someone more familiar with this part of the code can help me out here.
You can see the changes here:
https://github.com/codebrainz/geany/commits/gtkbuilder2/
There's still a few FIXME's and it needs to be cleaned up, but if you don't mind having a look to see if overall this is fixing the issues you raised.
Cheers, Matthew Brush
On 19/10/2011 08:47, Matthew Brush wrote:
On 11-10-17 05:22 AM, Nick Treleaven wrote:
Hi Matthew, I'm a bit concerned about the changed ui_lookup_widget (and hookup) functions - these are in the plugin API and can be used independently from Glade. (plugin) API function behaviour should not normally change, but it seems the owner parameter is now ignored. If we are caching lookups this is probably wrong as the widget may get destroyed.
OK! I got this more or less fixed up and working like it used to. The one exception is some items from the editor menu and maybe a few other menu items. I think it's related to some hackery somewhere in the existing code that moving around menu items to share the same widgets in more than one menu.
IMO it is necessary because there are often popup menu submenus that need to be identical, and maintaining duplicate menu items with Glade is a pain.
If your code does what Glade 2's interface.c did then I'm not sure why there would be incompatibilites.
I'm hoping someone more familiar with this part of the code can help me out here.
You can see the changes here:
https://github.com/codebrainz/geany/commits/gtkbuilder2/
There's still a few FIXME's and it needs to be cleaned up, but if you don't mind having a look to see if overall this is fixing the issues you raised.
Could you move that branch into the geany repo? It would be easier to look at.