GNOME Bugzilla – Bug 735053
[PATCH] Use GtkBuilder.
Last modified: 2014-09-16 15:16:16 UTC
Created attachment 283884 [details] [review] Use GtkBuilder. Now that the UI is frozen, let’s change the code! =D
Review of attachment 283884 [details] [review]: Looks straightforward. I like UI files. Ideally you would follow this up with a patch to split the menu into a separate UI file named menu.ui, since I don't like hiding the menu in the same file.
Created attachment 283898 [details] [review] Use GtkBuilder. I prefer doing all in one patch (using `git mv`…).
Review of attachment 283898 [details] [review]: I think iagno-menu.ui needs to be added to POTFILES.in.
Created attachment 283901 [details] [review] Use GtkBuilder.
Review of attachment 283901 [details] [review]: You forgot to 'git add data/iagno-menu.ui'
Created attachment 283939 [details] [review] Use GtkBuilder. You’re right, it could be useful… ^^’
Review of attachment 283939 [details] [review]: Thanks!
Created attachment 283995 [details] [review] Use GtkBuilder for preferences' dialog. Here is a patch for the preferences' dialog. The GtkDialog itself isn’t in the ui file for the use-header-bar flag to be switchable if requested.
Review of attachment 283995 [details] [review]: Nits: ::: src/iagno.vala @@ +487,3 @@ + { + // TODO graphical error + stderr.printf ("Could not load UI: %s\n", e.message); I would almost always use warning() or error() instead of stderr.printf(). In this case, error(), since something seriously horrible has gone wrong. error() will cause the program to crash. @@ +540,3 @@ catch (FileError e) { + // TODO graphical error? sensitivity? I would just change it to an error so that it crashes. (But in a different patch, not this one. New TODOs are appreciated, just not in unrelated patches. :)
Created attachment 284200 [details] [review] Use GtkBuilder for preferences' dialog. Using error() for the problems using UI files.
Review of attachment 284200 [details] [review]: Nits. Just set your next patch's status directly to accepted-commit-after-freeze. ::: src/iagno.vala @@ +97,1 @@ return; Actually you don't return, since error() causes the program to crash. :) @@ +487,3 @@ + { + error ("Could not load UI: %s\n", e.message); + return; Same here.
Created attachment 284227 [details] [review] Use GtkBuilder for preferences' dialog. Here is it!
Can you please rebase these?
Created attachment 286246 [details] [review] Use GtkBuilder. Pushing Iulian’s patches first was *really not* a good idea. ^^’
Created attachment 286247 [details] [review] Use GtkBuilder for preferences’ dialog. We really need to use GResource for menu and images, that will be probably the next bug opened.
Of course pushing his patches first was a good idea! I didn't want the newest contributor to have to deal with the merge conflicts, and you're no longer the newest contributor. :) Yeah, I actually thought that through. Slightly evil towards you; sorry about that. GResources are good for most programs, and we can certainly use one for our menu, but it's a bad fit for themeable games as that makes it dramatically more difficult for players to experiment with their own themes. User themes do occasionally make their way upstream (e.g. for 3.12 I replaced all but the high contrast themes in four-in-a-row with user-created themes) so we want to make sure those remain as easy as possible. In fact, we recently-ish changed Mahjongg to place its themes in a gresource; I'd like to undo that....
Thanks! P.S. I only branched a subset of the games today. I'll get to your accepted patches in the other games within the next week or so. Attachment 286246 [details] pushed as 210d59f - Use GtkBuilder.
Created attachment 286255 [details] [review] Bump Gtk+ required version to 3.13.4. (In reply to comment #16) > Of course pushing his patches first was a good idea! I didn't want the newest > contributor to have to deal with the merge conflicts, and you're no longer the > newest contributor. :) Yeah, I actually thought that through. Slightly evil > towards you; sorry about that. Yeah, I had in mind you thought of it. But this one was really a hard time. ^^ > GResources are good for most programs, and we can certainly use one for our > menu, but it's a bad fit for themeable games as that makes it dramatically more > difficult for players to experiment with their own themes. User themes do > occasionally make their way upstream (e.g. for 3.12 I replaced all but the high > contrast themes in four-in-a-row with user-created themes) so we want to make > sure those remain as easy as possible. In fact, we recently-ish changed > Mahjongg to place its themes in a gresource; I'd like to undo that.... In a program like Iagno (same doesn’t apply to Mahjongg), having a default theme loaded via GResource looks like a good idea (not only thinking of removing the theme engine). Here comes a patch that puts the three new svgs and the UI files in a GResource, with autoloading of the app-menu. First patch just bumps Gtk+ version.
Created attachment 286256 [details] [review] Use GResource.
Looks like I failed to reopen, and I don’t understand how to do that. xD
Created attachment 286258 [details] [review] Use GResource. Forgot the preferences’ dialog.
Created attachment 286259 [details] [review] Use GResource. Of course, I forgot to re-add the gresource file. ^^’
Created attachment 286288 [details] [review] Using Gtk namespace in iagno.vala. A little cleaning, now that the GtkBuilder patches has landed. ^^
Created attachment 286289 [details] [review] Let startup() do its work instead of activate(), and reorder. Here is a patch that puts the launching functions in the order they are called, so it’s quite more understandable; and so, that corrects the little hack of having all the code in activate() instead of in startup() but with a `if (window != null) return;` (so that the window could only be launched once).
Review of attachment 286259 [details] [review]: I hesitate to use a GResource for dark.svg and light.svg as that would have to be undone to fix bug #736693. The rest looks good.
Review of attachment 286288 [details] [review]: No, I think this makes the code harder to read. :(
Review of attachment 286289 [details] [review]: Yup! Marking as "reviewed" since it probably needs rebased.
Comment on attachment 286255 [details] [review] Bump Gtk+ required version to 3.13.4. Yup
(In reply to comment #25) > I hesitate to use a GResource for dark.svg and light.svg as that would have to > be undone to fix bug #736693. The rest looks good. It could be at every moment replaced with gtk_image_set_from_icon_set(), if I’m not mistaken. Having an UI file complete is quite better than having to look many files to know what you are working on…
Created attachment 286302 [details] [review] Let startup() do its work instead of activate(), and reorder. Yeah, the GResource patch should in my mind be pushed like this, whatever happens to themes. Here is an updated “reorder function” patch.
I don't think treating the pieces as icons is appropriate. But OK.... Attachment 286255 [details] pushed as ae661bb - Bump Gtk+ required version to 3.13.4. Attachment 286259 [details] pushed as a4c6c10 - Use GResource. Attachment 286302 [details] pushed as c7c25c0 - Let startup() do its work instead of activate(), and reorder.