Created attachment 239868 [details]
layout problem with search box opened

This is also related to https://bugzilla.gnome.org/show_bug.cgi?id=701062

*** Bug 701062 has been marked as a duplicate of this bug. ***

Created attachment 273035 [details] [review]
Toolbar in fullscreen mode not covering other elements

It seems the bug is caused because an overlay gtk element was used, which allows other gtk elements to be positioned over each other. Removing this use of an overlay element appears to fix the issue.

Review of attachment 273035 [details] [review]:

It does not seem to fix the issue.  However, the overlay has a
purpose, which is to allow hiding the toolbar when is not in use.

I think the issue is caused by the delay to hide the toolbar,
which requires no activity in the mouse to hide the toolbar
and then to show immediately after any pointer movement.

Evince could mimic Gedit's behaviour (at least 3.4) which is:
* Hide the toolbar right after the mouse leave the toolbar area
* Show the toolbar back when the mouse enters the zone where
  the toolbar is/should be

However, I have no clue on how get the toolbar back only with
the keyboard.

Created attachment 273256 [details] [review]
Fixes the toolbar behavior in fullscreen mode

I tried to mimic gedit's behaviour. The animation does not look so nice though, even if a shadow is added to the toolbar (I tried it, but the shadow does not move with the toolbar, it only appear in the end). Maybe because gedit uses GTK 3.10? Apart from that, it works here.

Review of attachment 273256 [details] [review]:

Thanks for the patch, I like this approach and the animation works perfectly here. I have a few minor comments. We still need a way to show the toolbar using the keyboard.

::: shell/ev-window.c
@@ +159,3 @@
 	GtkWidget *fs_overlay;
+	GtkWidget *toolbar_eventbox;
+	GtkWidget *toolbar_revealer;

Use fs_ prefix in both cases

@@ +286,3 @@
 #define TOOLBAR_RESOURCE_PATH "/org/gnome/evince/shell/ui/toolbar.xml"
 
+#define FULLSCREEN_POPUP_TIMEOUT 0

Do not use 0 timeouts, use g_idle instead. Although in this case, I think we should keep the toolbar visible for a little time before hiding it again when the mouse is moved out of the toolbar.

@@ +287,3 @@
 
+#define FULLSCREEN_POPUP_TIMEOUT 0
+#define FULLSCREEN_MOTION_TIME 1000 /* in milliseconds */

I think this should be FULLSCREEN_TRANSITION_TIME now or something similar.

