Bug 520749 - F11 make gedit go fullscreen
F11 make gedit go fullscreen
Status: RESOLVED FIXED
Product: gedit
Classification: Applications
Component: general
2.21.x
Other All
: Normal enhancement
: ---
Assigned To: Gedit maintainers
Gedit maintainers
:
: 396284 527503 (view as bug list)
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2008-03-06 13:42 UTC by Moo
Modified: 2009-01-07 15:10 UTC (History)
6 users (show)

See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Patch for F11 fullscreen in gedit (2.84 KB, patch)
2008-06-08 12:57 UTC, Vincenzo Scotti
none Details | Diff | Review
Patch for F11 fullscreen in gedit (2) (2.84 KB, patch)
2008-06-08 13:22 UTC, Vincenzo Scotti
none Details | Diff | Review
Improvement of previous patch (3.78 KB, patch)
2008-09-22 12:56 UTC, Vincenzo Scotti
none Details | Diff | Review
Improved shortcuts (3.78 KB, patch)
2008-10-21 18:03 UTC, Vincenzo Scotti
needs-work Details | Diff | Review
Improved shortcuts (3.48 KB, patch)
2008-10-26 09:32 UTC, Vincenzo Scotti
none Details | Diff | Review
First patch for fullscreen mode with popup button. (10.05 KB, patch)
2008-11-19 22:13 UTC, Giuseppe Fuggiano
needs-work Details | Diff | Review
fullscreen (18.06 KB, patch)
2009-01-07 12:44 UTC, Ignacio Casal Quinteiro (nacho)
none Details | Diff | Review

Description Moo 2008-03-06 13:42:34 UTC
I noticed that many GNOME software such as Eye of GNOME can go fullscreen by pressing F11.
Firefox and many other software do this too.

So I thought maybe gedit should do this?

Maybe its a good idea or a dumb idea.
Comment 1 Vincenzo Scotti 2008-06-08 12:57:59 UTC
Created attachment 112362 [details] [review]
Patch for F11 fullscreen in gedit

I have attached a patch for this feauters
Comment 2 Vincenzo Scotti 2008-06-08 12:58:45 UTC
Comment on attachment 112362 [details] [review]
Patch for F11 fullscreen in gedit

I have attached a patch for this featuer
Comment 3 Vincenzo Scotti 2008-06-08 13:18:43 UTC
I am sorry, the previous patch is wrong... here is the correct one to implement that feature
http://rafb.net/p/X9yyVE81.html
Comment 4 Vincenzo Scotti 2008-06-08 13:22:37 UTC
Created attachment 112364 [details] [review]
Patch for F11 fullscreen in gedit (2)

Here is the correct patch.
Comment 5 Vincenzo Scotti 2008-09-22 12:56:48 UTC
Created attachment 119160 [details] [review]
Improvement of previous patch

I've improved the patch, but when will i know if the patch is approved?
Comment 6 Vincenzo Scotti 2008-10-21 18:03:55 UTC
Created attachment 121045 [details] [review]
Improved shortcuts
Comment 7 Paolo Borelli 2008-10-25 09:33:50 UTC
The patch looks reasonable to me and I am in favour of having this in the core (even if a plugin could easily add this feature).

Some comments on the patch:
 - I don't think it's worth adding shortcuts to show menubar, statusbar etc
 - the shortuct for the button pane should not be changed
 - I'd love to see a special-case toolbar similar to totem and other apps that drops down automatically when moving the mouse at the top of the screen and that includes a "Leave Fullscreen" button at the top right. I think some people (Matthias Hasselman?) were even proposing adding such a widget to gtk, so there should be code available for cut&pasting this functionality
Comment 8 Vincenzo Scotti 2008-10-26 09:32:02 UTC
Created attachment 121373 [details] [review]
Improved shortcuts

I've modified the shortcut for the bottom pane.

