1364843 - Implement titlebar and titlebar button drawing using GtkHeaderBar.

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement, P3)

Description

[Martin Stránský [:stransky] (ni? me)]

Follow up from Bug 1283299 Part 6: Implement titlebar and titlebar button drawing using GtkHeaderBar.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

Review commit: https://reviewboard.mozilla.org/r/139254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139254/

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

https://reviewboard.mozilla.org/r/139254/#review166622

I'm writing down where I got to on this as Michael has shown an interest in
this work and may be able to review before I can.

Michael, much of work in reviewing here involves checking that this mimics
what is done in the corresponding GTK code.  However, I assume you've seen the
first version of this patch in bug 1283299.  I think my review there
covered everything except what is mentioned in
https://reviewboard.mozilla.org/r/62486/#review117336, and so
remaining is to check those issues are addressed and review the changes since
that patch.

::: commit-message-510b4:2
(Diff revision 1)
> +Bug 1364843 - Implement titlebar and titlebar button drawing using GtkHeaderBar, r?karlt
> +

Even though this has changed somewhat from Andrew's version, please include his name and email address either with yours in the "User" field (which is a free-format text field) or in the commit messages.

::: widget/gtk/WidgetStyleCache.cpp:532
(Diff revision 1)
>    AddToWindowContainer(widget);
>    return widget;
>  }
>  
>  static GtkWidget*
> +CreateHeaderBar(bool aMaximized)

Please avoid boolean parameters where it is not clear from the function name what they mean.

::: widget/gtk/WidgetStyleCache.cpp:546
(Diff revision 1)
> +  GtkWidget* widget = sGtkHeaderBarNewPtr();
> +  AddToWindowContainer(widget);
> +
> +  GtkStyleContext* style = gtk_widget_get_style_context(widget);
> +  gtk_style_context_add_class(style, MOZ_GTK_STYLE_CLASS_TITLEBAR);
> +  // "default-decoration" is a selector for widgets in titlebar

I'm not clear what this comment means.

Do you mean '"default-decoration" indicates that this is a default titlebar, not an application titlebar.'? Or 'Adwaita has selectors depending on "default-decoration"'?

::: widget/gtk/WidgetStyleCache.cpp:549
(Diff revision 1)
> +  if (aMaximized) {
> +    // TODO - is enough to set "maximized" on title bar widget
> +    // or do we need to add "maximized" to window container?
> +    gtk_style_context_add_class(style, "maximized");
> +  }

"maximized" needs to be on the same node as in GTK.

Adwaita, for example, uses:
.maximized .titlebar:not(headerbar)
.maximized headerbar

Postponing support for differences in maximized windows to another patch would be fine, but if this patch adds the class, then it needs to be on the correct node.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

I don't think that I am an appropriate reviewer for these patches. I don't have sufficient experience with gtk to perform the review.

I took a look this weekend to see if I would be able to help, but I don't think I'll be able to catch errors in the usage of the gtk APIs as I have never used them before.

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Jim - can you find someone to review this?

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

I updated the complete patch for latest trunk and working on that as a whole project. No need to review this particular patch right now - I'll attach another (and I hope fixed) one.

Comment 6

[Jim Mathies [:jimm]]

I can help find reviewers or do the reviews myself if need be, just ni me.

Comment 7

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1364843 - Implement GtkHeaderBar widgets at WidgetCache,

Based on patch by Andrew Comminos [:acomminos] <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/181420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181420/

Comment 8

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1364843 - Allow MOZ_GTK_HEADER_BAR* widget creation,

Based on patch by Andrew Comminos [:acomminos] <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/181422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181422/

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1364843 - Implement drawing and size query of MOZ_GTK_HEADER_BAR*,

Based on patch by Andrew Comminos [:acomminos] <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/181424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181424/

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Attachment: Bug 1364843 - Implement MOZ_GTK_HEADER_BAR* theme entries,

Based on patch by Andrew Comminos [:acomminos] <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/181426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181426/

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139254/diff/1-2/

Comment 12

[Martin Stránský [:stransky] (ni? me)]

