GNOME Bugzilla – Bug 735806
[PATCHes] Clean gnome-sudoku.vala file.
Last modified: 2014-09-05 13:56:20 UTC
Created attachment 285000 [details] [review] Use GResource for loading appmenu. Here are five cleaning patches: * the first one uses the GResource organisation to automatically load the app’menu, and cleans the GtkBuilders creation, not using try {} but Builder.from_resource(); * the second uses set_accels_for_action() as add_accelerator() is deprecated; * the third makes the creation of the window in startup() instead of in activate(), so that re-launching the app’ from command-line doesn’t finish with two buggy windows; * the next one cleans gnome-sudoku.vala namespaces; * the last cleans the grid saving when quitting application (what works when using the close button of the window didn’t work with the quit app’menu entry).
Created attachment 285001 [details] [review] Use set_accels_for_action().
Created attachment 285002 [details] [review] Reorder calls to ensure window uniqueness.
Created attachment 285003 [details] [review] Remove unused namespaces.
Created attachment 285004 [details] [review] Clean the grid saving when quitting application.
Review of attachment 285000 [details] [review]: Can you please rebase this whole patch series? I can't apply any except this first one. Looks good, but I bet we need to bump our GTK+ requirement in configure.ac for this (which is best done in a separate patch, so that it's more likely to appear in the changelogs). If you check the news file https://git.gnome.org/browse/gtk+/tree/NEWS it looks like this was added in 3.13.4.
Review of attachment 285001 [details] [review]: Looks good...
Review of attachment 285002 [details] [review]: Why did you move the intltool initialization? startup() should be called first, and main() should do nothing except call run().
Review of attachment 285003 [details] [review]: Looks good.
Review of attachment 285004 [details] [review]: This looks important. ::: src/gnome-sudoku.vala @@ +47,3 @@ {"help", help_cb }, {"about", about_cb }, + {"quit", quit } How does this work? I couldn't find quit in any of the parent classes?
Created attachment 285432 [details] [review] Bump Gtk+ required version to 3.13.4.
Created attachment 285433 [details] [review] Use GResource for loading appmenu.
Created attachment 285434 [details] [review] Use set_accels_for_action().
Created attachment 285435 [details] [review] Reorder calls to ensure window uniqueness. (In reply to comment #7) > Why did you move the intltool initialization? startup() should be called > first, and main() should do nothing except call run(). The intltool initialization should be done before the call of handle_local_options(). So either at the beginning at Sudoku() like in this patch, or in main() like in the previous.
Created attachment 285436 [details] [review] Remove unused namespaces.
Created attachment 285437 [details] [review] Clean the grid saving when quitting application. (In reply to comment #9) > This looks important. Yes, it is. =) > ::: src/gnome-sudoku.vala > @@ +47,3 @@ > {"help", help_cb }, > {"about", about_cb }, > + {"quit", quit } > > How does this work? I couldn't find quit in any of the parent classes? It’s the GApplication function to quit. No complete magic here. ^^
OK, thanks! Attachment 285432 [details] pushed as cc977f6 - Bump Gtk+ required version to 3.13.4. Attachment 285433 [details] pushed as f946bd6 - Use GResource for loading appmenu. Attachment 285434 [details] pushed as 9347b89 - Use set_accels_for_action(). Attachment 285435 [details] pushed as 01fca22 - Reorder calls to ensure window uniqueness. Attachment 285436 [details] pushed as b9c4bd6 - Remove unused namespaces. Attachment 285437 [details] pushed as 847d9e2 - Clean the grid saving when quitting application.