Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Christoph Feck]

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

(Updated Dec. 7, 2016, 7:18 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Konsole.


Changes
---

Submitted with commit 7e2f9d839f5af447a0fc7fd178dd5d8f58e489bb by Christoph 
Feck to branch master.


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_status=CONFIRMED=font=konsole


Thanks,

Christoph Feck

Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Christoph Feck]

> On Nov. 9, 2016, 1: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?
> 
> Kurt Hindenburg wrote:
> I'd rather not use 16.12 to test at this late time - how about master?

okey, will commit it to master.


- Christoph


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


On Nov. 6, 2016, 9: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, 9: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_status=CONFIRMED=font=konsole
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Kurt Hindenburg]

> 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_status=CONFIRMED=font=konsole
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Christoph Feck]

> On Nov. 9, 2016, 1: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.

Ping? Should I just commit it to 16.12 branch so that people can give feedback?


- Christoph


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


On Nov. 6, 2016, 9: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, 9: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_status=CONFIRMED=font=konsole
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Christoph Feck]

> On Nov. 9, 2016, 1: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)

Pretty sure "konsole" reviewers group includes the maintainer, but I would 
actually prefer feedback from someone using non-Western characters in Konsole.


- Christoph


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


On Nov. 6, 2016, 9: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, 9: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_status=CONFIRMED=font=konsole
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Martin Tobias Holmedahl Sandsmark]

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



looks good to me, but I think hindenburg should ack it (might want to add him 
to reviewers)

- Martin Tobias Holmedahl Sandsmark


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_status=CONFIRMED=font=konsole
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Christoph Feck]

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

(Updated Nov. 6, 2016, 9:40 p.m.)


Review request for KDE Frameworks and Konsole.


Changes
---

Use 'if' again (instead of ternary operator).


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

  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_status=CONFIRMED=font=konsole


Thanks,

Christoph Feck

Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Christoph Feck]

> On Nov. 6, 2016, 2:35 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> > src/TerminalDisplay.cpp, line 859
> > 
> >
> > Minor nitpick: is there any reason not to unconditionally add the 
> > LTR_OVERRIDE_CHAR?
> > 
> > I don't like the ? : stuff, and the line gets overly long.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
> otherwise it looks good, so +1 from me.

You are right about the ternary operator, I will change it in a minute.

Regarding the LTR stuff, apparently there are console applications (editors?) 
that actually understand RTL languages, and know how to reverse text layout 
(Pango/Harfbuzz-style). For those applications, we cannot use Qt's bidi 
processing (i.e. reversing), but have to render them in the order they are 
written to the console stream.

Since there seems to be no protocol to detect such applications, all Konsole 
can do is to offer a checkbox in its advanced settings.


- Christoph


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


On Oct. 29, 2016, 2:27 a.m., Christoph Feck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129281/
> ---
> 
> (Updated Oct. 29, 2016, 2:27 a.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_status=CONFIRMED=font=konsole
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Martin Tobias Holmedahl Sandsmark]

> On Nov. 6, 2016, 1:35 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> > src/TerminalDisplay.cpp, line 859
> > 
> >
> > Minor nitpick: is there any reason not to unconditionally add the 
> > LTR_OVERRIDE_CHAR?
> > 
> > I don't like the ? : stuff, and the line gets overly long.

otherwise it looks good, so +1 from me.


- Martin Tobias Holmedahl


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


On Oct. 29, 2016, 12:27 a.m., Christoph Feck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129281/
> ---
> 
> (Updated Oct. 29, 2016, 12:27 a.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_status=CONFIRMED=font=konsole
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

Re: Review Request 129281: [Konsole] Render text at primary font's baseline

[Martin Tobias Holmedahl Sandsmark]

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




src/TerminalDisplay.cpp (line 859)


Minor nitpick: is there any reason not to unconditionally add the 
LTR_OVERRIDE_CHAR?

I don't like the ? : stuff, and the line gets overly long.


- Martin Tobias Holmedahl Sandsmark


On Oct. 29, 2016, 12:27 a.m., Christoph Feck wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129281/
> ---
> 
> (Updated Oct. 29, 2016, 12:27 a.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_status=CONFIRMED=font=konsole
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>