(In reply to Jim Mathies [:jimm] from comment #6)
> I can help find reviewers or do the reviews myself if need be, just ni me.

Thanks a lot! The patches are ready for review. A complete working version is available at:
https://github.com/stransky/gecko-dev/tree/titlebar-csd

Comment 13

[Martin Stránský [:stransky] (ni? me)]

Jim, any update here? Thanks.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

https://reviewboard.mozilla.org/r/139254/#review166622

> Even though this has changed somewhat from Andrew's version, please include his name and email address either with yours in the "User" field (which is a free-format text field) or in the commit messages.

Fixed.

> Please avoid boolean parameters where it is not clear from the function name what they mean.

I'm not sure how to fix that - do you want to rename the parameter, for instance to "aWithMaximizedStyle" or so?

> I'm not clear what this comment means.
> 
> Do you mean '"default-decoration" indicates that this is a default titlebar, not an application titlebar.'? Or 'Adwaita has selectors depending on "default-decoration"'?

It emulates exactly what create_titlebar() at gtkwindow.c does.

> "maximized" needs to be on the same node as in GTK.
> 
> Adwaita, for example, uses:
> .maximized .titlebar:not(headerbar)
> .maximized headerbar
> 
> Postponing support for differences in maximized windows to another patch would be fine, but if this patch adds the class, then it needs to be on the correct node.

Fixed by MOZ_GTK_WINDOW_MAXIMIZED container.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139254/diff/2-3/

Comment 16

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8909941 [details]
Bug 1364843 - Implement GtkHeaderBar widgets at WidgetCache,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181420/diff/1-2/

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8909942 [details]
Bug 1364843 - Allow MOZ_GTK_HEADER_BAR* widget creation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181422/diff/1-2/

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8909943 [details]
Bug 1364843 - Implement drawing and size query of MOZ_GTK_HEADER_BAR*,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181424/diff/1-2/

Comment 19

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8909944 [details]
Bug 1364843 - Implement MOZ_GTK_HEADER_BAR* theme entries,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181426/diff/1-2/

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

Karl can't do this, I'll take them.

Comment 21

[Jim Mathies [:jimm]]

Oh, you're using moz review. Martin, can you rerout those review requests my way?

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

https://reviewboard.mozilla.org/r/139254/#review166622

> I'm not sure how to fix that - do you want to rename the parameter, for instance to "aWithMaximizedStyle" or so?

The issue is that it is not clear at the caller what the parameter means
because C++ does not have named parameters.

Here, the simple solution is CreateHeaderBar(WidgetNodeType aWidgetType).

The comparison moves from the caller to callee, but it is clear at the call
site what the parameter means.

(If MOZ_GTK_HEADER_BAR_MAXIMIZED didn't already exist, then another enum could
be used to pass HEADER_BAR_MAXIMIZED or HEADER_BAR_NORMAL, or whatever.)

> It emulates exactly what create_titlebar() at gtkwindow.c does.

That's a helpful comment, thanks.  It explains exactly why this is done this way, and provides a specific reference so that someone can check if they suspect an issue or want to change it.

Comment 23

[Darkspirit]

Attachment: Screenshot_20171012_044903.png

(In reply to Martin Stránský from comment #12)
> Thanks a lot! The patches are ready for review. A complete working version is available at:
> https://github.com/stransky/gecko-dev/tree/titlebar-csd

This looks very fancy on KDE @ Debian Testing, thank you very much!
But there are some issues:
1) I can't resize the window as there seems to be no (transparent?) border I could click on.
2) The "Drag Space" checkmark doesn't work. There is always a drag space.
3) The Light Theme has a drag space with a white background above the tabs. (it should be light-grey like inactive tabs)
4) The Default Theme has a thick grey line below the tabs. Is this intentional?

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

https://reviewboard.mozilla.org/r/139254/#review193840

::: widget/gtk/gtkdrawing.h:299
(Diff revision 3)
>    MOZ_GTK_SCROLLED_WINDOW,
> +  /* Paints a GtkHeaderBar */
> +  MOZ_GTK_HEADER_BAR,
> +  /* Paints a GtkHeaderBar in maximized state */
> +  MOZ_GTK_HEADER_BAR_MAXIMIZED,
> +  /* Paints a GtkHeaderBar title buttons */

No 'a' here.  English does not require an indefinite or definite article with plurals.

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 8909944 [details]
Bug 1364843 - Implement MOZ_GTK_HEADER_BAR* theme entries,

https://reviewboard.mozilla.org/r/181426/#review193850

Comment 26

