Implement titlebar and titlebar button drawing using GtkHeaderBar.

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Blocks 1 bug)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: tpi:+)

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
Follow up from Bug 1283299 Part 6: Implement titlebar and titlebar button drawing using GtkHeaderBar.
(Assignee)

Updated

2 years ago
Summary: Part 6: Implement titlebar and titlebar button drawing using GtkHeaderBar. → Implement titlebar and titlebar button drawing using GtkHeaderBar.

Updated

2 years ago
Whiteboard: tpi:+
Assignee: nobody → stransky
Status: NEW → ASSIGNED

Comment 2

2 years ago
mozreview-review
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.
Attachment #8867671 - Flags: review?(karlt) → review?(michael)
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.
Attachment #8867671 - Flags: review?(michael) → review?(karlt)
Jim - can you find someone to review this?
Flags: needinfo?(jmathies)
(Assignee)

Comment 5

2 years ago
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.
Attachment #8867671 - Flags: review?(karlt)
I can help find reviewers or do the reviews myself if need be, just ni me.
Flags: needinfo?(jmathies)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
(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
Flags: needinfo?(jmathies)
(Assignee)

Comment 13

2 years ago
Jim, any update here? Thanks.
(Assignee)

Comment 14

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

Karl can't do this, I'll take them.
Flags: needinfo?(jmathies)
Attachment #8867671 - Flags: review?(karlt) → review?(jmathies)
Oh, you're using moz review. Martin, can you rerout those review requests my way?

Updated

2 years ago
Flags: needinfo?(stransky)

Comment 22

2 years ago
mozreview-review-reply
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.
(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

2 years ago
mozreview-review
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.
Attachment #8867671 - Flags: review+

Comment 25

2 years ago
mozreview-review
Comment on attachment 8909944 [details]
Bug 1364843 - Implement MOZ_GTK_HEADER_BAR* theme entries,

https://reviewboard.mozilla.org/r/181426/#review193850
Attachment #8909944 - Flags: review?(karlt) → review+

Comment 26

2 years ago
mozreview-review
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

2 years ago
mozreview-review
Comment on attachment 8909942 [details]
Bug 1364843 - Allow MOZ_GTK_HEADER_BAR* widget creation,

https://reviewboard.mozilla.org/r/181422/#review193852
Attachment #8909942 - Flags: review?(karlt)
Attachment #8909941 - Flags: review?(karlt)
Attachment #8909943 - Flags: review?(karlt)
Attachment #8909941 - Flags: review?(jhorak)
Attachment #8909943 - Flags: review?(jhorak)
Sorry, I was working through bugmail while Jim was answering and didn't see his comment.  I'm very happy for Jim to review.
(Assignee)

Updated

2 years ago
Attachment #8909942 - Flags: review?(jhorak)
(Assignee)

Comment 29

2 years ago
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.
Flags: needinfo?(stransky)

Comment 30

2 years ago
mozreview-review
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);
Attachment #8909943 - Flags: review?(jhorak)

Comment 31

2 years ago
mozreview-review
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.
Attachment #8909941 - Flags: review?(jhorak)

Comment 32

2 years ago
mozreview-review
Comment on attachment 8909942 [details]
Bug 1364843 - Allow MOZ_GTK_HEADER_BAR* widget creation,

https://reviewboard.mozilla.org/r/181422/#review194078
Attachment #8909942 - Flags: review?(jhorak) → review+

Comment 33

2 years ago
mozreview-review
Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

https://reviewboard.mozilla.org/r/139254/#review194178
Attachment #8867671 - Flags: review?(jmathies) → review+
(Assignee)

Comment 34

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8909942 - Flags: review?(karlt) → review?(jhorak)
(Assignee)

Updated

2 years ago
Attachment #8909943 - Flags: review?(jhorak)
(Assignee)

Updated

2 years ago
Attachment #8909941 - Flags: review?(jhorak)
(Assignee)

Updated

2 years ago
Attachment #8909941 - Flags: review?(jhorak)
Attachment #8909942 - Flags: review?(jhorak)
Attachment #8909943 - Flags: review?(jhorak)
(Assignee)

Updated

2 years ago
Attachment #8867671 - Flags: review?(jhorak)
Attachment #8909944 - Flags: review?(jhorak)
Hey Martin, just checking here, if you need review help please flag me.
(Assignee)

Comment 41

2 years ago
(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 :)
(Assignee)

Updated

2 years ago
Blocks: 1409493

Comment 42

2 years ago
mozreview-review
Comment on attachment 8867671 [details]
Bug 1364843 - Added WidgetNodeType entries for GtkHeaderBar implementation,

https://reviewboard.mozilla.org/r/139254/#review195736
Attachment #8867671 - Flags: review?(jhorak) → review+

Comment 43

2 years ago
mozreview-review
Comment on attachment 8909941 [details]
Bug 1364843 - Implement GtkHeaderBar widgets at WidgetCache,

https://reviewboard.mozilla.org/r/181420/#review195740
Attachment #8909941 - Flags: review+

Comment 44

2 years ago
mozreview-review
Comment on attachment 8909942 [details]
Bug 1364843 - Allow MOZ_GTK_HEADER_BAR* widget creation,

https://reviewboard.mozilla.org/r/181422/#review195752
Attachment #8909942 - Flags: review+

Comment 45

2 years ago
mozreview-review
Comment on attachment 8909943 [details]
Bug 1364843 - Implement drawing and size query of MOZ_GTK_HEADER_BAR*,

https://reviewboard.mozilla.org/r/181424/#review196284
Attachment #8909943 - Flags: review+

Comment 46

2 years ago
mozreview-review
Comment on attachment 8909944 [details]
Bug 1364843 - Implement MOZ_GTK_HEADER_BAR* theme entries,

https://reviewboard.mozilla.org/r/181426/#review196288
Attachment #8909944 - Flags: review?(jhorak) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed

Comment 48

2 years ago
Pushed by stransky@redhat.com:
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
You need to log in before you can comment on or make changes to this bug.