Closed Bug 1433092 Opened 5 years ago Closed 5 years ago

[CSD] Titlebar button spacing is missing

Categories

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

Product:
Core
Component:
Widget: Gtk
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1433092 - Add spacing around titlebar buttons,
59 bytes, text/x-review-board-request
jhorak
: review+
Details
button alignment
12.64 KB, image/png
Details 
Gtk+ uses spacing between titlebar buttons - the buttons are places at vbox with default spacing 6pix.
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.
Attachment #8952128 - Flags: review?(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.
Attachment #8952128 - Flags: review?(jhorak)
Comment on attachment 8952128 [details]
Bug 1433092 - Add spacing around titlebar buttons,

https://reviewboard.mozilla.org/r/221362/#review229532
Attachment #8952128 - Flags: review?(jhorak) → review+
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/360486fecfe8
Add spacing around titlebar buttons, r=jhorak
https://hg.mozilla.org/mozilla-central/rev/360486fecfe8
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Depends on: 1441899
See Also: → 1442364
See Also: → 1442366
Attached image 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?
(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!
Depends on: 1451792
You need to log in before you can comment on or make changes to this bug.