GNOME Bugzilla – Bug 731276
Improvements for the popover
Last modified: 2014-07-15 17:16:46 UTC
Suggestions from Allan: * You have to click a square twice to make the popover appear. I think it should appear on single click - the second click isn't necessary. * The buttons in the popover need to be bigger, and the group of buttons needs to be centered inside the popover. I'll add one more: it should be easier to dismiss the earmark popover by clicking (either primary or secondary click) anywhere outside of it.
Created attachment 280185 [details] [review] Improvements for the popover and number picker - Popover will now appear on single click - The number-picker has been centered inside the popover - Size of buttons has been increased - http://i.imgur.com/BuozA6E.png (This also patches Bug 724825) Regarding the last suggestion (it should be easier to dismiss the earmark popover by clicking (either primary or secondary click) anywhere outside of it.), both popovers get dismissed on clicking (both primary and secondary clicks) on another cell, or a button in the game window, or anywhere outside the window. Let me know if the buttons still need to be bigger, I'll update the patch.
Review of attachment 280185 [details] [review]: Simple
> Regarding the last suggestion (it should be easier to dismiss the earmark > popover by > clicking (either primary or secondary click) anywhere outside of it.), both > popovers get dismissed on clicking (both primary and secondary clicks) on > another cell, or a button in the game window, or anywhere outside the window. I think this needs a bit more work. I'd like the popover to be dismissed if you click the currently-selected cell, or anywhere inside the game window that isn't a valid cell.
In our initial suggestion we had it that primary click closes the popover since you solved that cell - there is nothing more to do. Right/secondary interaction on a number would earmark without closing since you want to earmark some numbers. I guess every other interaction as well as a timeout should close the popover.
(In reply to comment #4) > I guess every other interaction as well as a > timeout should close the popover. It should not disappear after a timeout; there's no reason to do that.
> I think this needs a bit more work. I'd like the popover to be dismissed if > you click the currently-selected cell, or anywhere inside the game window that > isn't a valid cell. A Popover is either modal or non-modal. Modal - Pros - gets keyboard focus. So clicking anywhere outside dismisses the popover. Cons - gets keyboard focus. So once the popover is visible, pressing a number on your keyboard won't set the cell value to it. Buttons and other cells will need 2 clicks when the popover is visible (one will dismiss the popover, second will click the button) Non Modal - Pros - Doesn't get keyboard focus. So Undo/Redo, New Game etc. buttons work with one click, and cells can also be selected with a single click when the popover is visible. Pressing a number on keyboard works since the input focus is the cell and not the popover. Cons - Clicking anywhere inside the game window that's not a cell/button won't dismiss the popover I'd set the popover to Non Modal because the benefits were more, and I assumed that clicks on the board will be far more than clicks in empty space in the window. I can dismiss it on clicking second time the current cell again, with adding some logic. So, the question that remains is only the empty space to the right of the board. Let me know if you want the popover to be modal, I'll update the patch. Or we can make that space (i.e. the background) focus-able. But I'm not sure if we'd want that. Suggestions welcome :-)
Can we make it modal, but grab keyboard events so that number keys can be handled properly? Something like this: https://github.com/GNOME/gnome-calculator/blob/master/src/math-display.vala
Created attachment 280674 [details] [review] Improvements for the popover - Show popover on single click - make the number buttons bigger - Dismiss the popover on clicking same cell again Here's how the popover looks now - http://i.imgur.com/t2YDime.png
Review of attachment 280674 [details] [review]: Much better, good with this trivial fix: ::: src/number-picker.vala @@ +32,3 @@ label.use_markup = true; + label.margin = 4; + label.margin_left = 8; Please use margin_start and margin_end instead of margin_left and margin_right (deprecated properties that did not handle RTL properly, not that it makes any difference here).
This really turned out a lot better than I expected :)
Comment on attachment 280674 [details] [review] Improvements for the popover Pushed as d0d40d8 with following additions - - margin_left/right -> margin_start/end - Pressing Esc will close the popovers