GNOME Bugzilla – Bug 620837
[GtkBuilder transition] create_tag_dialog
Last modified: 2010-06-19 15:44:37 UTC
The create_tag_dialog needs to be migrated to GtkBuilder.
Instructions on how to fix this can be found here: http://f-spot.org/GtkBuilder_Transition Please indicate below if you are working on this (and if you stopped working on it), so we can avoid duplicate work. If you are stuck, just ask for help on IRC or the mailing list.
I'd like to work on this. Will start tuesday night, if it's not done already until that time :). So I'd be happy if you assign it to me, Ruben.
Allright, assigned! Let me know if the instructions were helpful. I might add a bonus section in a bit about moving the event hookups from GtkBuilder to code, to prevent compiler warnings. But let's focus on a normal conversion first. Good luck!
Hi Ruben. The instruction were definitely helpful. The only thing which was missing in my case, was the package glade-gnome for the required libraries in the f-spot.glade. I wonder why these weren't installed through apt-get build-dep f-spot. But probably because they're not needed for the build itself. I'm almost done. I replaced the old OptionMenu by a ComboBox. The tag-names and icons already appear and the functionality is working. The only thing that's missing is the correct indentation of the entries. Will try to finish that tomorrow night.
Created attachment 163256 [details] [review] Replaced create_tag_dialog with GtkBuilder version. Migration to GtKBuilder is finished. Let me know if the way I handled the new ComboBox is good, or if some improvements are needed. I'm not completely sure myself, if I'm satisfied with the current solution :-) The patch comes in 3 commits. Any quick advice how I can merge the 3 commits into one patch? Anyways, for now please get the remaining 2 patches in the next attachments.
Created attachment 163257 [details] [review] Fixes the indentation and alignment in the new create_tag_dialog
Created attachment 163258 [details] [review] Fixes formatting issues
I'm getting a warning saying, that the Pixbuf should be disposed. How do I actually do that? Should I keep an array of all the Pixbufs I get from Category.SizedIcon and dispose them after the dialog was closed?
Actually I like to have it in multiple small patches, makes it much easier to review. Using a tool such as git-bz makes this much easier btw. Pixbufs are tricky. They have unmanaged memory: if you don't Dispose() them, they will leak and memory will keep on growing. So yes, they have to be disposed explicitly. Will review your patches soon.
Ok, I was just a bit confused, because the documentation doesn't mention a Gdk.Pixbuf.Dispose method. But it seems to exist. Anyway I just realized that I don't create or delete them and the lost reference must happen somewhere else. I'm figuring it out. Also it seems that the current implementation of populating the ComboBox is a bit complicated. I'll try to refactor it. So don't rush with the review, I might add another patch to it. About the multiple patches: I know it's better to have several small patches. The reason why I asked was, that the 3rd one was just formatting, which I could have done right from the beginning :)
Created attachment 163338 [details] [review] Combobox now defaults to 'root' category instead of nothing. Minor fix as stated in the brief description of the patch. For the announced refactoring: I'm in the middle of doing that. Unfortunately I have no time the next 3 days, so it might take a little longer until that patch comes. Still, with the current patches everything should work just fine. The Pixbuf issues I mentioned earlier are actually not produced my me. I confused that. Btw where can I actually find the code formatting guidelines? I'm always wondering how I should actually format my stuff, because the existing code seems to be a bit inconsistent.
Review of attachment 163256 [details] [review]: Aha, I see you also replaced the obsolete OptionMenu. Very nice! Very clean patch too, merging in a second.
Review of attachment 163257 [details] [review]: Good patch, but mind the placement of brackets in code statements. Will fix those. ::: src/TagCommands.cs @@ +79,2 @@ foreach (Category category in categories) + { Please put this closing bracket on the same line as the foreach statement (K&R style). @@ +151,3 @@ } + else + { Same here: } else {
Review of attachment 163258 [details] [review]: Looks good.
Review of attachment 163338 [details] [review]: Nice attention to detail! ::: src/TagCommands.cs @@ +167,3 @@ } + else + { Same remark regarding style.
One general remark: your git isn't configured correctly, there's no email address configured. Please follow these instructions: (From http://live.gnome.org/Git/Developers#Setting_up_Git) > Setting up Git > > The first thing you should do is configure git to know who you are: > > git config --global user.name "Rupert Monkey" > git config --global user.email rupert@example.com
Ignore that last remark, looks like it's corrected for the last two patches. I fixed it in the first two.
Very nicely done, merged! Thanks for cleaning up the deprecated widget as well, this needs to be done as well before GNOME 3.0 is there.
Oh and apologies for the late review, this was held up by the 0.7.0 release.
(In reply to comment #19) > Oh and apologies for the late review, this was held up by the 0.7.0 release. No problem at all. I followed the events on irc etc., so I knew that it would happen some time after the 0.7.0 release. About the refactoring: I stopped it for now because I wondered if it didn't make more sense to actually ditch the whole dialog at some point in time and integrate the tag-creation in the tags-view itself. Just an idea... Btw I really like how you drive this project. It's really motivating! As soon as time allows again, I'll work on the next patch.