Closed
Bug 1433092
Opened 7 years ago
Closed 7 years ago
[CSD] Titlebar button spacing is missing
Categories
(Core :: Widget: Gtk, defect, P2)
Core
Widget: Gtk
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: stransky, Assigned: stransky)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Gtk+ uses spacing between titlebar buttons - the buttons are places at vbox with default spacing 6pix.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment 12•7 years ago
|
||
mozreview-review |
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+
Comment 13•7 years ago
|
||
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/360486fecfe8
Add spacing around titlebar buttons, r=jhorak
Comment 14•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 15•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
(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!
You need to log in
before you can comment on or make changes to this bug.
Description
•