GNOME Bugzilla – Bug 731086
Popover in fullscreen
Last modified: 2014-07-04 11:20:23 UTC
In gedit fullscreen mode an auto-hiding header bar is implemented. However, this doesn't play nice with the Gtk.Popover: * fullscreen gedit * click the settings button from the headerbar shown after moving the cursor to the top Expected: Popover remains visible until user clicks somewhere, maybe the headerbar should not hide either. What happens instead: The headerbar is hidden, and when the animation finishes, the popover also disappears, making the popover menu unusable.
Created attachment 279557 [details] [review] window: fix gear menu with fullscreen mode
We have a similar problem with the fullscreen recent menu but it appear to not working with this solution and as stated in https://bugzilla.gnome.org/show_bug.cgi?id=732287 the recent menu must probably be re-done, we are going to way for it.
Review of attachment 279557 [details] [review]: seen on irc with pbor so ok
Review of attachment 279557 [details] [review]: ::: gedit/gedit-window-private.h @@ +100,2 @@ gint bottom_panel_item_removed_handler_id; + gint fullscreen_eventbox_handler_id; The handler id is never used, so maybe you wanted to disconnect it in dispose() or something? If yes, g_signal_connect_object() can be used instead. @@ +101,3 @@ + gint fullscreen_eventbox_handler_id; + + gboolean fullscreen_eventbox_leave_state; It's better to have the booleans at the end of the struct with bit fields, like: guint blah : 1; It takes less memory. ::: gedit/gedit-window.c @@ +1797,3 @@ +real_fullscreen_controls_leave_notify_event (gpointer data) +{ + GeditWindow *window = (GeditWindow *)data; No need to cast data, it is a gpointer. Or you can do GEDIT_WINDOW (data) to have a type checking. @@ +1799,3 @@ + GeditWindow *window = (GeditWindow *)data; + + gboolean gear_menu_state = gtk_toggle_button_get_active ((GtkToggleButton *)window->priv->fullscreen_gear_button); Normally gedit uses the macros to cast, here GTK_TOGGLE_BUTTON(). @@ +1803,3 @@ + window->priv->fullscreen_eventbox_leave_state = TRUE; + + if (gear_menu_state == FALSE) For a boolean condition, no need to compare with TRUE or FALSE, write directly: if (!gear_menu_state). On the other hand for pointers it's clearer to compare with NULL (but lots of code doesn't do that unfortunaty). "if (!pointer)" is more difficult to understand than "if (pointer == NULL)". @@ +1808,3 @@ + } + + return FALSE; For an event return value it's clearer to use GDK_EVENT_PROPAGATE and GDK_EVENT_STOP. Same for a GSource, with G_SOURCE_CONTINUE and G_SOURCE_REMOVE. @@ +1819,3 @@ GeditWindow *window) { + g_timeout_add (5, real_fullscreen_controls_leave_notify_event, window); An idle doesn't work? 5 milliseconds doesn't seem really robust. @@ +2406,3 @@ + gboolean gear_menu_state = gtk_toggle_button_get_active (fullscreen_gear_button); + + if (!gear_menu_state && window->priv->fullscreen_eventbox_leave_state == TRUE) Same as comment above, no need for "== TRUE". @@ +2937,3 @@ gtk_menu_button_set_menu_model (window->priv->fullscreen_gear_button, gear_menu); + g_signal_connect ((GtkToggleButton *)window->priv->fullscreen_gear_button, The first parameter is a gpointer, so the cast is not needed, unless you want to explicitly say the signal comes from GtkToggleButton.
Created attachment 279619 [details] [review] fixes for the preceding patch
Created attachment 279621 [details] [review] fixes for the preceding patch
Review of attachment 279621 [details] [review]: A comment below, the rest looks good. ::: gedit/gedit-window.c @@ +1808,3 @@ } + return GDK_EVENT_PROPAGATE; The g_idle_add() below calls this callback function, so the return value should probably be G_SOURCE_REMOVE, no?
Created attachment 279625 [details] [review] fixes for the preceding patch
Comment on attachment 279625 [details] [review] fixes for the preceding patch pushed as c1e0466