I've added a shortcut to each panel because if someone wants any panel in fullscreen mode, he can't.
I think that the Leave Fullscreen button isn't useful, because there's the F11 shortcut that do this work.
Comment 9 Paolo Borelli 2008-10-26 12:01:29 UTC
The leave-fullscreen is needed becasue if a user goes in fullscreen mode by mistake (e.g. accidentally pressing F11) he has no way to figure out how to return to the normal mode, especially since the menubar is hidden.
Comment 10 Giuseppe Fuggiano 2008-11-19 22:13:50 UTC
Created attachment 123081 [details] [review]
First patch for fullscreen mode with popup button.

This patch cannot be committed.

The reason is that actually it has a little bug: switching between workspaces the popped-up button still remains visible.  I'd like to have an hint for that.  Thank you.
Comment 11 Paolo Borelli 2008-11-23 10:49:27 UTC
Comment on attachment 123081 [details] [review]
First patch for fullscreen mode with popup button.

Ok, below some inline comments to the patch.

About the buttom popup when switching workspace, you probably need to connect to some notification: for instance taking a look at totem code I see they connect to "notify::is-active"



>+++ b/gedit/gedit-commands-view.c	2008-11-19 23:04:28.000000000 +0100
>@@ -121,3 +121,15 @@ _gedit_cmd_view_show_bottom_pane (GtkAct
> 		gtk_widget_hide (GTK_WIDGET (panel));
> 	}
> }
>+
>+void
>+_gedit_cmd_view_fullscreen_mode (GtkAction *action,
>+				 GeditWindow *window)
>+{
>+	gedit_debug (DEBUG_COMMANDS);
>+
>+	if (gedit_window_is_fullscreen (window))
>+		gedit_window_unfullscreen (window);
>+	else
>+		gedit_window_fullscreen (window);
>+}

I don't think that checking gedit_window_is_fullscreen before calling unfullscreen is needed: unfullscreen should simply return if the window is already not fullscreen.


>diff -Naurp a/gedit/gedit-ui.h b/gedit/gedit-ui.h
>--- a/gedit/gedit-ui.h	2008-11-19 00:58:48.000000000 +0100
>+++ b/gedit/gedit-ui.h	2008-11-19 23:04:28.000000000 +0100
>@@ -61,6 +61,11 @@ static const GtkActionEntry gedit_always
> 	  N_("Open a file"), G_CALLBACK (_gedit_cmd_file_open) },
> 	{ "FileOpenURI", NULL, N_("Open _Location..."), "<control>L",
> 	  N_("Open a file from a specified location"), G_CALLBACK (_gedit_cmd_file_open_uri) },
>+
>+	/* View menu */
>+	{ "ViewFullscreenMode", GTK_STOCK_FULLSCREEN, NULL, "F11",
>+	  N_("Switch in fullscreen mode"),
>+          G_CALLBACK (_gedit_cmd_view_fullscreen_mode) },
> 	


I took a look around and I see that some apps (totem) put fullscreen at the top of the view menu, others (evince, terminal) put it below the controls to show toolbar etc. What does the HIG specify?
I'd put it after show toolbar etc.


