GNOME Bugzilla – Bug 691678
notebook tabs are drawn incorrectly using win32 theme
Last modified: 2018-04-15 00:00:26 UTC
When gtk_notebook_set_tab_pos is used to position tabs at the left, right, or bottom of the notebook, the tabs are drawn incorrectly. Under GTK 2 the tabs were drawn correctly. The GTK 2 draw_themed_tab_button function appears to handle this by checking the tab position, then drawing the tab on a pixmap and rotating or flipping the resulting pixmap so that the tab is drawn correctly on the screen.
Created attachment 239015 [details] [review] Fix notebook tab position on Windows
I tested this patch against an older version of GTK+ on Windows XP using the default blue and green theme. I haven't figured out how to build from git on Windows so I haven't tested it against the latest version of GTK+. It applies cleanly though, so I assume it'll work ;)
Tested and working against 3.6.4 ; default themes on WinXP/7.
Created attachment 239925 [details] [review] Fix copy-paste error in previous patch Oops, I just noticed that I forgot to change the command name in the parser error messages.
Created attachment 239927 [details] [review] Fix copy-paste error in previous patch (the previous upload seems to have failed.. sorry for the extra email)
(In reply to comment #5) > Created an attachment (id=239927) [details] [review] > Fix copy-paste error in previous patch > > (the previous upload seems to have failed.. sorry for the extra email) Neat, thank you very much ! Retested OK.
Review of attachment 239927 [details] [review]: I'm no GTK developer myself, but here is a try: ::: gtk/gtkcssimagewin32.c @@ +67,3 @@ + if (wimage->rotate == 1) + cairo_translate (cr, height, 0); + if (wimage->rotate == 2) Shouldn't there be "else if" instead of just "if"? @@ +69,3 @@ + if (wimage->rotate == 2) + cairo_translate (cr, width, height); + if (wimage->rotate == 3) Shouldn't there be "else if" instead of just "if"? @@ +71,3 @@ + if (wimage->rotate == 3) + cairo_translate (cr, 0, width); + if (wimage->rotate) Here you will rotate even for values > 3, but with no translation. So rotate(4) will work, but not rotate(5). Either limit to the [0;3] range and use enums for readability (prefered, IMHO), or allow any value and use the modulo operator for your checks (which I'm not sure would be a good idea, as it would prevent you from keeping value for future use). @@ +217,3 @@ + if (!_gtk_css_parser_try_int (parser, &wimage->rotate)) + { + _gtk_css_parser_error (parser, "Expected a valid integer value"); Maybe you could check the values are in the [0;3] range here? I don't know if there's a strict or more liberal policy for values checks, though. You could either ignore an out-of-range value, or report it as an error.
Created attachment 263956 [details] [review] Revised version with "else if" and check [0;3] Patch is great, and works on all branches starting from 3.6. What do you think of the attached revised version ? Following Luis' suggestion, it now checks if the supplied value is in [0;3] interval, and returns a warning if not. As we are now sure the value is correct, we can call cairo_rotate() unconditionally.
I saw in your email to gtk-devel-list that there were some issues with the patch on >= 3.6, are those resolved now? That patch looks fine, I think I would leave the condition in for cairo_rotate though. If 'rotate == 0' then the rotation will have no effect so it's not really needed.
Created attachment 263975 [details] [review] Revised version with "else if" and check [0;3] Yes, was my fault b.t.w, had some mess in the build environment, so it didn't apply property. Agreed ; what do you think of this new version which checks if value != 0 to do the cairo_rotate() ?
Review of attachment 263975 [details] [review]: ::: gtk/gtk-win32-base.css @@ +72,3 @@ + cairo_translate (cr, 0, width); + + if (wimage->rotate != 0) I'd go with: if (winimage->rotate > 0 && winimage->rotate < 4) Otherwise, out-of-bound values would be rotated but not translated... This way out-of-bound values are just ignored.
Are you sure this bug is win32-only? I see no rotation either in the default css theme... I've also seen that css3 has rotations using transform:rotate(20deg). As Benjamin's goal is to be as close as possible to the CSS spec, using "rotate" with something that isn't an angle may not be appropriate. Would be nice if a GTK dev could confirm the way to go...
I believe tarnyko's patch checks the out-of-bounds case earlier on during parsing. I suppose you can check it again if you want to make it more clear, but the `winimage->rotate < 4' case should have no effect. I think using a non-angle here is actually preferable. If this were a general purpose "rotate" function, then I agree, using an angle would be better. But since this is really "transpose-draw-rotate", non 90 degree angles don't really have meaning. It's more like which edge you want to draw the tab decorations on.
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla. If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab: https://gitlab.gnome.org/GNOME/gtk/issues/new