GNOME Bugzilla – Bug 744023
Rework the UI
Last modified: 2016-02-14 07:29:32 UTC
Created attachment 296165 [details] [review] WIP patch. I’m not happy with Klotski’s UI, because: * when showing the left panel, the game is moved and resized; * you have a way to lost your current game without possible undo; * it looks bad; * it’s quite large when you show the left panel; * etc. I’ve tried some things, but I’m not for now happy with what I get. We cannot use a start-screen “as usual”, because we want to see the puzzle we are going to play… so the panel thing is probably the good way to go (or, could it be a popover?). Here’s a work-in-progress patch. Does that help someone having ideas about all that?
Review of attachment 296165 [details] [review]: Oh yes, much better. Are you not planning to complete this work? It looks nearly done to me. The only issue I see is that the Previous and Next buttons don't navigate to the next row in the tree view. This also fixes the "rows remain bold for too long" bug when using the old Previous/Next. ::: src/gnome-klotski.vala @@ +9,3 @@ */ +using Gtk; But you know I think this makes the code harder to read :( @@ +721,3 @@ + "comments", _("Sliding block puzzles"), + "copyright", + "Copyright © 1999–2008 Lars Rydlinge\nCopyright © 2014–2015 Michael Catanzaro", Since you're making such an extensive change, why not add yourself here!
Created attachment 296840 [details] [review] New UI. Use GtkBuilder. So. I continued to do researches on this, and the best UI I found uses a popover. This UI could probably be used in other games like Mahjongg. What do you think of it?
Review of attachment 296840 [details] [review]: OK, it almost works well. The Start Game button that just closes the popover is pretty useless, though, and it's not clear from the label View Puzzles that selecting another puzzle discards your current game without warning. So: * The Start Game button needs to go. I think it would work just fine without that button. * Come to think of it, do we really need the Next and Previous buttons on the bottom? It might actually confuse the user that they differ from the back and forward buttons on the top. (Don't change the buttons on top.) So maybe you can get rid of all the buttons on the bottom, does that sound good? * Rename "View Puzzles" to "Change Puzzles" or something like that, I think. * Is there code left to set the puzzle label bold when highlighted? That can probably be removed, I think. ::: src/Makefile.am @@ +25,3 @@ + --pkg librsvg-2.0 \ + --gresources klotski.gresource.xml \ + --target-glib=2.38 Don't hardcode this! In configure.ac, use AC_SUBST on GLIB_REQUIRED so you can use it here. ::: src/gnome-klotski.vala @@ +9,3 @@ */ +using Gtk; :( I guess I can't argue with you on this since you're the one doing the work. @@ +515,3 @@ + set_accels_for_action ("app.help", {"F1"}); + set_accels_for_action ("app.quit", {"<Primary>q"}); + set_accels_for_action ("win.restart-puzzle", {"<Primary>n"}); /* or <Primary>r ? or both ? */ Does this hotkey actually work? It's probably a good thing that it doesn't, without a warning dialog. @@ +663,3 @@ + "comments", _("Sliding block puzzles"), + "copyright", + "Copyright © 1999–2008 Lars Rydlinge\nCopyright © 2014–2015 Michael Catanzaro", Add yourself again :p
(In reply to Michael Catanzaro from comment #3) > * Rename "View Puzzles" to "Change Puzzles" or something like that, I think. "Change Puzzle"? Or "New Game" to make it clear that you can restart the current game? I am leaning towards the label "Change Puzzle" and putting "Start Over" in the app menu; how about that?
Created attachment 296919 [details] [review] New UI. Use GtkBuilder. > * The Start Game button needs to go. I think it would work just fine without that button. Ok, as discussed yesterday, I added a “Start Over” button at the left of the headerbar, and removed the blue button. That works great! > * Come to think of it, do we really need the Next and Previous buttons on the bottom? It might actually confuse the user that they differ from the back and forward buttons on the top. (Don't change the buttons on top.) So maybe you can get rid of all the buttons on the bottom, does that sound good? Not sure, I think it is less distracting to use a button than to click when looking for the next puzzle to play. I didn’t changed that. (Note: weird behaviour to correct before final release of popover closing itself when it shouldn’t.) > * Rename "View Puzzles" to "Change Puzzles" or something like that, I think. Done to “Change Puzzle”. > * Is there code left to set the puzzle label bold when highlighted? That can probably be removed, I think. I don’t know what you’re talking about. x) > ::: src/Makefile.am > @@ +25,3 @@ > + --pkg librsvg-2.0 \ > + --gresources klotski.gresource.xml \ > + --target-glib=2.38 > > Don't hardcode this! In configure.ac, use AC_SUBST on GLIB_REQUIRED so you can use it here. I don’t know how you want me to do that. But Iagno, Taquin and Mines (at least…) are using such a “--target-glib 2.XX” flag, so it may need some work to correct all. ^(^’ > + set_accels_for_action ("win.restart-puzzle", {"<Primary>n"}); /* or <Primary>r ? or both ? */ > > Does this hotkey actually work? It's probably a good thing that it doesn't, without a warning dialog. Hotkeys were quite all broken, I corrected that. This one is disabled, it needs an “undo” notification I think. > + "Copyright © 1999–2008 Lars Rydlinge\nCopyright © 2014–2015 Michael Catanzaro", > > Add yourself again :p Ok, done there. ^(^’
Review of attachment 296919 [details] [review]: Very well written! Just took a quick look. All I got are these nitpicks :P ::: data/klotski.ui @@ +103,3 @@ + <property name="headers-visible">False</property> + <property name="activate-on-single-click">True</property> + <property name="can-focus">False</property><!-- for up/down keybinding --> nit: whitespace erty>_<!-- for up/ @@ +147,3 @@ + <property name="headers-visible">False</property> + <property name="activate-on-single-click">True</property> + <property name="can-focus">False</property><!-- for up/down keybinding --> nit: whitespace @@ +191,3 @@ + <property name="headers-visible">False</property> + <property name="activate-on-single-click">True</property> + <property name="can-focus">False</property><!-- for up/down keybinding --> nit: whitespace @@ +268,3 @@ + <property name="width-request">600</property> + <property name="height-request">400</property> + <property name="border-width">25</property><!-- TODO a view margin --> nit: whitespace ::: src/score-dialog.vala @@ +28,3 @@ public ScoreDialog (History history, HistoryEntry? selected_entry = null, bool show_close = false) { + // Object (use_header_bar: Gtk.Settings.get_default ().gtk_dialogs_use_header ? 1 : 0); Do you want to get rid of this line? @@ +65,3 @@ + { + TreeIter iter; + Do you want to get rid of lines 61-67?
Created attachment 296944 [details] [review] New UI. Use GtkBuilder. This version uses GLIB_REQUIRED for the argument to --target-glib. We should correct it in the other games eventually.
Created attachment 296973 [details] [review] New UI. Use GtkBuilder. > ::: src/score-dialog.vala > @@ +28,3 @@ > public ScoreDialog (History history, HistoryEntry? selected_entry = null, bool show_close = false) > { > + // Object (use_header_bar: Gtk.Settings.get_default ().gtk_dialogs_use_header ? 1 : 0); > > Do you want to get rid of this line? Needed discussion, it was a hidden TODO. :þ I removed the buttons-madness of the ScoreDialog, so this line came back (a little modified, because I try to support non-dialog-header configs with it). > @@ +65,3 @@ > + { > + TreeIter iter; > + > > Do you want to get rid of lines 61-67? I probably should, but do not understand for now why it was there initially. So I’m going to keep it commented some more days. > This version uses GLIB_REQUIRED for the argument to --target-glib. We should correct it in the other games eventually. Right, I’ve corrected some other games.
Attachment 296973 [details] pushed as fdf0b06 - New UI. Use GtkBuilder.
I've updated this CSS for GTK+ 3.19 in git, please sanity-check that I haven't broken your work. :)
Looks good, thanks. :·) I pushed one more little CSS rule to go around bug 744531. I’ll try to do a big testing of games after the .90 release, it cannot hurt. ^(^
By the way, shouldn’t we bump GTK+ required version..?