> 	/* Edit menu */
> 	{ "EditPreferences", GTK_STOCK_PREFERENCES, N_("Pr_eferences"), NULL,
>@@ -148,10 +153,10 @@ static const GtkActionEntry gedit_quit_m
> 
> static const GtkToggleActionEntry gedit_always_sensitive_toggle_menu_entries[] =
> {
>-	{ "ViewToolbar", NULL, N_("_Toolbar"), NULL,
>+	{ "ViewToolbar", NULL, N_("_Toolbar"), "<shift>F9",
> 	  N_("Show or hide the toolbar in the current window"),
> 	  G_CALLBACK (_gedit_cmd_view_show_toolbar), TRUE },
>-	{ "ViewStatusbar", NULL, N_("_Statusbar"), NULL,
>+	{ "ViewStatusbar", NULL, N_("_Statusbar"), "<shift><control>F9",
> 	  N_("Show or hide the statusbar in the current window"),
> 	  G_CALLBACK (_gedit_cmd_view_show_statusbar), TRUE },
> 	{ "ViewSidePane", NULL, N_("Side _Pane"), "F9",

Drop these changes. I don't think showing statusbar and toolbar need accelerators and no other app have them

>diff -Naurp a/gedit/gedit-ui.xml b/gedit/gedit-ui.xml
>--- a/gedit/gedit-ui.xml	2008-11-19 00:58:48.000000000 +0100
>+++ b/gedit/gedit-ui.xml	2008-11-19 23:04:28.000000000 +0100
>@@ -86,6 +86,8 @@
>         <placeholder name="LanguagesMenuPlaceholder">
>         </placeholder>
>       </menu>
>+      <separator/>
>+      <menuitem name="ViewFullscreenMenu" action="ViewFullscreenMode"/>
>     </menu>
> 

Ops. I guess the comment above about the menu placement does not apply completely afterall: you put it at the end...
Let's still check which is the proper place to put it and make sure to have the actions in the same order of ui.xml.

>     <menu name="SearchMenu" action="Search">
>diff -Naurp a/gedit/gedit-window.c b/gedit/gedit-window.c
>--- a/gedit/gedit-window.c	2008-11-19 00:58:48.000000000 +0100
>+++ b/gedit/gedit-window.c	2008-11-19 23:04:28.000000000 +0100
>@@ -57,6 +57,8 @@
> 
> #define LANGUAGE_NONE (const gchar *)"LangNone"
> 
>+#define FULLSCREEN_POPUP_TIMEOUT 5
>+
> #define GEDIT_WINDOW_GET_PRIVATE(object)(G_TYPE_INSTANCE_GET_PRIVATE ((object),\
> 					 GEDIT_TYPE_WINDOW,                    \
> 					 GeditWindowPrivate))
>@@ -2346,6 +2348,12 @@ drop_uris_cb (GtkWidget    *widget,
> }
> 
> static void
>+leave_fullscreen_button_clicked_cb (GeditWindow *window)
>+{
>+	gedit_window_unfullscreen (window);
>+}
>+
>+static void
> can_search_again (GeditDocument *doc,
> 		  GParamSpec    *pspec,
> 		  GeditWindow   *window)
>@@ -3072,6 +3080,7 @@ gedit_window_init (GeditWindow *window)
> 	window->priv->removing_tabs = FALSE;
> 	window->priv->state = GEDIT_WINDOW_STATE_NORMAL;
> 	window->priv->destroy_has_run = FALSE;
>+	window->priv->timeout_id = 0;
> 

let's use a more descriptive name here, e.g. fullscreen_button_timeout_id


> 	window->priv->window_group = gtk_window_group_new ();
> 	gtk_window_group_add_window (window->priv->window_group, GTK_WINDOW (window));
>@@ -3464,6 +3473,50 @@ gedit_window_set_active_tab (GeditWindow
> 				       page_num);
> }
> 
>+void
>+gedit_window_fullscreen (GeditWindow *window)
>+{
>+	g_return_if_fail (GEDIT_IS_WINDOW (window));
>+
>+	if (gedit_window_is_fullscreen (window))
>+		return;
>+
>+	/* Go to fullscreen mode and hide bars */
>+	gtk_window_fullscreen (&window->window);
>+	gtk_notebook_set_show_tabs (GTK_NOTEBOOK (window->priv->notebook), FALSE);
>+	gtk_widget_hide (gtk_ui_manager_get_widget (window->priv->manager, "/MenuBar"));
>+	gtk_widget_hide (window->priv->toolbar);
>+	gtk_widget_hide (window->priv->statusbar);
>+
>+	_gedit_window_fs_popup_build (window);
>+}
>+
>+void
>+gedit_window_unfullscreen (GeditWindow *window)
>+{
>+	g_return_if_fail (GEDIT_IS_WINDOW (window));
>+
>+	if (!gedit_window_is_fullscreen (window))
>+		return;
>+
>+	/* Unfullscreen and show bars */
>+	gtk_window_unfullscreen (&window->window);
>+	gtk_notebook_set_show_tabs (GTK_NOTEBOOK (window->priv->notebook), TRUE);
>+	gtk_widget_show (gtk_ui_manager_get_widget (window->priv->manager, "/MenuBar"));
>+	gtk_widget_show (window->priv->toolbar);
>+	gtk_widget_show (window->priv->statusbar);
>+

Careful here: statusbar etc should not be shown unconditionally, but according to the prefs.

>+	_gedit_window_fs_popup_destroy (window);
>+}
>+
>+gboolean
>+gedit_window_is_fullscreen (GeditWindow *window)
>+{
>+	g_return_val_if_fail (GEDIT_IS_WINDOW (window), FALSE);
>+
>+	return window->priv->window_state & GDK_WINDOW_STATE_FULLSCREEN;
>+}
>+

I do not think this function needs to be public.

> GtkWindowGroup *
> gedit_window_get_group (GeditWindow *window)
> {
>@@ -3616,6 +3669,92 @@ _gedit_window_set_saving_session_state (
> 	}
> }
> 
>+void
>+_gedit_window_fs_popup_show (GeditWindow *window)
>+{

This function should be static, not exported.

Also, let's not abbreviate fullscreen to fs

>+	GdkScreen *screen;
>+	GdkRectangle rect;
>+	gint fs_window_width;
>+	gint fs_window_height;
>+	guint monitor;
>+
>+	screen = gtk_window_get_screen (&window->window);
>+	monitor = gdk_screen_get_monitor_at_window (screen, (GTK_WIDGET (&window->window))->window);
>+	gdk_screen_get_monitor_geometry (screen, monitor, &rect);
>+
>+	gtk_window_get_size (GTK_WINDOW (window->priv->fs_window),
>+			     &fs_window_width, &fs_window_height);
>+
>+	if (gtk_widget_get_direction (window->priv->fs_window) == GTK_TEXT_DIR_LTR)
>+	{
>+		gtk_window_move (GTK_WINDOW (window->priv->fs_window),
>+				 rect.x + rect.width - fs_window_width,
>+				 rect.y);
>+	}
>+	else
>+	{
>+		gtk_window_move (GTK_WINDOW (window->priv->fs_window),
>+				 rect.x, rect.y);
>+	}
>+
>+	gtk_widget_show_all (window->priv->fs_window);
>+	
>+	if (window->priv->timeout_id != 0)
>+		g_source_remove (window->priv->timeout_id);
>+
>+	window->priv->timeout_id = g_timeout_add_seconds (FULLSCREEN_POPUP_TIMEOUT,
>+							  (GSourceFunc) _gedit_window_fs_popup_hide,
>+							  window);
>+}
>+
>+gboolean
>+_gedit_window_fs_popup_hide (GeditWindow *window)

This function should be static, not exported.

>+{
>+	if (window->priv->timeout_id != 0)
>+	{
>+		gtk_widget_hide (window->priv->fs_window);
>+

You also need to remove the timeout here

>+		window->priv->timeout_id = 0;
>+	}
>+
>+	return FALSE;
>+}
>+
>+void
>+_gedit_window_fs_popup_build (GeditWindow *window)
>+{
>+	window->priv->fs_window = gtk_window_new (GTK_WINDOW_POPUP);
>+	window->priv->fs_button = gtk_button_new_from_stock (GTK_STOCK_LEAVE_FULLSCREEN);
>+
>+	gtk_window_set_transient_for (GTK_WINDOW (window->priv->fs_window), &window->window);
>+	gtk_window_set_decorated (GTK_WINDOW (window->priv->fs_window), FALSE);
>+	gtk_container_add (GTK_CONTAINER (window->priv->fs_window), window->priv->fs_button);
>+
>+	window->priv->motion_handler_id = g_signal_connect (G_OBJECT (&window->window), "motion-notify-event",

I am not sure I want to show the button each time the mouse is moved... I'd prefer to show it only when mouse is moved out the top of the screen. Opinions?

The cast to G_OBJECT is not needed when using g_signal_connect.


>+							    G_CALLBACK (_gedit_window_fs_popup_show), window);
>+
>+	g_signal_connect_swapped (G_OBJECT (window->priv->fs_button), "clicked",
>+				  G_CALLBACK (leave_fullscreen_button_clicked_cb), window);
>+

Do not use connect_swapped, just make leave_fullscreen_button_clicked_cb have the correct signature.
Style nitpick: I'd move the leave_fullscreen_button_clicked_cb function near to where it is used.

>+	gtk_widget_realize (window->priv->fs_window);
>+}
>+
>+void
>+_gedit_window_fs_popup_destroy (GeditWindow *window)
>+{
>+	if (window->priv->timeout_id != 0)
>+	{
>+		g_source_remove (window->priv->timeout_id);
>+
>+		window->priv->timeout_id = 0;
>+	}
>+
>+	g_signal_handler_disconnect (G_OBJECT (&window->window), 
>+				     window->priv->motion_handler_id);
>+
>+	gtk_widget_destroy (window->priv->fs_window);
>+}
>+
> /**
>  * gedit_window_get_tab_from_location:
>  * @window: a #GeditWindow
>diff -Naurp a/gedit/gedit-window.h b/gedit/gedit-window.h
>--- a/gedit/gedit-window.h	2008-11-19 00:58:48.000000000 +0100
>+++ b/gedit/gedit-window.h	2008-11-19 23:04:28.000000000 +0100
>@@ -124,6 +124,12 @@ GeditTab	*gedit_window_get_active_tab		(
> void		 gedit_window_set_active_tab		(GeditWindow         *window,
> 							 GeditTab            *tab);
> 
>+void		 gedit_window_fullscreen		(GeditWindow         *window);
>+
>+void		 gedit_window_unfullscreen		(GeditWindow         *window);
>+
>+gboolean	 gedit_window_is_fullscreen		(GeditWindow         *window);
>+
> /* Helper functions */
> GeditView	*gedit_window_get_active_view		(GeditWindow         *window);
> GeditDocument	*gedit_window_get_active_document	(GeditWindow         *window);
>@@ -155,6 +161,7 @@ GeditTab        *gedit_window_get_tab_fr
> 
> GeditTab        *gedit_window_get_tab_from_uri		(GeditWindow         *window,
> 							 const gchar         *uri);
>+
> /*
>  * Non exported functions
>  */
>@@ -173,6 +180,14 @@ void		 _gedit_window_set_default_path 	(
> void		 _gedit_window_set_saving_session_state	(GeditWindow         *window,
> 							 gboolean             saving_session);
> 
>+void		 _gedit_window_fs_popup_build		(GeditWindow         *window);
>+
>+void		 _gedit_window_fs_popup_show		(GeditWindow         *window);
>+
>+gboolean	 _gedit_window_fs_popup_hide		(GeditWindow         *window);
>+
>+void		 _gedit_window_fs_popup_destroy		(GeditWindow         *window);
>+
> /* these are in gedit-window because of screen safety */
> void		 _gedit_recent_add			(GeditWindow	     *window,
> 							 const gchar         *uri,
>diff -Naurp a/gedit/gedit-window-private.h b/gedit/gedit-window-private.h
>--- a/gedit/gedit-window-private.h	2008-11-19 00:58:48.000000000 +0100
>+++ b/gedit/gedit-window-private.h	2008-11-19 23:04:28.000000000 +0100
>@@ -48,6 +48,12 @@ struct _GeditWindowPrivate
> 	GtkWidget      *hpaned;
> 	GtkWidget      *vpaned;	
> 
>+	/* Widgets and utils for fullscreen mode */
>+	GtkWidget      *fs_button;
>+	GtkWidget      *fs_window;
>+	guint           timeout_id;
>+	gulong          motion_handler_id;
>+
> 	/* statusbar and context ids for statusbar messages */
> 	GtkWidget      *statusbar;	
> 	guint           generic_message_cid;
Comment 12 Giuseppe Fuggiano 2008-11-23 13:16:36 UTC
Hi Paolo, thanks for commenting my patch.

(In reply to comment #11)
> About the buttom popup when switching workspace, you probably need to connect
> to some notification: for instance taking a look at totem code I see they
> connect to "notify::is-active"

Yes, I recently discovered that popup windows are not managed by the window manager, so I think I will do the same as totem.


> >+++ b/gedit/gedit-commands-view.c	2008-11-19 23:04:28.000000000 +0100
> >@@ -121,3 +121,15 @@ _gedit_cmd_view_show_bottom_pane (GtkAct
> > 		gtk_widget_hide (GTK_WIDGET (panel));
> > 	}
> > }
> >+
> >+void
> >+_gedit_cmd_view_fullscreen_mode (GtkAction *action,
> >+				 GeditWindow *window)
> >+{
> >+	gedit_debug (DEBUG_COMMANDS);
> >+
> >+	if (gedit_window_is_fullscreen (window))
> >+		gedit_window_unfullscreen (window);
> >+	else
> >+		gedit_window_fullscreen (window);
> >+}
> 
> I don't think that checking gedit_window_is_fullscreen before calling
> unfullscreen is needed: unfullscreen should simply return if the window is
> already not fullscreen.

_gedit_cmd_view_fullscreen_mode() is called when the user press F11 or click to the corresponding action in the menu.  This means that gedit_window_fullscreen() should return if the window is already fullscreen too, and we would have the following weird code, in my opinion:

void
_gedit_cmd_view_fullscreen_mode (GtkAction *action,
				 GeditWindow *window)
{
	gedit_debug (DEBUG_COMMANDS);

	gedit_window_fullscreen (window);    /* return if already fullscreen */
	gedit_window_unfullscreen (window);  /* return if already not fullscreen */
}

which is not very readable.


[cut] 
> >diff -Naurp a/gedit/gedit-ui.xml b/gedit/gedit-ui.xml
> >--- a/gedit/gedit-ui.xml	2008-11-19 00:58:48.000000000 +0100
> >+++ b/gedit/gedit-ui.xml	2008-11-19 23:04:28.000000000 +0100
> >@@ -86,6 +86,8 @@
> >         <placeholder name="LanguagesMenuPlaceholder">
> >         </placeholder>
> >       </menu>
> >+      <separator/>
> >+      <menuitem name="ViewFullscreenMenu" action="ViewFullscreenMode"/>
> >     </menu>
> > 
> 
> Ops. I guess the comment above about the menu placement does not apply
> completely afterall: you put it at the end...
> Let's still check which is the proper place to put it and make sure to have the
> actions in the same order of ui.xml.

Yes, the difference here is whether they should be active or inactive if no document is opened.  Here I choosen that fullscreen action should still be active, but this should be different though.  If no document is opened or if the user is in fullscreen mode and close all documents, the window should go unfullscreen automatically. What about you?


> >     <menu name="SearchMenu" action="Search">
> >diff -Naurp a/gedit/gedit-window.c b/gedit/gedit-window.c
> >--- a/gedit/gedit-window.c	2008-11-19 00:58:48.000000000 +0100
> >+++ b/gedit/gedit-window.c	2008-11-19 23:04:28.000000000 +0100
> >@@ -57,6 +57,8 @@
> > 
> > #define LANGUAGE_NONE (const gchar *)"LangNone"
> > 
> >+#define FULLSCREEN_POPUP_TIMEOUT 5
> >+
> > #define GEDIT_WINDOW_GET_PRIVATE(object)(G_TYPE_INSTANCE_GET_PRIVATE ((object),\
> > 					 GEDIT_TYPE_WINDOW,                    \
> > 					 GeditWindowPrivate))
> >@@ -2346,6 +2348,12 @@ drop_uris_cb (GtkWidget    *widget,
> > }
> > 
> > static void
> >+leave_fullscreen_button_clicked_cb (GeditWindow *window)
> >+{
> >+	gedit_window_unfullscreen (window);
> >+}
> >+
> >+static void
> > can_search_again (GeditDocument *doc,
> > 		  GParamSpec    *pspec,
> > 		  GeditWindow   *window)
> >@@ -3072,6 +3080,7 @@ gedit_window_init (GeditWindow *window)
> > 	window->priv->removing_tabs = FALSE;
> > 	window->priv->state = GEDIT_WINDOW_STATE_NORMAL;
> > 	window->priv->destroy_has_run = FALSE;
> >+	window->priv->timeout_id = 0;
> > 
> 
> let's use a more descriptive name here, e.g. fullscreen_button_timeout_id

In general, I didn't want to pollute too much the code with very-long-names (caused by "fullscreen" and "unfullscreen" tokens I think), but if this should help in readness, I obviously agree :-)


[cut]
> 
> >+	_gedit_window_fs_popup_destroy (window);
> >+}
> >+
> >+gboolean
> >+gedit_window_is_fullscreen (GeditWindow *window)
> >+{
> >+	g_return_val_if_fail (GEDIT_IS_WINDOW (window), FALSE);
> >+
> >+	return window->priv->window_state & GDK_WINDOW_STATE_FULLSCREEN;
> >+}
> >+
> 
> I do not think this function needs to be public.

Why?  If an external object could change the fullscreen state of GeditWindow (gedit_window_fullscreen and unfullscreen are public), isn't a good idea to provide also what's the current state information?


[cut]
> >+void
> >+_gedit_window_fs_popup_build (GeditWindow *window)
> >+{
> >+	window->priv->fs_window = gtk_window_new (GTK_WINDOW_POPUP);
> >+	window->priv->fs_button = gtk_button_new_from_stock (GTK_STOCK_LEAVE_FULLSCREEN);
> >+
> >+	gtk_window_set_transient_for (GTK_WINDOW (window->priv->fs_window), &window->window);
> >+	gtk_window_set_decorated (GTK_WINDOW (window->priv->fs_window), FALSE);
> >+	gtk_container_add (GTK_CONTAINER (window->priv->fs_window), window->priv->fs_button);
> >+
> >+	window->priv->motion_handler_id = g_signal_connect (G_OBJECT (&window->window), "motion-notify-event",
> 
> I am not sure I want to show the button each time the mouse is moved... I'd
> prefer to show it only when mouse is moved out the top of the screen. Opinions?

Yes, the GNOME HIG recite:

"Pressing ESC should cause the application to leave full screen mode. A Leave Fullscreen button should be placed in the upper right hand corner of the window. The button should disappear after the mouse is unused for 5 seconds, and should appear again when the moused is moved. Alternately, in applications where the mouse is used frequently in full screen mode, all but a two pixel row of the button may be slid off the top of the screen. The button should slide back on the screen when the mouse moves near it."

Gedit users don't use the mouse frequently, though, since Gedit is a text editor and to type text they should use the keyboard.  In addiction, the button don't pollute too much the visual. So, the HIG approach is to show the button every time the mouse is moved for 5 seconds.

[cut]

The cutted quoted text refers to comments where I totally agree and will be fixed.  But what about the rest?
Comment 13 Paolo Borelli 2008-11-23 15:29:22 UTC
> >+void
> > >+_gedit_cmd_view_fullscreen_mode (GtkAction *action,
> > >+				 GeditWindow *window)
> > >+{
> > >+	gedit_debug (DEBUG_COMMANDS);
> > >+
> > >+	if (gedit_window_is_fullscreen (window))
> > >+		gedit_window_unfullscreen (window);
> > >+	else
> > >+		gedit_window_fullscreen (window);
> > >+}
> > 
> > I don't think that checking gedit_window_is_fullscreen before calling
> > unfullscreen is needed: unfullscreen should simply return if the window is
> > already not fullscreen.
> 
> _gedit_cmd_view_fullscreen_mode() is called when the user press F11 or click to
> the corresponding action in the menu.  This means that
> gedit_window_fullscreen() should return if the window is already fullscreen
> too, and we would have the following weird code, in my opinion:

mmm... you are right I guess. Let's call the function toggle_fullscreen_mode then.

> 
> [cut] 
> > >diff -Naurp a/gedit/gedit-ui.xml b/gedit/gedit-ui.xml
> > >--- a/gedit/gedit-ui.xml	2008-11-19 00:58:48.000000000 +0100
> > >+++ b/gedit/gedit-ui.xml	2008-11-19 23:04:28.000000000 +0100
> > >@@ -86,6 +86,8 @@
> > >         <placeholder name="LanguagesMenuPlaceholder">
> > >         </placeholder>
> > >       </menu>
> > >+      <separator/>
> > >+      <menuitem name="ViewFullscreenMenu" action="ViewFullscreenMode"/>
> > >     </menu>
> > > 
> > 
> > Ops. I guess the comment above about the menu placement does not apply
> > completely afterall: you put it at the end...
> > Let's still check which is the proper place to put it and make sure to have the
> > actions in the same order of ui.xml.
> 
> Yes, the difference here is whether they should be active or inactive if no
> document is opened.  Here I choosen that fullscreen action should still be
> active, but this should be different though.  If no document is opened or if
> the user is in fullscreen mode and close all documents, the window should go
> unfullscreen automatically. What about you?
> 
> 

I think the menu should be like this:
1) sensitivity: disabled when there is no doc open, closing the last doc should exit fullscreen mode
2) order: it should go after view toolbar/statusbar/etc and a separator
3) make sure the GtkActions and the menu items in the .ui file are ordered in the same way for code cleanliness

