Closed Bug 416003 Opened 16 years ago Closed 16 years ago

Use different arrow widgets for arrows in different context

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta5

People

(Reporter: frnchfrgg, Assigned: frnchfrgg)

References

Details

Attachments

(1 file, 9 obsolete files)

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.
_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.
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.
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)
Sorry, I meant "to accomodate bug 415163"
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+
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)
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 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+
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+
Blocks: 416868
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 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+
Attached patch Patch v2.3 (obsolete) — Splinter Review
Addresses Teune's comments. Carrying review.
Attachment #302839 - Attachment is obsolete: true
Attachment #303016 - Flags: review+
Attached patch Patch v2.4 (obsolete) — Splinter Review
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)
Attached patch Patch v3 (obsolete) — Splinter Review
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 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+
Attached patch Patch v3.1 (obsolete) — Splinter Review
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+
Attached patch Patch v3.2 (obsolete) — Splinter Review
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.
Blocks: 406475
No longer blocks: 406475
Blocks: 412773
Attachment #303634 - Flags: superreview?(roc)
Attached patch patch v3.5Splinter Review
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
Closed: 16 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.

Attachment

General

Created:
Updated:
Size: