1433092 - [CSD] Titlebar button spacing is missing

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P2)

Description

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

Gtk+ uses spacing between titlebar buttons - the buttons are places at vbox with default spacing 6pix.

Comment 1

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

Attachment: Bug 1433092 - Add spacing around titlebar buttons,

GtkHeaderBar has property "spacing" which defines space between buttons at titlebar.
Get this property and apply as margin to titlebar buttons when there's more than one.
Also cache this value for furter use at titlebar metrics cache.

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

Comment 2

[Jan Horak [:jhorak]]

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

https://reviewboard.mozilla.org/r/221362/#review227466

::: commit-message-ad133:1
(Diff revision 1)
> +Bug 1433092 - Add spacing among titlebar buttons, r?jhorak

s/among/around

::: widget/gtk/gtk3drawing.cpp:313
(Diff revision 1)
>      return MOZ_GTK_SUCCESS;
>  }
>  
> +// We support LTR layout only here for now.
> +static void
> +InitAddToolbarButtonSpacing(WidgetNodeType aWidgetType,

The AddTollbarButtonSpacing as a name of the function should be better, because I don't see any initialization there.

::: widget/gtk/gtk3drawing.cpp:346
(Diff revision 1)
> +  buttonSpacing /= 2;
> +
> +  static_assert(TOOLBAR_BUTTONS >= 4,
> +                "We need at least 4 titlebar buttons available.");
> +  // It's a first button - apply spacing to right side only.
> +  if (buttonPosition == 0) {

Please use switch there.

::: widget/gtk/gtk3drawing.cpp:363
(Diff revision 1)
> +      *spacingLeft += buttonSpacing;
> +  }
> +}
> +
>  static void
>  InitToolbarButtonMetrics(ToolbarButtonGTKMetrics *aButtonMetrics,

There's no initialization of static vars in this function, it just returns ToolbarButtonGTKMetrics, please rename to GetToolbarButtonMetric or similar.

::: widget/gtk/gtk3drawing.cpp:400
(Diff revision 1)
> +                                &aButtonMetrics->buttonMargin.right);
> +
>      gint left = 0, top = 0, right = 0, bottom = 0;
> -    moz_gtk_add_margin_border_padding(style, &left, &top, &right, &bottom);
> +    moz_gtk_add_border_padding(style, &left, &top, &right, &bottom);
>  
> +    // Buton size is calculated as min-width/height + border/padding.

typo Buton/Button

::: widget/gtk/gtk3drawing.cpp:418
(Diff revision 1)
> +    aButtonMetrics->minSizeWithBorderMargin.height = height +
> +        aButtonMetrics->buttonMargin.top + aButtonMetrics->buttonMargin.bottom;
>  }
>  
> -const ToolbarButtonGTKMetrics*
> -GetToolbarButtonMetrics(WidgetNodeType aWidgetType)
> +static void
> +InitAllToolbarButtonMetrics(void)

Hm, there seems to be a lot of Init functions which starts to be confusing.

InitAllToolbarButtonMetrics -> EnsureToolbarMetrics (or keep the InitToolbarButtonMetrics)
InitToolbarButtonMetrics -> CreateToolbarButtonMetrics
InitAddToolbarButtonSpacing -> AddToolbarButtonSpacing
or consider better names.

Comment 3

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

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

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

Comment 4

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

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

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

Comment 5

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

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

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

Comment 6

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

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

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

Comment 7

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

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221362/diff/5-6/

Comment 8

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

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221362/diff/6-7/

Comment 9

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

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221362/diff/7-8/

Comment 10

[Jan Horak [:jhorak]]

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

https://reviewboard.mozilla.org/r/221362/#review228574

::: widget/gtk/gtk3drawing.cpp:364
(Diff revision 8)
> -const ToolbarButtonGTKMetrics*
> -GetToolbarButtonMetrics(WidgetNodeType aWidgetType)
> +// We support LTR layout only here for now.
> +static void
> +CalculateToolbarButtonSpacing(WidgetNodeType aWidgetType)
> +{
> +    int buttonIndex = (aWidgetType - MOZ_GTK_HEADER_BAR_BUTTON_CLOSE);
> +    ToolbarButtonGTKMetrics* metrics = sToolbarMetrics.button + buttonIndex;

Could you please rather pass ToolbarButtonGTKMetrics as a function argument to avoid direct access to the static/global variable? You won't need to compute the buttonIndex after that.

Same for CalculateToolbarButtonMetrics.

Ideally that we use sToolbarMetrics only in GetToolbarButtonMetrics and EnsureToolbarMetrics.

::: widget/gtk/gtk3drawing.cpp:395
(Diff revision 8)
> +    metrics->minSizeWithBorderMargin.height +=
> +        metrics->buttonMargin.top + metrics->buttonMargin.bottom;
> +}
> +
> +static int
> +GetGtkHeaderBarLayout(WidgetNodeType* aButtonLayout, int aMaxButtonNums)

nit: please use GetGtkHeaderBarButtonLayout

::: widget/gtk/gtk3drawing.cpp:398
(Diff revision 8)
> +
> +static int
> +GetGtkHeaderBarLayout(WidgetNodeType* aButtonLayout, int aMaxButtonNums)
> +{
> +  NS_ASSERTION(aMaxButtonNums >= TOOLBAR_BUTTONS,
> +               "We're missing titlebar buttons!");

This is a bit misleading, please use something like:
Requested number of buttons is higher than storage capacity.

::: widget/gtk/gtk3drawing.cpp:439
(Diff revision 8)
> +
> +  return activeButtonNums;
> +}
> +
> +static void
> +EnableAndPlaceTitlebarButtons()

I think you can put this into EnsureToolbarMetrics or possibly join with the code in EnsureToolbarMetrics.

::: widget/gtk/gtk3drawing.cpp:456
(Diff revision 8)
> +        }
> +        // Mark last button.
> +        if (i == (activeButtonNums-1)) {
> +            metrics->lastButton = true;
> +        }
> +    }

...and call CalculateToolbar\* stuff there.

Comment 11

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

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221362/diff/8-9/

Comment 12

[Jan Horak [:jhorak]]

Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

https://reviewboard.mozilla.org/r/221362/#review229532

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/360486fecfe8
Add spacing around titlebar buttons, r=jhorak

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/360486fecfe8

Comment 15

[Marco Castelluccio [:marco]]

The spacing isn't exactly the same as other GNOME applications like Gedit (there's more spacing in Firefox compared to Gedit). I'll wait for bug 1434646 to be fixed before filing a bug.

Comment 16

[3ship]

Attachment: button alignment

I noticed that the spacing between the window control buttons and the vertical border is slightly off. See attachment (Arc GTK theme - top: Firefox; bottom: Nautilus)

On a different note: In Arc GTK theme the window buttons of *inactive* windows are faded to grayscale, while in Firefox with drawInTitlebar enabled they are not. Should I make a seperate bug report for this or is it on purpose?

Comment 17

[Dão Gottwald [:dao]]

(In reply to 3ship from comment #16)
> On a different note: In Arc GTK theme the window buttons of *inactive*
> windows are faded to grayscale, while in Firefox with drawInTitlebar enabled
> they are not. Should I make a seperate bug report for this or is it on
> purpose?

It's not on purpose, please file a bug. Thanks!