> > > 	window->priv->destroy_has_run = FALSE;
> > >+	window->priv->timeout_id = 0;
> > > 
> > 
> > let's use a more descriptive name here, e.g. fullscreen_button_timeout_id
> 

Yeah, I think here the longer but descriptive name is better.

> 
> [cut]
> > 
> > >+	_gedit_window_fs_popup_destroy (window);
> > >+}
> > >+
> > >+gboolean
> > >+gedit_window_is_fullscreen (GeditWindow *window)
> > >+{
> > >+	g_return_val_if_fail (GEDIT_IS_WINDOW (window), FALSE);
> > >+
> > >+	return window->priv->window_state & GDK_WINDOW_STATE_FULLSCREEN;
> > >+}
> > >+
> > 
> > I do not think this function needs to be public.
> 
> Why?  If an external object could change the fullscreen state of GeditWindow
> (gedit_window_fullscreen and unfullscreen are public), isn't a good idea to
> provide also what's the current state information?
> 

Well, the reason was that this is not at all specific to a gedit window: it could be a method of GtkWindow and I wonder if there is a reason why it is not there. But since we need it in toggle_fullscreen_mode, ok, let's add it.



> > I am not sure I want to show the button each time the mouse is moved... I'd
> > prefer to show it only when mouse is moved out the top of the screen. Opinions?
> 
> Yes, the GNOME HIG recite:
> 
> "Pressing ESC should cause the application to leave full screen mode. A Leave
> Fullscreen button should be placed in the upper right hand corner of the
> window. The button should disappear after the mouse is unused for 5 seconds,
> and should appear again when the moused is moved. Alternately, in applications
> where the mouse is used frequently in full screen mode, all but a two pixel row
> of the button may be slid off the top of the screen. The button should slide
> back on the screen when the mouse moves near it."
> 
> Gedit users don't use the mouse frequently, though, since Gedit is a text
> editor and to type text they should use the keyboard.  In addiction, the button
> don't pollute too much the visual. So, the HIG approach is to show the button
> every time the mouse is moved for 5 seconds.
> 
> [cut]
> 

Dunno... I think that users of gedit fullscreen will use the mouse: for instance I often use it for cut&paste (especially middle-mouse-button paste). Also I think some things like the filemanager sidepane etc could also make sense in fullscreen mode and there the mouse is used a lot. So I'd go for the other solution suggested by the HIG.





Comment 14 Paolo Borelli 2008-11-29 14:01:10 UTC
*** Bug 527503 has been marked as a duplicate of this bug. ***
Comment 15 Paolo Borelli 2009-01-05 13:29:35 UTC
*** Bug 396284 has been marked as a duplicate of this bug. ***
Comment 16 Ignacio Casal Quinteiro (nacho) 2009-01-07 12:44:44 UTC
Created attachment 125927 [details] [review]
fullscreen

This should fix the bug.
Comment 17 Ignacio Casal Quinteiro (nacho) 2009-01-07 15:10:51 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.

Note You need to log in before you can comment on or make changes to this bug.