Created attachment 258447 [details] [review]
shell: Port the toolbar to headerbar as our titlebar and add a close button

Created attachment 258448 [details]
Screenshot

How do you maximize/minimize the window? does it scale for documents with a longer title? what happens when you make the window smaller and the title doesn't fit?

Double click on the headerbar to maximize/minimize the window.
If the title is too longer, you see '...' in the title (according to PANGO_ELLIPSIZE_END).
I not think have any problem with the title...

I think you mean maximize/restore the window.

The title might be an issue on non-maximized windows, because it could make Evince look ugly, for example with filenames like:

Encryption_Section_from_SC2N788_ISO_32000-2_NP-Ballot.pdf or
'Bird et al. - 2007 - Open Borders Immigration in Open Source Projects.pdf'

I mean, even with the ellipsis.

Created attachment 258498 [details] [review]
shell: Add some margin around the title in the headerbar

This what Florian do in Polari, I think.

The use of the title might have conflicts with https://bugzilla.gnome.org/show_bug.cgi?id=648279

Yosef, I think that at least we want the part of the patch where you erase the margin on the buttons and center them. Could you please split the patch into two, so they can be reviewed separetedly ? 

Thanks

Right, that indeed fixes bug #709005. I've split the patch, made some minor changes and pushed it to both branches. Thanks!

While doing those changes it would be nice to keep in considerations other environments. While the headerbars look nice in gnome-shell they made applications fit less in other desktops (especially when mixed with non GNOME3 apps/where the theme and wm controls layout is different from the one close button on the right)

