Closed Bug 408620 Opened 12 years ago Closed 12 years ago

Native theming should set the correct orientation on underlying gtk objects before drawing

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: frnchfrgg, Assigned: frnchfrgg)

Details

Attachments

(3 files, 8 obsolete files)

4.81 KB, patch
twanno
: review+
mtschrep
: approval1.9+
Details | Diff | Splinter Review
23.38 KB, patch
twanno
: review+
mtschrep
: approval1.9+
Details | Diff | Splinter Review
33.63 KB, patch
twanno
: review+
mtschrep
: approval1.9+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.8.1.8) Gecko/20071004 Iceweasel/2.0.0.8 (Debian-2.0.0.8-1)
Build Identifier: 

Some themes draw things differently depending on the orientation (LTR, RTL); for example a tabpanel could have a rounded top-right corner on LTR while it would be the top-left on RTL. The same could be true of every widget, so we should set the correct orientation on the underlying gtk widgets before drawing.

This is possible due to work on bug 316748.

Reproducible: Always
Status: NEW → ASSIGNED
Assignee: nobody → frnchfrgg-mozbugs
Status: ASSIGNED → NEW
Attachment #293529 - Flags: review?(roc)
I need advice for the moz_gtk_container_paint() function (around line 1300), where I think there's a bug; the object used in the call to GDK is always gCheckBoxWidget, even when it is a radio. At least, if this is the correct way to do, ensure_checkbox_widget should always be called... I didn't do anything in this function for now.

The rest is complete, compiles, and runs correctly, though I didn't find a theme taking the direction into account. Teune, what themes did you encounter that did ?
>     if (!gToolbarSeparatorWidget) {
>         ensure_toolbar_widget();
>         gToolbarSeparatorWidget = gtk_separator_tool_item_new();
>         setup_widget_prototype(gToolbarSeparatorWidget);
>     }
>+    return MOZ_GTK_SUCCESS;
> }

While you're at it: gtk_separator_tool_item_new() does not return a GtkWidget, maybe this should be changed to GTK_WIDGET(gtk_separator_tool_item_new()) ?

