Use different arrow widgets for arrows in different context

RESOLVED FIXED in mozilla1.9beta5

Status

()

Core
Widget: Gtk
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Julien "_FrnchFrgg_" RIVAUD, Assigned: Julien "_FrnchFrgg_" RIVAUD)

Tracking

Trunk
mozilla1.9beta5
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

20.46 KB, patch
beltzner
: approval1.9+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Currently every arrow (button down arrows, editable menulists dropdown arrow, and scrollbar arrows) uses the gArrowWidget, which is attached to the gComboBoxEntryWidget. This means that metrics and dimensions can be wrong, and even the rendering be wrong (except for the scrollbars which pass the scrollbar widget to the drawing function, not the arrow widget) since some themes change their rendering depending on what widget the arrow is attached to (eg. Clearlooks).

This is needed for bug 415830 since we'll then need to draw a non editable "combobox arrow", and not a "ComboBoxEntry arrow" (the two differ in Clearlooks).

Patch coming soon.

Comment 1

10 years ago
_FrnchFrgg_, before you attach a patch, maybe you should take a look at bug 415163, since you may want to take into account that the arrow size calculations are being changed a lot there.
(Assignee)

Comment 2

10 years ago
I didn't add a gScrollbarArrowWidget, since the use of an arrow widget to get the scrollbar button rect was wrong, we should just use the passed rect argument (which is what gtk does); the correct metrics are retrieved directly from the scrollbar widget anyway in moz_gtk_get_scrollbar_metrics.
(Assignee)

Comment 3

10 years ago
Created attachment 301782 [details] [diff] [review]
Use different arrow widgets for arrows in different context

This patch is mostly cleanup, and shouldn't change much to accomodate bug 416003. I'll do it now, but wanted nevertheless to post the patch.
Assignee: nobody → frnchfrgg-mozbugs
Status: NEW → ASSIGNED
Attachment #301782 - Flags: review?(ventnor.bugzilla)
(Assignee)

Comment 4

10 years ago
Sorry, I meant "to accomodate bug 415163"

Comment 5

10 years ago
Comment on attachment 301782 [details] [diff] [review]
Use different arrow widgets for arrows in different context