[Karl Tomlinson (:karlt)]

Comment on attachment 8909941 [details]
Bug 1364843 - Implement GtkHeaderBar widgets at WidgetCache,

https://reviewboard.mozilla.org/r/181420/#review193848

Thank you, Martin for splitting up the patches into conceptual parts.  That
makes them easier to understand.

I don't have much time to give to this project, sorry, and so I think we need
to find other reviewers.  Jan has delved into native theming before.  Jan,
would you be willing to review these, please?

::: widget/gtk/WidgetStyleCache.cpp:534
(Diff revision 2)
> +  MOZ_ASSERT(gtk_check_version(3, 10, 0) == nullptr,
> +             "GtkHeaderBar is only available on GTK 3.10+.");
> +  if (gtk_check_version(3, 10, 0) != nullptr)
> +    return nullptr;

No need for both the assert and the early return.
If not used with earlier versions, then just the assert is fine.
If it is used, then the assert need to be removed.

Making these distinctions helps readers understand how and when the code is used, whether error paths are required, etc.

Comment 27

[Karl Tomlinson (:karlt)]

Comment on attachment 8909942 [details]
Bug 1364843 - Allow MOZ_GTK_HEADER_BAR* widget creation,

https://reviewboard.mozilla.org/r/181422/#review193852

Comment 28

[Karl Tomlinson (:karlt)]

Sorry, I was working through bugmail while Jim was answering and didn't see his comment.  I'm very happy for Jim to review.

Comment 29

[Martin Stránský [:stransky] (ni? me)]

Jim, Thanks. I'm going to leave that review on Jan if you don't mind and ask you for review for the next bugs.

Comment 30

[Jan Horak [:jhorak]]

Comment on attachment 8909943 [details]
Bug 1364843 - Implement drawing and size query of MOZ_GTK_HEADER_BAR*,

https://reviewboard.mozilla.org/r/181424/#review194034

::: widget/gtk/gtk3drawing.cpp:318
(Diff revision 2)
> +
> +    rect->x += margin.left;
> +    rect->y += margin.top;
> +    rect->width -= margin.left + margin.right;
> +    rect->height -= margin.top + margin.bottom;
> +

Please use InsetByMargin as in moz_gtk_header_bar_paint and remove unnecessary code.

::: widget/gtk/gtk3drawing.cpp:1974
(Diff revision 2)
> +static gint
> +moz_gtk_header_bar_paint(WidgetNodeType widgetType,
> +                         cairo_t *cr, GdkRectangle* rect, GtkWidgetState* state)
> +{
> +    GtkStateFlags state_flags = GetStateFlagsFromGtkWidgetState(state);
> +    GtkStyleContext *style;

GtkStyleContext *style = GetStyleContext(widgetType, GTK_TEXT_DIR_LTR, state_flags);

Comment 31

[Jan Horak [:jhorak]]

Comment on attachment 8909941 [details]
Bug 1364843 - Implement GtkHeaderBar widgets at WidgetCache,

https://reviewboard.mozilla.org/r/181420/#review194052

::: widget/gtk/WidgetStyleCache.cpp:560
(Diff revision 2)
> +    AddToWindowContainer(headerbar);
> +  }
> +
> +  // Emulate what create_titlebar() at gtkwindow.c does.
> +  GtkStyleContext* style = gtk_widget_get_style_context(headerbar);
> +  gtk_style_context_add_class(style, MOZ_GTK_STYLE_CLASS_TITLEBAR);

It seems that there's no use of MOZ_GTK_STYLE_CLASS_TITLEBAR, use "titlebar" instead and remove above definition.

