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

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Julien "_FrnchFrgg_" RIVAUD]

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

Comment 1

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Fix missing return value in ensure_toolbar_separator

Comment 2

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Replace C++ comment with C comment in gtk2drawing.c

Comment 3

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Add direction parameter to all moz_*_paint() functions

Comment 4

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: [INCOMPLETE] Use new direction parameter to set the direction of underlying widget before painting

Comment 5

[Julien "_FrnchFrgg_" RIVAUD]

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 ?

Comment 6

[Teune van Steeg]

>     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.

Comment 7

[Julien "_FrnchFrgg_" RIVAUD]

>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...

Comment 8

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: [1/3] Correct coding glitches in gtk2drawing.c

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.

Comment 9

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: [2/3] Add direction parameter to all moz_*_paint() functions

To apply on top of [1/3]

Comment 10

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: [3/3] Use new direction parameter to set the direction of underlying widget before painting

To apply on top of [2/3].

Note: Since each patch compiles, they may be committed separately at your will.

Comment 11

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: [1/3] Correct coding glitches in gtk2drawing.c, NON BROKEN VERSION

The previous version had a missing "style = widget->style" line which would probably make FF crash. This is the correct version.

Comment 12

[Teune van Steeg]

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);

Comment 13

[Reed Loden [:reed]]

(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

:)

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-).

Comment 15

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: [1/3] Correct coding glitches in gtk2drawing.c

Address Teune's comments.

Comment 16

[Teune van Steeg]

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

Comment 17

[Teune van Steeg]

Comment on attachment 293644 [details] [diff] [review]
[1/3] Correct coding glitches in gtk2drawing.c

looks fine

Comment 18

[Teune van Steeg]

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

Comment 19

[Julien "_FrnchFrgg_" RIVAUD]

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.

Comment 20

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: [3/3] Use new direction parameter to set the direction of underlying widget before painting, v2

Addresses Teune's comment 16

Comment 21

[Teune van Steeg]

(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);

Comment 22

[Julien "_FrnchFrgg_" RIVAUD]

(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 ?

Comment 23

[Julien "_FrnchFrgg_" RIVAUD]

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.

Comment 24

[Teune van Steeg]

(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.

Comment 25

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: [2/3] Add direction parameter to all moz_*_paint() functions, V2

Wrapping nits addressed.

Comment 26

[Julien "_FrnchFrgg_" RIVAUD]

(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. 

Comment 27

[Teune van Steeg]

Comment on attachment 293728 [details] [diff] [review]
[2/3] Add direction parameter to all moz_*_paint() functions, V2

Thanks

Comment 28

[Reed Loden [:reed]]

Comment on attachment 293644 [details] [diff] [review]
[1/3] Correct coding glitches in gtk2drawing.c

Fix some various code problems.

Comment 29

[Reed Loden [:reed]]

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.

Comment 30

[Reed Loden [:reed]]

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.

Comment 31

[Reed Loden [:reed]]

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 32

[Reed Loden [:reed]]

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 33

[Reed Loden [:reed]]

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