> static void
>-moz_gtk_get_dropdown_button(GtkWidget *widget,
>+moz_gtk_get_combo_box_entry_button(GtkWidget *widget,
>                             gpointer client_data)

Fix the alignment of the second argument.
Attachment #301782 - Flags: review?(ventnor.bugzilla) → review+
(Assignee)

Comment 6

10 years ago
Created attachment 301799 [details] [diff] [review]
Use different arrow widgets for arrows in different context [v2]

This time on top of the patch of bug 415163. The differences with previous patch are minor, mostly applying the same treatment to the added code. I'll ask superreview when 415163 gets superreview+ or approval+
Attachment #301782 - Attachment is obsolete: true
Attachment #301799 - Flags: review?(ventnor.bugzilla)
(Assignee)

Comment 7

10 years ago
I set 415163 blocking because the added code there also need to get the same treatment, so bug 415163 landing as-is would reopen this bug if fixed before. I think it's better to first land 415163, then this bug.
Depends on: 415163

Comment 8

10 years ago
Comment on attachment 301799 [details] [diff] [review]
Use different arrow widgets for arrows in different context [v2]


> static gint
>-calculate_arrow_dimensions(GdkRectangle* rect, GdkRectangle* arrow_rect,
>-                           GtkTextDirection direction)
>+calculate_arrow_dimensions(GtkWidget* arrown GdkRectangle* rect,
>+                           GdkRectangle* arrow_rect, GtkTextDirection direction)
> {

Typo, GtkWidget* arrown
Try compiling the code before submitting it as a patch :)

>     case MOZ_GTK_DROPDOWN_ARROW:
>-        return moz_gtk_dropdown_arrow_paint(drawable, rect, cliprect, state,
>+        return moz_gtk_combo_box_entry_button_paint(drawable, rect, cliprect, state,
>                                             flags, direction);

Update the alignment.
Attachment #301799 - Flags: review?(ventnor.bugzilla) → review+
(Assignee)

Comment 9

10 years ago
Created attachment 301803 [details] [diff] [review]
Use different arrow widgets for arrows in different context [v2.1]

Addressed comments. This patch also corrects a mistake in the patch of bug 415163 (a missing calculate_arrow_dimensions)
Attachment #301799 - Attachment is obsolete: true
Attachment #301803 - Flags: review+
(Assignee)

Updated

10 years ago
Blocks: 416868
(Assignee)

Comment 10

10 years ago
Created attachment 302839 [details] [diff] [review]
Patch v2.2 (update to latest trunk of patch reviewed by ventron)

Could you verify that this update to latest trunk is correct, to be on top of your patch ? Then I can carry ventron's review+.
Attachment #301803 - Attachment is obsolete: true
Attachment #302839 - Flags: review?(twanno)

Comment 11

10 years ago
Comment on attachment 302839 [details] [diff] [review]
Patch v2.2 (update to latest trunk of patch reviewed by ventron)

This patch applies cleanly.

However there are some other remarks I have:

>+static GtkWidget* gScrollbarArrowWidget;
> [...]
>+ensure_scrollbar_arrow_widget()
When code isn't used there is no need to add it.

>     if (have_arrow_scaling)
>-        gtk_widget_style_get(gArrowWidget,
>-                             "arrow_scaling", &arrow_scaling, NULL);
>-    extent = MIN((rect->width - misc->xpad * 2),
>-                 (rect->height - misc->ypad * 2)) * arrow_scaling;
>+        gtk_widget_style_get(arrow, "arrow_scaling", &arrow_scaling, NULL);
>+        extent = MIN((rect->width - misc->xpad * 2),
>+                     (rect->height - misc->ypad * 2)) * arrow_scaling;
The last two lines were indented correctly.

>-    gArrowWidget = NULL;
Don't forget to add cleanup for the replacement (gButtonArrowWidget)
Attachment #302839 - Flags: review?(twanno) → review+
(Assignee)

Comment 12

10 years ago
Created attachment 303016 [details] [diff] [review]
Patch v2.3

Addresses Teune's comments. Carrying review.
Attachment #302839 - Attachment is obsolete: true
Attachment #303016 - Flags: review+
(Assignee)

Comment 13

10 years ago
Created attachment 303023 [details] [diff] [review]
Patch v2.4

OK, so since I do a lot of mistakes recently, re-requesting review (I had forgotten a ";", not sure how it passed the compile check. I must have compiled the wrong branch).
Attachment #303016 - Attachment is obsolete: true
Attachment #303023 - Flags: review?(twanno)
(Assignee)

Comment 14

10 years ago
Created attachment 303514 [details] [diff] [review]
Patch v3

Latest work on 415163 bitrotted a lot this patch, updated.
Attachment #303023 - Attachment is obsolete: true
Attachment #303514 - Flags: review?(twanno)
Attachment #303023 - Flags: review?(twanno)

Comment 15

10 years ago
Comment on attachment 303514 [details] [diff] [review]
Patch v3

A few nits:

>     ensure_combo_box_entry_widgets();
>+    gtk_widget_set_direction(gComboBoxEntryButtonWidget, direction);
The direction will be set in button_paint.

>     case MOZ_GTK_DROPDOWN_ARROW:
>-        ensure_combo_box_entry_widget();
>-        w = gDropdownButtonWidget;
>+        ensure_combo_box_entry_widgets();
This should already be correct in the patch for bug 415163, to make that patch compile.

>     case MOZ_GTK_DROPDOWN_ARROW:
>-        return moz_gtk_dropdown_arrow_paint(drawable, rect, cliprect, state,
>+        return moz_gtk_combo_box_entry_button_paint(drawable, rect, cliprect, state,
>                                             flags, direction);
The indentation should be corrected.
Attachment #303514 - Flags: review?(twanno) → review+
(Assignee)

Comment 16

10 years ago
Created attachment 303629 [details] [diff] [review]
Patch v3.1

Yeah, sorry for the merge mistakes (the set_direction was removed from a previous patch, and I added it again by copying things around; same thing for the name of widget not correctly changed). I also corrected the indentation as asked.

Carrying review.
Attachment #303514 - Attachment is obsolete: true
Attachment #303629 - Flags: review+
(Assignee)

Comment 17

10 years ago
Created attachment 303634 [details] [diff] [review]
Patch v3.2

Tiny change to make bug 416868 independent from bug 415830.
Attachment #303629 - Attachment is obsolete: true
Attachment #303634 - Flags: review+
How far are we away from landing this? If possible, it would be very nice to have this in beta 4 to get some broader testing.

Updated

10 years ago
Blocks: 406475

Updated

10 years ago
No longer blocks: 406475

Updated

10 years ago
Blocks: 412773
Attachment #303634 - Flags: superreview?(roc)
(Assignee)

Comment 19

10 years ago
Created attachment 305972 [details] [diff] [review]
patch v3.5

Unbitrotted to match recent changes in bug 415163
Attachment #303634 - Attachment is obsolete: true
Attachment #305972 - Flags: superreview?(roc)
Attachment #303634 - Flags: superreview?(roc)
Attachment #305972 - Flags: superreview?(roc) → superreview+
Attachment #305972 - Flags: approval1.9?
Comment on attachment 305972 [details] [diff] [review]
patch v3.5

a1.9=beltzner
Attachment #305972 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.90; previous revision: 1.89
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.55; previous revision: 1.54
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.150; previous revision: 1.149
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
You need to log in before you can comment on or make changes to this bug.