GNOME Bugzilla – Bug 637835
[PATCH] Object separation for glade preview feature
Last modified: 2011-02-09 07:15:14 UTC
Created attachment 176901 [details] [review] Object separation for glade preview feature The attached patch implements object separation for the glade preview feature. It makes glade launch only one preview per toplevel widget and kills the corresponding previewer process if the widget is removed from the project. We need this phase ready, so we'll be able to start implementing the update communication protocol between glade and glade-previewer. Please review it.
Created attachment 176951 [details] [review] Preview improvements This patch improves on my previous one. It implements the update protocol between glade and glade-previewer. Please review it.
IRC log from online review session: <tristan> only I expect it to do more :) <Marco> ok <tristan> the problem I have with it is that it makes GladeProject still do too much <tristan> its the responsability I wanted to offload to the GladePreview object * tristan opens patch in webpage... <tristan> so, I want to remove: glade_preview_add/remove_watch mainly <tristan> possibly the _get_pid() also <tristan> however... the _get_pid() is okay... so long as it's only used for a hash entry <tristan> (i.e. to compare against other running previews) <Marco> andok <tristan> other remarks... <tristan> glade_preview_kill_preview and glade_preview_launch_preview should probably just drop the extra _preview() suffix <tristan> glade_preview_update() should probably take a new GtkWidget argument <tristan> however... actually <tristan> I'd rather stay away from GtkWidget arguments and use GladeWidget arguments as much as possible <Marco> ok <Marco> anything else? <tristan> lastly... GladePreview object should declare a signal... so notify that the child exited <tristan> but, I figure you would figure that by yourself... since the project wont get any add_watch anymore :) <Marco> ok <tristan> Marco, and yes... I've been thinking... I'm not sure if you'll like this idea or not... what do you think about dropping the preview feature from 3.8 ? <tristan> 2 incentives for doing this * pablog (~pablog@212.225.182.136) has joined #glade3 <tristan> a.) I would rather have you spending time doing cool stuff like implementing theming/desktop settings configuration <tristan> ... than have you frustrated and porting your code back and forth between branches <tristan> ... which is time consuming as specially after my huge refactor <tristan> and b.) ... it would just be something to make Glade 3.10 for GTK+ 3.0 that much cooler :) <Marco> :) <Marco> I'll try to get it done <Marco> I'd really like to get it in 3.8 <Marco> I think there's enough time and it wont cause any feature bloat <tristan> well... you realize I plan to release 3.8 and 3.10 in parallel ? <tristan> i.e. 3.10 is not a future date <Marco> hmmm <Marco> did not know that <Marco> I'd like it in gnome 3.0 <tristan> right, 3.8 is the last Glade for GTK+ 2.x <tristan> 3.10 will support GTK+ 3.0 and have no support for libglade and old deprecated widgets <tristan> (that are no longer accessible in 3.0) <tristan> basically 3.10 is the future and 3.8 is the support release <tristan> 3.10 will be for gnome 3.0 yes <Marco> hmmmm <Marco> ok, <tristan> Marco, either way... at some point we're going to have to stop backporting features from master to glade-3-8 <Marco> then I think it will require a lot less work if I only care about 3.10 <tristan> well, anyway I think it's a wasted effort to try to put it in both <tristan> yeah I much agree <Marco> should rebase my patches on master? <Marco> *shoud I? <tristan> Marco, are you building GTK+ 3.0 from git ? <Marco> yes <Marco> well, I use jhbuild <tristan> you will need 'toplevel-embedding' branch from GTK+ git to operate Glade master <tristan> for now <tristan> until it lands, hopefully very soon <tristan> Marco, and well... I'm sorry to say that... you will have serious conflicts :( <Marco> so, I think I'll wait until the dust sets <tristan> Marco, oh and one more thing... please license your glade-preview.c with LGPL, and the previewer.c also if you'd be so kind. <Marco> ok <tristan> thanks <Marco> tristan, can you post these remarks about my patches on bugzilla (or else I may forget some points)?
Created attachment 177963 [details] [review] Preview improvements Nothing new here. Just ported the patch to glade master, ie glade 3.9.0.
Created attachment 179551 [details] [review] Object separation for glade preview feature Object separation is half done now. The patch has been ported to glade 3.9.1. Hope to bring more updates soon.
Created attachment 179571 [details] [review] Object separation for glade preview feature Object separation for the preview feature is done. Please review it.
Created attachment 179572 [details] [review] Object separation for glade preview feature Fixes a double unref in previous patch. Please review it.
Thanks for updating the patch. Few comments: - The attached patch does not include the glade-preview.[ch] files (or the private glade-preview-tokens.h file it seems you've added). So I cant really review it... if you do a local git commit of your with the entire patch in one commit... you can generate the diff by just saying 'git show > preview.patch'. - There are still g_print() debug traces which should be removed. - I'm seeing that there is still a GladePreview api 'glade_preview_exit' which you are explicitly calling when removing the preview object from the project's hash table. I don't think this api is needed, the project should only have to unref the preview object... when the preview object finalizes... it should know that it needs to kill it's preview process. - The preview "live update" feature should not go in the project properties dialog... whether to update previews "live" should be a user preference (we still dont have a user preferences dialog but we definitely need to add one). Project properties dialog only shows things that are serialized into the project data... a Glade user preferences dialog on the other hand should show options that are serialized into the Glade session data (glade_app_config_load/save). A user preferences dialog should be implemented at the GladeApp level and shown via Edit->Preferences, however I would prefer that for now we leave out the live editing feature and try to land your object separation patch first. Furthermore... if and when we do live/real time updates of running previews... we can probably optimize this by only serializing the portions of the Glade file that are needed to show that toplevel. (there is a chance that live preview updates can work even if the glade file has 15 toplevels and 2000 widgets or such, by just reducing the amount of objects that get serialized for the update).
Created attachment 179606 [details] [review] Object separation for glade preview feature (In reply to comment #7) > - The attached patch does not include the glade-preview.[ch] > files (or the private glade-preview-tokens.h file it seems > you've added). Forgot to git add it. It is now included in the updated patch. Please review it. > - There are still g_print() debug traces which should > be removed. Removed. > - I'm seeing that there is still a GladePreview api > 'glade_preview_exit' which you are explicitly calling > when removing the preview object from the project's > hash table. You mean glade_preview_kill? > A user preferences dialog should be implemented at the GladeApp > level and shown via Edit->Preferences, however I would prefer that > for now we leave out the live editing feature and try to land > your object separation patch first. Ok, but I'll leave it in the patch for now. > Furthermore... if and when we do live/real time updates of running > previews... we can probably optimize this by only serializing > the portions of the Glade file that are needed to show that toplevel. > (there is a chance that live preview updates can work even if the > glade file has 15 toplevels and 2000 widgets or such, by just reducing > the amount of objects that get serialized for the update). Ok. What must be done to serialize only a selected toplevel?
Ok, so lets try to land this as soon as possible so we can move on. To land this patch: a.) We need the header files listed somewhere in Makefile.am even though they are private (they go in noinst_HEADERS) b.) Lets drop auto-update feature at least for now, and remove the changes to the project properties dialog c.) Remove glade_preview_kill(), this should be implied by disposing the preview object d.) The preview needs to keep track of the io watch source id returned by g_child_watch_add() e.) The when explicitly killing the preview, you must g_source_remove (the_child_watch_source_id), since the child will exit at an unpredictable time... you need to do this before sending the kill command to the child process, without removing the watch source we risk firing the signal on an object that is already destroyed. f.) In GladeProject, seems you still have some 8 space source indentation going on, please fix the indentation to match the rest of the file. g.) From what I see, you are blocking the "exited" signal when unreffing the preview. Instead you should completely disconnect it. This is not because the signal will fire (because you took care of the problem "e" above, the signal wont fire), but just to make sure that there is no further reference held implicitly by the signal connection. That should pretty much wrap it up for this patch.
Created attachment 179608 [details] [review] Object separation for glade preview feature Done. The only thing I didn't do was to remove the auto-update feature. All the other point have been fixed. Please review it.
Created attachment 179610 [details] [review] Object separation for glade preview feature Included Changelog entry. Re-licensed some files to LGPL. Please review it.
Ok lets try again: a.) You still didnt remove the auto update stuff, please just remove that for now. b.) In glade_project_remove_object(), you have code that pulls the preview from the hash table and then unrefs it... instead the GDestroyNotify that you pass to g_hash_table_new_full() for the preview hash should be a function that removes the "exits" signal callback and then unrefs the preview. And in this case, instead of unreffing the preview and leaving an invalid pointer reference in the hash table, just calling g_hash_table_remove() will result in the unreffing of the object. c.) Please remove glade_project_add_preview() function, calling g_hash_table_insert() directly is fine. d.) Im still seeing places in GladeProject with bad indentation, example: @@ -745,8 +771,10 @@ glade_project_init (GladeProject * project) priv->first_modification = NULL; priv->first_modification_is_na = FALSE; - priv->preview_channels = - g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL); + priv->previews = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, NULL); + priv->auto_update_preview = FALSE; +
Created attachment 180367 [details] [review] Object separation for glade preview feature Whats has been done: - Removed auto-update stuff. - Removed glade_project_add_preview. - Fixed indentation issues. Previews are still been removed just like before: g_hash_table_find is called and then g_object_unref is used. This does not leaves an invalid pointer in the table: when the object is unrefed, glade_project_preview_exits is called, which then removes it from the hash table. I see no problem on the way it is done right now.
The problem that you don't see (besides code readability and clarity, which is also very important) is as follows... most notably the preview object is leaked when the user closes the preview explicitly as far as I can see. - If the user hits the X button then the child watch source fires - From the child watch source you emit the CHILD_EXIT signal - From glade-project you don't unref the preview from the CHILD_EXIT signal Futhermore... since you are calling the CHILD_EXIT explicitly from the dispose function... if the project did indeed unref the preview from the child_exits handler... the signal would fire again... probably making some unclear codepaths run. Things seem to work right for now because the preview is simply leaked in this case. So, how to do this cleanly: - Create a GDestroyNotify function to pass to the hash table a g_hash_table_new() time to bookkeep the preview. void destroy_preview (GladePreview *preview) { g_signal_handlers_disconnect_by_func (preview, child_watch_handler, project); g_object_unref (preview); } - When creating the preview, just insert the preview into the hash table and add the signal connection. - From glade-preview.c... do NOT fire the exited signal from the dispose handler... simply remove the iochannel and the child watch source from there if (preview->priv->watch != 0). - From the glade-preview.c child watch function... before emitting the CHILD_EXITED signal set preview->priv->watch = 0 (the child watch source is a one time GSource, after it runs once the GSource is automatically removed). This way we get the following reliable behavior: - If the GladeProject disposes... we call g_hash_table_destroy() on the preview hash and all previews get disposed. - This causes the GladePreview dispose code to cleanly and silently kill the previews that are running. - If the glade-previewer program exits, this causes the child watch source to fire and the CHILD_EXITED signal to fire - GladeProject will remove that preview from the hash, causing the signal to disconnect and the preview object to be unreffed - Again, the GladePreview object will be disposed but now does not need to emit the CHILD_EXITED signal. This means there is no need for glade_project_kill_previews(), since simply removing the preview from the hash table in a straight forward way results in the preview being destroyed with the proper behavior. It's also questionable if we need to do anything at all at project "close" time... the GladeProject should simply be calling g_hash_table_destroy() at GladeProject->dispose time... once before recursively removing all the objects from the project. One other thing is that glade_preview_launch(), goes awry where it calls 'goto end' for a failed preview... seems this is simply a typo in your code... connecting the child watch source obviously should come before 'end:' in your code.
What about completely removing the hash table and add a property in GladeWidget to store a pointer to its preview? I think that would make a lot of things simpler.
Created attachment 180410 [details] [review] Object separation for glade preview feature Destruction is now been done the way it should be. Please review it.
Created attachment 180413 [details] [review] Object separation for glade preview feature Complaints about the patch have been addressed. Please review it.
I committed the patch finally. I made some minor changes: - At project close time... we were still using a g_hash_table_foreach() to explicitly kill previews, our destroy notify function of the hash table does this for us so now we simply g_hash_table_destroy() at dispose time before unparenting all the widgets. - There was still a complex lookup to find the preview on a widget when launching previews from glade-project.c, since we are using the qdata to bookkeep a widget's preview this was unneeded, so I was able to remove the related code by using the qdata instead. Other than that, just a few source indentation was changed, I also changed GladePreview apis to take the preview object first as this is a standard of GObject programming (i.e. glade_preview_update should take the preview object first). And the preview apis that take a text string now take a const gchar *. Note, since this preview object is internal to the glade core I'm going to make it private (which means simply prefixing the apis with the '_' and putting it into a private header).
And this is committed in Glade master now :) Thanks.