From konsole-devel Sat Dec 03 17:51:14 2016 From: Kurt Hindenburg Date: Sat, 03 Dec 2016 17:51:14 +0000 To: konsole-devel Subject: Re: Review Request 129281: [Konsole] Render text at primary font's baseline Message-Id: <20161203175114.23280.69522 () mimi ! kde ! org> X-MARC-Message: https://marc.info/?l=konsole-devel&m=148078748213135 --===============5541804976526750667== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Nov. 9, 2016, 12:22 p.m., Martin Tobias Holmedahl Sandsmark wrote: > > looks good to me, but I think hindenburg should ack it (might want to add him to reviewers) > > Christoph Feck wrote: > Pretty sure "konsole" reviewers group includes the maintainer, but I would actually prefer feedback from someone using non-Western characters in Konsole. > > Christoph Feck wrote: > Ping? Should I just commit it to 16.12 branch so that people can give feedback? I'd rather not use 16.12 to test at this late time - how about master? - Kurt ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129281/#review100745 ----------------------------------------------------------- On Nov. 6, 2016, 8:40 p.m., Christoph Feck wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129281/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2016, 8:40 p.m.) > > > Review request for KDE Frameworks and Konsole. > > > Bugs: 371687 > http://bugs.kde.org/show_bug.cgi?id=371687 > > > Repository: konsole > > > Description > ------- > > When Konsole draws a line of text, it first computes the rectangle of the line that the text covers (taking into account cells width and height), then passes this rectangle to the drawText(QRect, flags, text) call. > > Qt detects if the selected font does not offer all characters in the text, and substitutes individual characters with a different font. Due to designer choices, the same font point size does not lead to same pixel height (or ascent size) in all fonts, so the substituted characters might be larger than the characters from the primary font. > > Using a rectangle causes Qt to position glyphs relative to the bounding box of the text, instead of anchored to the baseline. > > This patch uses a pixel position instead of a rectangle to draw the text, taking into account only the baseline of the primary font. > > I have added all "frameworks" developers to increase possible test coverage. > > > Diffs > ----- > > src/TerminalDisplay.cpp 39a8b84 > > Diff: https://git.reviewboard.kde.org/r/129281/diff/ > > > Testing > ------- > > On my system, lines with substituted Unicode characters are no longer shifted away from the baseline, and therefore do not appear cropped. > > Further testing is needed, as there are many (equivalent, similar, or different) bug reports about font rendering on different systems, see https://bugs.kde.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=CONFIRMED&component=font&product=konsole > > > Thanks, > > Christoph Feck > > --===============5541804976526750667== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit

This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129281/

On November 9th, 2016, 12:22 p.m. UTC, Martin Tobias Holmedahl Sandsmark wrote:
looks good to me, but I think hindenburg should ack it (might want to add him to reviewers)
On November 9th, 2016, 3:09 p.m. UTC, Christoph Feck wrote:
Pretty sure "konsole" reviewers group includes the maintainer, but I would actually prefer feedback from someone using non-Western characters in Konsole.
On November 30th, 2016, 11:18 p.m. UTC, Christoph Feck wrote:
Ping? Should I just commit it to 16.12 branch so that people can give feedback?

I'd rather not use 16.12 to test at this late time - how about master?

- Kurt

On November 6th, 2016, 8:40 p.m. UTC, Christoph Feck wrote:

Review request for KDE Frameworks and Konsole.

By Christoph Feck.

Updated Nov. 6, 2016, 8:40 p.m.

Bugs: 371687

Repository: konsole

Description

When Konsole draws a line of text, it first computes the rectangle of the line that the text covers (taking into account cells width and height), then passes this rectangle to the drawText(QRect, flags, text) call.

Qt detects if the selected font does not offer all characters in the text, and substitutes individual characters with a different font. Due to designer choices, the same font point size does not lead to same pixel height (or ascent size) in all fonts, so the substituted characters might be larger than the characters from the primary font.

Using a rectangle causes Qt to position glyphs relative to the bounding box of the text, instead of anchored to the baseline.

This patch uses a pixel position instead of a rectangle to draw the text, taking into account only the baseline of the primary font.

I have added all "frameworks" developers to increase possible test coverage.

Testing

On my system, lines with substituted Unicode characters are no longer shifted away from the baseline, and therefore do not appear cropped.

Further testing is needed, as there are many (equivalent, similar, or different) bug reports about font rendering on different systems, see https://bugs.kde.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=CONFIRMED&component=font&product=konsole

Diffs

src/TerminalDisplay.cpp (39a8b84)

--===============5541804976526750667==--