@@ +4098,3 @@
 {
 	if (!ev_toolbar_has_visible_popups (EV_TOOLBAR (window->priv->toolbar)))
+		gtk_revealer_set_reveal_child ( GTK_REVEALER (window->priv->toolbar_revealer), FALSE);

Don't add space after the (

@@ +4131,3 @@
 					   EvWindow         *window)
 {
+	if(gtk_revealer_get_reveal_child( GTK_REVEALER (window->priv->toolbar_revealer) )) {

Add space betweeh in and (, and between child and (, and remove the spaces before GTK_REVEALER and after the )

if (gtk_revealer_get_reveal_child (GTK_REVEALER (window->priv->toolbar_revealer))) {

@@ +4134,3 @@
+		ev_window_remove_fullscreen_timeout (window);
+	} else {
+		gtk_revealer_set_reveal_child ( GTK_REVEALER (window->priv->toolbar_revealer), TRUE);

Remove the space between ( and GTK_REVEALER

@@ +4147,3 @@
 					   EvWindow  *window)
 {
+	if(gtk_revealer_get_reveal_child( GTK_REVEALER (window->priv->toolbar_revealer) )) {

Same here about spaces.

@@ +4181,3 @@
+	gtk_widget_set_valign (window->priv->toolbar_eventbox, GTK_ALIGN_START);
+	gtk_revealer_set_transition_type (GTK_REVEALER (window->priv->toolbar_revealer), GTK_REVEALER_TRANSITION_TYPE_SLIDE_UP );
+	gtk_revealer_set_transition_duration (GTK_REVEALER (window->priv->toolbar_revealer), FULLSCREEN_MOTION_TIME );

Remove the space before the last ) on both cases here.

@@ +4214,3 @@
 	ev_window_update_fullscreen_action (window);
 
+	gtk_revealer_set_reveal_child ( GTK_REVEALER(window->priv->toolbar_revealer), TRUE);

Remove the space after the (

@@ +4215,3 @@
 
+	gtk_revealer_set_reveal_child ( GTK_REVEALER(window->priv->toolbar_revealer), TRUE);
+	ev_window_add_fullscreen_timeout (window);

I think it's a good idea to show the toolbar right after entering fullscreen mode, and hide it after a while, so that the user knows there's a hidden toolbar there, but with the 0 timeout you are using the toolbar is not shown in the end.

Thank you for the feedback! I solved the issues with names and spaces, and also implemented a function that will show the toolbar when some key is pressed. I don't know which key combination to choose though... I am using Shift+Space, but this sounds not very intuitive. Do you have any suggestions?

I would use Ctrl + T. This option is currently used for the Save current settings as default, but this is an option that is intended to be used seldomly, so we could remove the shorcut for this option and use Ctrl+T for this instead.  Carlos, what do you think?

I don't know. One thing I noticed is that if you press F10 to show the gear menu when the toolbar is hidden, the menu is shown, but the toolbar remains hidden. We should show the toolbar in that case. It's not a bug in this patch, but in current code, I noticed when trying to show the toolbar with the keyboard. I don't want to block this patch on this, since the keyboard access is also a problem with the current code, so please, submit a new a patch without the key shortcut and move the keyboard access to a different bug.

Btw, we used to have shift+ctrl+t, but we removed it long time ago because ephy removed it. See bugs #350098 and #328783

Created attachment 273639 [details] [review]
Fixes the toolbar behavior in fullscreen mode

The function that reads a key and shows the toolbar is implemented, only checking for F10 at the moment. So if F10 is pressed, the menu opens and the toolbar is shown. This should fix the issue Carlos observed.

Whenever a key is chosen to show the toolbar, it is simple to add it there.

Review of attachment 273639 [details] [review]:

Thanks!, I've done some other modifications and pushed to git master.

::: shell/ev-window.c
@@ -160,2 @@
 	gint64     fs_motion_start_time;
 	guint      fs_motion_n_events;

These are unused now, so they can be removed.

@@ +4100,3 @@
 }
 
+

Extra line here

@@ +4104,3 @@
 fullscreen_toolbar_timeout_cb (EvWindow *window)
 {
 	window->priv->fs_timeout_id = 0;

I've noticed this is also wrong, because we are always reseting the timeout id, even when this function can return TRUE keeping the source alive.

@@ -4118,2 @@
 static void
-ev_window_fullscreen_show_toolbar (EvWindow *window)

I think we can keep this method to simplify the code a bit. It can check if the toolbar is visible or not, and start the timer when it's shown but the pointer is not inside the toolbar.

@@ +4135,3 @@
+	} else {
+		gtk_revealer_set_reveal_child (GTK_REVEALER (window->priv->fs_revealer), TRUE);
+	}

That way here you only need to call ev_window_fullscreen_show_toolbar

@@ +4147,3 @@
 {
+	if (gtk_revealer_get_reveal_child (GTK_REVEALER (window->priv->fs_revealer))) {
+		ev_window_add_fullscreen_timeout (window);

We can do this unconditionally.

@@ +4155,3 @@
 
+static gboolean
+ev_window_fullscreen_toolbar_key_press (GtkWidget   *widget,

This is not the way to do this, we already have a callback for the F10 shortcut, we should show the toolbar there, right before showing the menu.

@@ +4231,3 @@
 
+	gtk_revealer_set_reveal_child (GTK_REVEALER(window->priv->fs_revealer), TRUE);
+	ev_window_add_fullscreen_timeout (window);

Here you would only need to call ev_window_fullscreen_show_toolbar() too.

@@ +5717,3 @@
 					      "This feature places a moveable cursor in text pages, "
 					      "allowing you to move around and select text with your keyboard. "
+					      "Do you want to enable the caret navigation on?"));

What?

I'm keeping the bug open, because this patch improves a lot the situation but still doesn't solve the issue. I think we should hide the toolbar without the timeout when opening the search bar, and we should move the sidebar menu to the toolbar.

*** Bug 731340 has been marked as a duplicate of this bug. ***

Created attachment 296770 [details] [review]
Shows/hides toolbar when using find or command menu

Hi!

Firstly, I would like to say that this is my first contribution to Open Source so I hope I did everything right. I would love to get some feedback.

The patch is explained in commit message, there is one problem about it and I don't know what to do with - when "find_bar" is shown the "fs_toolbar" is always invisible and when user presses F10 then, there is still command menu appearing from nowhere. Should "find_bar" be covered then?

Review of attachment 296770 [details] [review]:

Thanks for the patch and sorry for the delay to review it. I like the idea, I have only a couple of comments

::: shell/ev-window.c
@@ +4914,3 @@
+
+	if (ev_window->priv->fs_toolbar)
+		ev_window_fullscreen_show_toolbar (ev_window);

It's a bit weird that the menu appears first and then the toolbar is revealed in a transition. For this particular case I think we could check that the toolbar is not already revealed and then save the current transition type, change the transition to NONE, reveal the toolbar, and then set the transition again.

@@ +5390,3 @@
 	gtk_widget_show (ev_window->priv->find_sidebar);
 
+	if (gtk_widget_get_visible (ev_window->priv->fs_toolbar))

Note that fs_toolbar can be NULL at this point. I don't think we need to check if it's visible, we want to hide it anyway, so just check if the pointer is not NULL and hide it.

@@ +5391,3 @@
 
+	if (gtk_widget_get_visible (ev_window->priv->fs_toolbar))
+		gtk_widget_set_visible (ev_window->priv->fs_toolbar, FALSE);

Hiding the child this way works, but confuses the revelear, and we keep a timeout to actually hide the toolbar after two seconds. I think it would be better to use the revealer instead, with the same approach as before, setting a NONE transition, and then restoring it. We could add helper functions to hide/show the toollbar without animations. Hiding the toolbar without animation would also cancel the timeout source.

@@ +5418,3 @@
 	gtk_widget_grab_focus (ev_window->priv->view);
 	g_action_group_change_action_state (G_ACTION_GROUP (ev_window), "toggle-find", g_variant_new_boolean (FALSE));
+	gtk_widget_set_visible (ev_window->priv->fs_toolbar, TRUE);

Maybe it would be better to ensure the toolbar is revealed if it was revelaed when the search started. You should also null check fs_toolbar here.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/evince/issues/336.