GNOME Bugzilla – Bug 736598
Create popovers on demand
Last modified: 2015-03-09 14:25:29 UTC
I opened the inspector today and was surprised to see a huge list of popover widgets. We seem to create a popover for each cell when starting the game. This is really excessive; can we create these on demand instead?
I’m thinking about not using a popover, but a “virtual keyboard” at the top of the right column instead. If it’s not done, I would prefer the way to have *only one* popover, detached and reattached. One or two in fact, depending on how earmarks are managed (that’s another story).
Created attachment 298808 [details] [review] Create popovers on demand I'm aware this can be improved. A thorough review is appreciated.
Review of attachment 298808 [details] [review]: What improvements were you thinking of? It looks mostly good to me. Although I did find one interesting regression: the Clear Board button no longer clears earmarks. You'll want to look into that. It would be nice to have NumberPickerPopover inherited from GtkPopover, instead of creating two NumberPickers and putting them into a GtkPopover. That would simplify things nicely. (But in a follow-up patch, not as part of this patch.)
(In reply to Michael Catanzaro from comment #3) > Review of attachment 298808 [details] [review] [review]: > > What improvements were you thinking of? It looks mostly good to me. I wasn't sure about the hide methods but I guess those are ok. How should I handle pressing enter or left clicking (and ctrl + enter and right clicking) the same cell multiple times? I see 3 cases here: 1) First click/press shows the popover, 2nd destroys the popover, 3rd opens and so on 2) First click shows the popover, 2nd destroys and creates a new popover (the destroy/show animation will play) 3) First click shows the popover, 2nd does nothing (the popover will stay there with no animation) > Although I did find one interesting regression: the Clear Board button no longer clears earmarks. You'll want to look into that. I will look into it. > It would be nice to have NumberPickerPopover inherited from GtkPopover, > instead of creating two NumberPickers and putting them into a GtkPopover. > That would simplify things nicely. (But in a follow-up patch, not as part of > this patch.) Good idea. I would take care of that after I'm done with this one.
(In reply to Iulian Radu from comment #4) > I wasn't sure about the hide methods but I guess those are ok. Yes, although ideally you wouldn't have to handle both widgets separately. Actually, I think you only need to use gtk_widget_destroy on the parent widget (the GtkPopover); the child widget (the NumberPicker) should get destroyed along with that. > How should I handle pressing enter or left clicking (and ctrl + enter and > right clicking) the same cell multiple times? I see 3 cases here: > 1) First click/press shows the popover, 2nd destroys the popover, 3rd opens > and so on > 2) First click shows the popover, 2nd destroys and creates a new popover > (the destroy/show animation will play) > 3) First click shows the popover, 2nd does nothing (the popover will stay > there with no animation) I think case (1): the additional click should dismiss the popover. Case (2) just seems silly. :)
Created attachment 298880 [details] [review] Create popovers on demand * Additional clicks dismisses the popover * Fix Clear Board button * Use only one method to handle both widgets' destruction * Play hide animation before destroy (I connected notify::visible for that) * Only destroy the popover (the NumberPicker will get destroyed with it)
Looks good to me! Attachment 298880 [details] pushed as 21c7182 - Create popovers on demand