After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 768679 - [PATCH] Fix win32_render_layout_line to draw colors
[PATCH] Fix win32_render_layout_line to draw colors
Status: RESOLVED FIXED
Product: pango
Classification: Platform
Component: win32
unspecified
Other Windows
: Normal enhancement
: ---
Assigned To: gtk-win32 maintainers
pango-maint
Depends on:
Blocks:
 
 
Reported: 2016-07-11 13:08 UTC by Nicolas Hake
Modified: 2017-04-08 01:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
proposed patchset (10.43 KB, patch)
2016-07-11 13:08 UTC, Nicolas Hake
none Details | Review
[1/6] enable rendering colored text (1.80 KB, patch)
2016-07-11 14:08 UTC, Nicolas Hake
committed Details | Review
[2/6] enable colored underline drawing (3.38 KB, patch)
2016-07-11 14:09 UTC, Nicolas Hake
committed Details | Review
[3/6] reset BkMode so ExtTextOutW doesn't draw its own background (1.09 KB, patch)
2016-07-11 14:09 UTC, Nicolas Hake
committed Details | Review
[4/6] draw background box in correct color (1.77 KB, patch)
2016-07-11 14:10 UTC, Nicolas Hake
committed Details | Review
[5/6] draw background box in correct position (1.14 KB, patch)
2016-07-11 14:11 UTC, Nicolas Hake
committed Details | Review
[6/6] update docs to remove "uncolored" (1.25 KB, patch)
2016-07-11 14:11 UTC, Nicolas Hake
committed Details | Review
Side-by-side comparison of old and new behavior (6.06 KB, image/png)
2016-09-10 15:21 UTC, Nicolas Hake
  Details
Source producing attachment 335254 (2.04 KB, text/plain)
2016-09-19 17:39 UTC, Nicolas Hake
  Details

Description Nicolas Hake 2016-07-11 13:08:39 UTC
Created attachment 331218 [details] [review]
proposed patchset

The docs for win32_render_layout and win32_render_layout_line explicitly state that the DC they're passed is used for "uncolored" drawing. However, the code clearly attempts to support colors; it's just doing so incorrectly. This is a series of patches that fix drawing in pangowin32 so at least the most common cases of text color, background color, and underline color are supported. (Alpha transparency isn't.)
Comment 1 Ignacio Casal Quinteiro (nacho) 2016-07-11 13:15:22 UTC
Review of attachment 331218 [details] [review]:

Can you please provide one attachment per format patch? Thanks
Comment 2 Nicolas Hake 2016-07-11 14:08:28 UTC
Created attachment 331219 [details] [review]
[1/6] enable rendering colored text
Comment 3 Nicolas Hake 2016-07-11 14:09:06 UTC
Created attachment 331220 [details] [review]
[2/6] enable colored underline drawing
Comment 4 Nicolas Hake 2016-07-11 14:09:55 UTC
Created attachment 331221 [details] [review]
[3/6] reset BkMode so ExtTextOutW doesn't draw its own background
Comment 5 Nicolas Hake 2016-07-11 14:10:30 UTC
Created attachment 331222 [details] [review]
[4/6] draw background box in correct color
Comment 6 Nicolas Hake 2016-07-11 14:11:05 UTC
Created attachment 331223 [details] [review]
[5/6] draw background box in correct position
Comment 7 Nicolas Hake 2016-07-11 14:11:55 UTC
Created attachment 331225 [details] [review]
[6/6] update docs to remove "uncolored"
Comment 8 Nicolas Hake 2016-09-10 15:21:29 UTC
Created attachment 335254 [details]
Side-by-side comparison of old and new behavior

Added a side-by-side comparison screenshot to illustrate why these patches should be merged. The code drawing the window is the same in both cases; the only difference is the used pangowin32.dll. Left window is drawn to by vanilla pango 1.40.2.

Ultimately it would likely be preferable to turn the win32 backend into a subclass of PangoRenderer to avoid having to implement all of its functionality separately. This is also something I've considered, but it seemed to be more reasonable for now to just fix the incorrect WinAPI calls.
Comment 9 Ignacio Casal Quinteiro (nacho) 2016-09-10 15:27:27 UTC
The patches look good to me. I would like an opinion from LRN or Fan though.
Comment 10 LRN 2016-09-10 15:39:57 UTC
attachment 331219 [details] [review] - looks ok
attachment 331220 [details] [review] - looks ok
attachment 331221 [details] [review] - looks ok
attachment 331222 [details] [review] - looks ok
attachment 331223 [details] [review] - looks ok
attachment 331225 [details] [review] - looks ok

I should note that my "looks ok" isn't worth much, as i never did use Windows GDI drawing myself. But commit messages in these patches are meaningful, and the changes seem to be matching them.
Comment 11 Fan, Chun-wei 2016-09-19 08:54:01 UTC
Hi Nicolas,

I need to say that I am on the same stateof this as LRN is regarding GDI.  Any chance that you have the source file that reflects the image you have in comment #8?

With blessings, thank you!
Comment 12 Nicolas Hake 2016-09-19 17:39:37 UTC
Created attachment 335873 [details]
Source producing attachment 335254 [details]

Sure. I've built the test application with MSVC, but I don't think it's using any nonstandard extensions, so gcc should work fine too.
Comment 13 Nicolas Hake 2016-11-28 09:59:21 UTC
Do you require any additional information or code changes to move forward with this patch series?
Comment 14 Behdad Esfahbod 2016-11-29 21:29:02 UTC
This looks good to go in, if someone can commit please.
Comment 15 Matthias Clasen 2016-11-30 15:24:45 UTC
if you mark it as a-c-n, I'll pick it up in an occasional sweep
Comment 16 Behdad Esfahbod 2016-12-01 01:33:26 UTC
Review of attachment 331219 [details] [review]:

lgtm
Comment 17 Behdad Esfahbod 2016-12-01 01:33:43 UTC
Review of attachment 331220 [details] [review]:

lgtm
Comment 18 Behdad Esfahbod 2016-12-01 01:33:54 UTC
Review of attachment 331221 [details] [review]:

lgtm
Comment 19 Behdad Esfahbod 2016-12-01 01:34:00 UTC
Review of attachment 331222 [details] [review]:

lgtm
Comment 20 Behdad Esfahbod 2016-12-01 01:34:06 UTC
Review of attachment 331223 [details] [review]:

lgtm
Comment 21 Behdad Esfahbod 2016-12-01 01:34:52 UTC
Review of attachment 331225 [details] [review]:

lgtm
Comment 22 Behdad Esfahbod 2016-12-01 01:35:13 UTC
Review of attachment 331225 [details] [review]:

lgtm