::: widget/gtk/WidgetStyleCache.cpp:574
(Diff revision 2)
> +{
> +  MOZ_ASSERT(gtk_check_version(3, 10, 0) == nullptr,
> +             "GtkHeaderBar is only available on GTK 3.10+.");
> +
> +  if (gtk_check_version(3, 10, 0) != nullptr)
> +    return nullptr;

Like above from Karl.

::: widget/gtk/WidgetStyleCache.cpp:582
(Diff revision 2)
> +
> +  GtkWidget* widget = gtk_button_new();
> +  gtk_container_add(GTK_CONTAINER(GetWidget(MOZ_GTK_HEADER_BAR)), widget);
> +
> +  GtkStyleContext* style = gtk_widget_get_style_context(widget);
> +  gtk_style_context_add_class(style, MOZ_GTK_STYLE_CLASS_TITLEBUTTON);

I guess it's safe to use "titlebutton" directly.

Comment 32

[Jan Horak [:jhorak]]

Comment on attachment 8909942 [details]
Bug 1364843 - Allow MOZ_GTK_HEADER_BAR* widget creation,

https://reviewboard.mozilla.org/r/181422/#review194078

Comment 33

[Jim Mathies [:jimm]]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

https://reviewboard.mozilla.org/r/139254/#review194178

Comment 34

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8909941 [details]
Bug 1364843 - Implement GtkHeaderBar widgets at WidgetCache,

https://reviewboard.mozilla.org/r/181420/#review195366

::: widget/gtk/WidgetStyleCache.cpp:534
(Diff revision 2)
> +  MOZ_ASSERT(gtk_check_version(3, 10, 0) == nullptr,
> +             "GtkHeaderBar is only available on GTK 3.10+.");
> +  if (gtk_check_version(3, 10, 0) != nullptr)
> +    return nullptr;

Leaved the assert as the code is actually used by Gtk+ > 3.20.

Comment 35

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139254/diff/3-4/

Comment 36

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8909941 [details]
Bug 1364843 - Implement GtkHeaderBar widgets at WidgetCache,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181420/diff/2-3/

Comment 37

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8909942 [details]
Bug 1364843 - Allow MOZ_GTK_HEADER_BAR* widget creation,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181422/diff/2-3/

Comment 38

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8909943 [details]
Bug 1364843 - Implement drawing and size query of MOZ_GTK_HEADER_BAR*,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181424/diff/2-3/

Comment 39

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8909944 [details]
Bug 1364843 - Implement MOZ_GTK_HEADER_BAR* theme entries,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/181426/diff/2-3/

Comment 40

[Jim Mathies [:jimm]]

Hey Martin, just checking here, if you need review help please flag me.

Comment 41

[Martin Stránský [:stransky] (ni? me)]

(In reply to Jim Mathies [:jimm] from comment #40)
> Hey Martin, just checking here, if you need review help please flag me.

Thanks, I'm just having a little wrestling with mozreview :)

Comment 42

[Jan Horak [:jhorak]]

Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

https://reviewboard.mozilla.org/r/139254/#review195736

Comment 43

[Jan Horak [:jhorak]]

Comment on attachment 8909941 [details]
Bug 1364843 - Implement GtkHeaderBar widgets at WidgetCache,

https://reviewboard.mozilla.org/r/181420/#review195740

Comment 44

[Jan Horak [:jhorak]]

Comment on attachment 8909942 [details]
Bug 1364843 - Allow MOZ_GTK_HEADER_BAR* widget creation,

https://reviewboard.mozilla.org/r/181422/#review195752

Comment 45

[Jan Horak [:jhorak]]

Comment on attachment 8909943 [details]
Bug 1364843 - Implement drawing and size query of MOZ_GTK_HEADER_BAR*,

https://reviewboard.mozilla.org/r/181424/#review196284

Comment 46

[Jan Horak [:jhorak]]

Comment on attachment 8909944 [details]
Bug 1364843 - Implement MOZ_GTK_HEADER_BAR* theme entries,

https://reviewboard.mozilla.org/r/181426/#review196288

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Patches 2-4 don't meet the review requirements for Autoland to push this.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4997882c1be8
Added WidgetNodeType entries for GtkHeaderBar implementation, r=jhorak,jimm,karlt
https://hg.mozilla.org/integration/autoland/rev/c0fc86849c94
Implement GtkHeaderBar widgets at WidgetCache, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/01f47a805571
Allow MOZ_GTK_HEADER_BAR* widget creation, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/7b3000e3131c
Implement drawing and size query of MOZ_GTK_HEADER_BAR*, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/f902cf92b444
Implement MOZ_GTK_HEADER_BAR* theme entries, r=jhorak,karlt

Comment 49

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4997882c1be8
https://hg.mozilla.org/mozilla-central/rev/c0fc86849c94
https://hg.mozilla.org/mozilla-central/rev/01f47a805571
https://hg.mozilla.org/mozilla-central/rev/7b3000e3131c
https://hg.mozilla.org/mozilla-central/rev/f902cf92b444