Closed
Bug 1433092
Opened 6 years ago
Closed 6 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•6 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•6 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•6 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•6 years ago
|
||
Pushed by stransky@redhat.com: https://hg.mozilla.org/integration/autoland/rev/360486fecfe8 Add spacing around titlebar buttons, r=jhorak
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/360486fecfe8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 15•6 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•6 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•6 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
•