(In reply to comment #5)
> I need advice for the moz_gtk_container_paint() function (around line 1300),
> where I think there's a bug; the object used in the call to GDK is always
> gCheckBoxWidget, even when it is a radio. At least, if this is the correct way
> to do, ensure_checkbox_widget should always be called... I didn't do anything
> in this function for now.

You can change it to do the same as moz_gtk_toggle_label_paint a few lines down.

> 
> The rest is complete, compiles, and runs correctly, though I didn't find a
> theme taking the direction into account. Teune, what themes did you encounter
> that did ?
> 

I know that most themes included with gnome by default (e.g. Clearlooks, Crux, High Contrast theme) take direction into account for at least the option menu widgets. Clearlooks does also do that for tab panels, but most widgets are drawn in such a way that it doesn't matter whether the direction is RTL or LTR.
>While you're at it: gtk_separator_tool_item_new() does not return a GtkWidget,
>maybe this should be changed to GTK_WIDGET(gtk_separator_tool_item_new()) ?

Yes, I saw the warning, and decided to not do anything as I didn't really know how to fix it. By the way, do you prefer one big patch (on git I prefer to do a lot of small commits) ?

> Clearlooks does also do that for tab panels

I have clearlooks and didn't notice anything...
Cast the toolbar separator to GtkWidget to avoid a warning.
Fix missing return value in ensure_toolbar_separator_widget().
Correct moz_gtk_container_paint() use of gtk widgets.
Replace C++ comment with C comment.
Attachment #293528 - Attachment is obsolete: true
Attachment #293529 - Attachment is obsolete: true
Attachment #293576 - Flags: superreview?(roc)
Attachment #293576 - Flags: review?(twanno)
Attachment #293528 - Flags: review?(roc)
Attachment #293529 - Flags: review?(roc)
To apply on top of [1/3]
Attachment #293530 - Attachment is obsolete: true
Attachment #293577 - Flags: superreview?(roc)
Attachment #293577 - Flags: review?(twanno)
Attachment #293530 - Flags: review?(roc)
To apply on top of [2/3].

Note: Since each patch compiles, they may be committed separately at your will.
Attachment #293534 - Attachment is obsolete: true
Attachment #293578 - Flags: superreview?(roc)
Attachment #293578 - Flags: review?(twanno)
Attachment #293534 - Flags: review?(roc)
The previous version had a missing "style = widget->style" line which would probably make FF crash. This is the correct version.
Attachment #293576 - Attachment is obsolete: true
Attachment #293593 - Flags: superreview?(roc)
Attachment #293593 - Flags: review?(twanno)
Attachment #293576 - Flags: superreview?(roc)
Attachment #293576 - Flags: review?(twanno)
Comment on attachment 293593 [details] [diff] [review]
[1/3] Correct coding glitches in gtk2drawing.c, NON BROKEN VERSION

since you ask me to review, which I believe I am actually not qualified to do:
>     if (isradio) {
>         ensure_radiobutton_widget();
>-        style = gRadiobuttonWidget->style;  
>-        moz_gtk_widget_get_focus(gRadiobuttonWidget, &interior_focus, &focus_width, &focus_pad);
>+        widget = gRadiobuttonWidget;
>+
One nit: this extra line is not needed

After this patch, I think the following code in moz_gtk_tooltip_paint would be causing the only remaining warning: 
    gtk_style_attach(style, gTooltipWidget->window);
If you feel like fixing that in this patch, the code should be:
    style = gtk_style_attach(style, gTooltipWidget->window);
(In reply to comment #12)
> (From update of attachment 293593 [details] [diff] [review])
> since you ask me to review, which I believe I am actually not qualified to do:

[04:32:56PM] <_FrnchFrgg_> roc: Should I request review from Teune or from you ?
[04:33:28PM] <roc> _FrnchFrgg_: r from teune, sr from me

:)
Status: NEW → ASSIGNED
Teune, you've been working on this code a lot lately, you're as qualified as anyone we have around. I'll review it too, don't worry :-).
Address Teune's comments.
Attachment #293593 - Attachment is obsolete: true
Attachment #293644 - Flags: superreview?(roc)
Attachment #293644 - Flags: review?(twanno)
Attachment #293593 - Flags: superreview?(roc)
Attachment #293593 - Flags: review?(twanno)
Comment on attachment 293578 [details] [diff] [review]
[3/3] Use new direction parameter to set the direction of underlying widget before painting

> 
>+    gtk_widget_set_direction(scrollbar, direction);
>+
>     /* Some theme engines (i.e., ClearLooks) check the scrollbar's allocation
 [...]
> 
>+    gtk_widget_set_direction(scrollbar, direction);
>+
>     style = GTK_WIDGET(scrollbar)->style;
 [...]
> 
>+    gtk_widget_set_direction(scrollbar, direction);
>+
>     /* Make sure to set the scrollbar range before painting so that

In those three cases it should be: gtk_widget_set_direction(GTK_WIDGET(scrollbar), direction).

>     GtkStyle* style;
>     GtkStateType state_type = ConvertGtkState(state);
> 
>     ensure_window_widget();
>+    gtk_widget_set_direction(gProtoWindow, direction);
>+
>     style = gProtoWindow->style;
> 
>     TSOffsetStyleGCs(style, rect->x, rect->y);
> 
>     gtk_paint_resize_grip(style, drawable, state_type, cliprect, gProtoWindow,
>                           NULL, GDK_WINDOW_EDGE_SOUTH_EAST, rect->x, rect->y,
>                           rect->width, rect->height);

When the direction is RTL the edge should probably be GDK_WINDOW_EDGE_SOUTH_WEST (although the resizer is currently not painted when the direction is RTL) 

And it seems you forgot to add gtk_widget_set_direction in moz_gtk_container_paint
Attachment #293578 - Flags: review?(twanno) → review-
Comment on attachment 293644 [details] [diff] [review]
[1/3] Correct coding glitches in gtk2drawing.c

looks fine
Attachment #293644 - Flags: review?(twanno) → review+
Comment on attachment 293577 [details] [diff] [review]
[2/3] Add direction parameter to all moz_*_paint() functions

>     case MOZ_GTK_RADIOBUTTON_CONTAINER:
>         return moz_gtk_container_paint(drawable, rect, cliprect, state,
>-                                       (widget == MOZ_GTK_RADIOBUTTON_CONTAINER));
>+                                      (widget == MOZ_GTK_RADIOBUTTON_CONTAINER),
>+                                      direction);
small nit: this should be lined up correctly

The rest looks good
Attachment #293577 - Flags: review?(twanno) → review+
I did that because if I align those according to custom, the lines are more than 80 characters long, and having

return moz_gtk_container_paint(drawable, rect, cliprect, state,
                               (widget ==
                                MOZ_GTK_RADIOBUTTON_CONTAINER),
                               direction);
wasn't really satifying. I can change if you want.
Addresses Teune's comment 16
Attachment #293578 - Attachment is obsolete: true
Attachment #293709 - Flags: superreview?(roc)
Attachment #293709 - Flags: review?(twanno)
Attachment #293578 - Flags: superreview?(roc)
(In reply to comment #19)
> I did that because if I align those according to custom, the lines are more
> than 80 characters long, and having
> 
> return moz_gtk_container_paint(drawable, rect, cliprect, state,
>                                (widget ==
>                                 MOZ_GTK_RADIOBUTTON_CONTAINER),
>                                direction);
> wasn't really satifying. I can change if you want.
> 

No, that is indeed not the best solution, but in this case I don't think you should worry about exceeding the 80 character boundary by 1 character.

BTW in line with your patch the following lines would qualify more for adjustment (also 81 characters long):
>+        return moz_gtk_resizer_paint(drawable, rect, cliprect, state, direction);
>+        return moz_gtk_progress_chunk_paint(drawable, rect, cliprect, direction);
(In reply to comment #21)
I wrapped the "return" lines, and corrected the alignment without wrapping for moz_gtk_container paint. I also corrected the same thing at two or three more places.
Have you other comments, or can I post the resulting patch ?
In fact it seems that the previous patch I posted didn't have those 80 chars corrections I now reverted, so in fact you had spotted the only occurence in what I sent.

While searching for overlong lines in gtk2drawing.c I encountered a lot of lines not fitting in 80 columns, without any good reason. I didn't touch them, though a cleanup patch could be done.
(In reply to comment #22)
> Have you other comments, or can I post the resulting patch ?
> 

I have no other comments, so go ahead and post :)

(In reply to comment #23)
> While searching for overlong lines in gtk2drawing.c I encountered a lot of
> lines not fitting in 80 columns, without any good reason. I didn't touch them,
> though a cleanup patch could be done.
> 

I wouldn't do that in this bug. If roc thinks that gtk2drawing.c needs some cleaning up in that respect, another bug should be filed.
Wrapping nits addressed.
Attachment #293577 - Attachment is obsolete: true
Attachment #293728 - Flags: superreview?(roc)
Attachment #293728 - Flags: review?(twanno)
Attachment #293577 - Flags: superreview?(roc)
(In reply to comment #24)
 
> I wouldn't do that in this bug. If roc thinks that gtk2drawing.c needs some
> cleaning up in that respect, another bug should be filed.

Sure, that's what I meant. 

Attachment #293728 - Attachment description: Add direction parameter to all moz_*_paint() functions, V2 → [2/3] Add direction parameter to all moz_*_paint() functions, V2
Attachment #293709 - Flags: review?(twanno) → review+
Comment on attachment 293728 [details] [diff] [review]
[2/3] Add direction parameter to all moz_*_paint() functions, V2

Thanks
Attachment #293728 - Flags: review?(twanno) → review+
Attachment #293644 - Flags: superreview?(roc) → superreview+
Attachment #293709 - Flags: superreview?(roc) → superreview+
Attachment #293728 - Flags: superreview?(roc) → superreview+
Comment on attachment 293644 [details] [diff] [review]
[1/3] Correct coding glitches in gtk2drawing.c

Fix some various code problems.
Attachment #293644 - Flags: approval1.9?
Comment on attachment 293728 [details] [diff] [review]
[2/3] Add direction parameter to all moz_*_paint() functions, V2

Add scaffolding to fix an RTL issue.
Attachment #293728 - Flags: approval1.9?
Comment on attachment 293709 [details] [diff] [review]
[3/3] Use new direction parameter to set the direction of underlying widget before painting, v2

Use added scaffolding to fix RTL issue.
Attachment #293709 - Flags: approval1.9?
Attachment #293728 - Flags: approval1.9? → approval1.9+
Attachment #293644 - Flags: approval1.9? → approval1.9+
Attachment #293709 - Flags: approval1.9? → approval1.9+
Comment on attachment 293644 [details] [diff] [review]
[1/3] Correct coding glitches in gtk2drawing.c

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.54; previous revision: 1.53
done
Comment on attachment 293728 [details] [diff] [review]
[2/3] Add direction parameter to all moz_*_paint() functions, V2

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.55; previous revision: 1.54
done
Comment on attachment 293709 [details] [diff] [review]
[3/3] Use new direction parameter to set the direction of underlying widget before painting, v2

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.56; previous revision: 1.55
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.