(In reply to comment #10)
> While doing those changes it would be nice to keep in considerations other
> environments. While the headerbars look nice in gnome-shell they made
> applications fit less in other desktops (especially when mixed with non GNOME3
> apps/where the theme and wm controls layout is different from the one close
> button on the right)

FWIW, the patch applied fixes a bug that made the toolbar look huge with newer gtk+ (bug #709005)

IMVHO, the HeaderBar present several challenges for Evince, as the ones already mentioned, plus additional ones like:
 - To make the sidebar discoverable, it would be necessary
   an item in the toolbar (see the screenshot in
   https://bugzilla.gnome.org/show_bug.cgi?id=510686#c13)
 - To use the new Recent View, it is necessary a button in
   the toolbar
   https://bugzilla.gnome.org/show_bug.cgi?id=633501
 - To annotate documents, it would be necessary (at least) an
   additional button or combobox.
   No screenshot yet, but it will come once we finish adding the
   support in Poppler.

Created attachment 274247 [details] [review]
shell: Port to GtkHeaderBar as our titlebar and add a close button

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

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

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

(In reply to comment #11)
> (In reply to comment #10)
> > While doing those changes it would be nice to keep in considerations other
> > environments. While the headerbars look nice in gnome-shell they made
> > applications fit less in other desktops (especially when mixed with non GNOME3
> > apps/where the theme and wm controls layout is different from the one close
> > button on the right)
> 
> FWIW, the patch applied fixes a bug that made the toolbar look huge with newer
> gtk+ (bug #709005)
> 
> IMVHO, the HeaderBar present several challenges for Evince, as the ones already
> mentioned, plus additional ones like:
>  - To make the sidebar discoverable, it would be necessary
>    an item in the toolbar (see the screenshot in
>    https://bugzilla.gnome.org/show_bug.cgi?id=510686#c13)
>  - To use the new Recent View, it is necessary a button in
>    the toolbar
>    https://bugzilla.gnome.org/show_bug.cgi?id=633501
>  - To annotate documents, it would be necessary (at least) an
>    additional button or combobox.
>    No screenshot yet, but it will come once we finish adding the
>    support in Poppler.

The general approach on other applications (and Evince itself) seems to be not to expose every feature as its own button, but put some behind one or two menus. Can the same not be done here?

If there's a usability concern with moving to GtkHeaderBar, can you please revert the previous change from before the existence of headerbars that hides the title bar when maximised, as there's a pretty significant usability issue of not having a close button when maximised right now. Or alternatively, use GtkHeaderBar only when maximised, which would effectively just fix that bug without introducing other usability concerns.

Review of attachment 274247 [details] [review]:

Thanks for the patch. It seems the page selector is now expanded, any idea why?

::: shell/ev-toolbar.c
@@ -176,3 @@
-                gtk_widget_set_margin_left (tool_item, 12);
-        else
-                gtk_widget_set_margin_right (tool_item, 12);

Why are you removing the original separation of the items in the toolbar?

@@ +203,1 @@
+        gtk_widget_show_all (GTK_WIDGET (ev_toolbar));

Why did you remove all the gtk_widget_show? and why do you show the toolbar here? I expect the one using the toolbar to decide whether to show it or not and when. I'm not a fan of show_all, I prefer the previous approach, and I think it has nothing to do with using a toolbar or a headerbar. Please, use a different bug if you want to propose changes unrelated to this one.

::: shell/ev-window-title.c
@@ +156,3 @@
 	case EV_WINDOW_TITLE_DOCUMENT:
 		gtk_window_set_title (window, title);
+		gtk_header_bar_set_title (header_bar, title);

Since the toolbar receives the window as construct paramter, and ev-window-title sets the window title. I think it would be easier if the toolbar uses the window to set its title. We can probably bind both title properties

::: shell/ev-window.c
@@ -7419,2 @@
 	ev_window->priv->toolbar = ev_toolbar_new (ev_window);
-	gtk_widget_set_no_show_all (ev_window->priv->toolbar, TRUE);

Why?

(In reply to comment #16)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > While doing those changes it would be nice to keep in considerations other
> > > environments. While the headerbars look nice in gnome-shell they made
> > > applications fit less in other desktops (especially when mixed with non GNOME3
> > > apps/where the theme and wm controls layout is different from the one close
> > > button on the right)
> > 
> > FWIW, the patch applied fixes a bug that made the toolbar look huge with newer
> > gtk+ (bug #709005)
> > 
> > IMVHO, the HeaderBar present several challenges for Evince, as the ones already
> > mentioned, plus additional ones like:
> >  - To make the sidebar discoverable, it would be necessary
> >    an item in the toolbar (see the screenshot in
> >    https://bugzilla.gnome.org/show_bug.cgi?id=510686#c13)
> >  - To use the new Recent View, it is necessary a button in
> >    the toolbar
> >    https://bugzilla.gnome.org/show_bug.cgi?id=633501
> >  - To annotate documents, it would be necessary (at least) an
> >    additional button or combobox.
> >    No screenshot yet, but it will come once we finish adding the
> >    support in Poppler.
> 
> The general approach on other applications (and Evince itself) seems to be not
> to expose every feature as its own button, but put some behind one or two
> menus. Can the same not be done here?
> 
> If there's a usability concern with moving to GtkHeaderBar, can you please
> revert the previous change from before the existence of headerbars that hides
> the title bar when maximised, as there's a pretty significant usability issue
> of not having a close button when maximised right now. Or alternatively, use
> GtkHeaderBar only when maximised, which would effectively just fix that bug
> without introducing other usability concerns.

I think we should move to the headerbar in master and revert the fullscreen patch in stable branch

Comment on attachment 274247 [details] [review]
shell: Port to GtkHeaderBar as our titlebar and add a close button

I've finally reworked this and pushed to git master. Thanks.

hmm, I've broken fullscreen mode, because the headerbar can't be reparented once it's set as a window titlebar, I'll try to fix it or revert this patch otherwise.

I've revert it for now, we need to use a different toolbar widget but our toolbar doesn't allow to create more than one instance due to shared GtkAction

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

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

Now that we don't use GtkActions in the toolbar, I've fixed the fullscreen mode and pushed a new patch to git master.