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)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta5
People
(Reporter: frnchfrgg, Assigned: frnchfrgg)
References
Details
Attachments
(1 file, 9 obsolete files)
20.46 KB,
patch
|
roc
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
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•16 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•16 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•16 years ago
|
||
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•16 years ago
|
||
Sorry, I meant "to accomodate bug 415163"
Comment 5•16 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•16 years ago
|
||
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•16 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•16 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•16 years ago
|
||
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 | ||
Comment 10•16 years ago
|
||
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•16 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•16 years ago
|
||
Addresses Teune's comments. Carrying review.
Attachment #302839 -
Attachment is obsolete: true
Attachment #303016 -
Flags: review+
Assignee | ||
Comment 13•16 years ago
|
||
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•16 years ago
|
||
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•16 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•16 years ago
|
||
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•16 years ago
|
||
Tiny change to make bug 416868 independent from bug 415830.
Attachment #303629 -
Attachment is obsolete: true
Attachment #303634 -
Flags: review+
Comment 18•16 years ago
|
||
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•16 years ago
|
Attachment #303634 -
Flags: superreview?(roc)
Assignee | ||
Comment 19•16 years ago
|
||
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+
Updated•16 years ago
|
Attachment #305972 -
Flags: approval1.9?
Comment 20•16 years ago
|
||
Comment on attachment 305972 [details] [diff] [review] patch v3.5 a1.9=beltzner
Attachment #305972 -
Flags: approval1.9? → approval1.9+
Updated•16 years ago
|
Keywords: checkin-needed
Comment 21•16 years ago
|
||
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.
Description
•