Improve built-in line/block characters drawing
Needs ReviewPublic

Authored by mglb on Mon, Feb 4, 8:05 PM.

Details

Reviewers
fvogt
Group Reviewers
Konsole
VDG
Summary
  • Fix bold lines (BUG 402415).
  • Make drawing pixel-perfect.
  • Make line width proportional to font size.
  • Move relevant code to separate file and namespace.
  • Remove code for checking supported line characters from Character class. Information about what is supported is now in one place together width drawing code.
  • Remove fontembedder/LineFont files (no longer used).
  • Add test script for displaying supported characters table.
  • Add triple and quadruple dashes (U+2504...U+250B).
  • Change shade block characters (U+2591...U+2593) look. When antialiasing is turned on, shades are drawn as transculent solid rectangles with 25%, 50% and 75% alpha. This matches the characters name/description and their usage. Without antialiasing, previous method with patterns is used.

Screenshots

Font size: 10pt; character width: 8px

Font size: 11pt; character width: 9px

Font size: 12pt; character width: 10px

Font size: 13-14pt; character width: 11px; w/o antialiasing

Font size: 13-14pt; character width: 11px

Font size: 15pt; character width: 12px

Font size: 6-7pt; character width: 5px

Font size: 8-9pt; character width: 7px; w/o antialiasing

Font size: 8-9pt; character width: 7px

Alignment test (8pt)

Note: Copyrights in LineBlockCharactersDrawer.cpp are based on
git blame -w src/TerminalDisplay.cpp executed before moving the code
to a separate file. Years from first/last commit. Authors sorted by
year. Whitespace-only changes were ignored. Maksim's code was commited
by Waldo Bastian who mentioned him as the author in commit message
(see 5062b40dd).

BUG: 402415

Test Plan

Common steps for all tests

  • Open Edit Current Profile → Appearance.
  • Turn on Draw intense colors in bold font.
  • Turn off Use line characters contained in font.
  • (Optional) select a font which is able to display bold characters in Konsole (e.g. DejaVu Sans Mono).

Check characters validity

  • Run ./tests/line_block_characters_table.py.
  • Open Edit Current Profile → Appearance.
  • By switching Use line characters contained in font on and off, compare built-in characters drawing with characters from a font. General shape and line directions must be the same. Small offsets, line width differences (as long as proportions between lines in a character are kept), and quality differences are allowed.

Review overall quality

  • Run ./tests/line_block_characters_table.py.
  • Review glyphs quality in different font sizes.
  • Open Edit Current Profile → Appearance.
  • Toggle Smooth fonts, review quality again.

Check alignment

  • Display tests/UTF-8-demo.txt
  • At the bottom of the file you can find a few alignment images. Check if all lines align properly. If you're unsure how it should look, compare it with font characters by turning on Use line characters contained in font option.

Diff Detail

Repository
R319 Konsole
Branch
arc/line-block-characters-drawing
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8231
Build 8249: arc lint + arc unit
mglb requested review of this revision.Mon, Feb 4, 8:05 PM
mglb created this revision.
fvogt requested changes to this revision.Wed, Feb 6, 7:18 PM
fvogt added a subscriber: fvogt.
fvogt added inline comments.
src/LineBlockCharacters.cpp
172

Stray ';' - results in compiler warning.

194

For fontWidth less than 8 this returns 1 for both bold and normal lines. Intentional?

IMO increasing the LightWidthToFontSizeRatio to 1/5 is acceptable.

src/LineBlockCharacters.h
63

AFAICT only this variant is actually used - so is the other one necessary at all?

This revision now requires changes to proceed.Wed, Feb 6, 7:18 PM

thanks for doing this, been dreading doing something like it myself. :-)

looks good in general to me.

src/LineBlockCharacters.cpp
167

rotl is a pretty normal acronym, but probably more readable if it's called rotateBitsLeft or something?

249

lightPath, heavyPath, maybe?

274

topIndex?

283

not a fan of two-letter abbreviated variable names (against the Qt code style fwiw).

450

maybe a comment to show which is which? in case of future debugging.

src/TerminalDisplay.cpp
783

no idea what these types are, isn't the first one just a bool? auto feels unnecessary (and I'm allergic to auto).

mglb updated this revision to Diff 51233.Sat, Feb 9, 3:15 AM
  • Resolve problems
  • Some additional improvements
mglb updated this revision to Diff 51234.Sat, Feb 9, 3:21 AM
mglb retitled this revision from Improve built-in line/box characters drawing to Add line/block characters table for testing.
mglb edited the summary of this revision. (Show Details)

Update description (I hope so)

mglb retitled this revision from Add line/block characters table for testing to Improve built-in line/box characters drawing.Sat, Feb 9, 3:23 AM
mglb edited the summary of this revision. (Show Details)
mglb marked 3 inline comments as done.Sat, Feb 9, 4:02 AM
mglb added inline comments.
src/LineBlockCharacters.cpp
194

It now returns 1 for normal and bold lines when the fontWidth is less than 7. It is just not possible to fit bold characters in such narrow space.

1/5 turned out to be too low. 1/6.5 works best. Minimum bold line width
is now set to normal line width + 1, so it is always bolder for character widths >= 7.

mglb marked an inline comment as done.Sat, Feb 9, 7:37 AM
mglb added a comment.Sat, Feb 9, 7:48 AM

@sandsmark I'll take care of your remarks (probably) today

fvogt resigned from this revision.Sat, Feb 9, 10:31 AM

I can confirm that it fixes all border display issues that I encountered in the released version.

mglb updated this revision to Diff 51295.Sun, Feb 10, 12:14 AM
mglb retitled this revision from Improve built-in line/box characters drawing to Add line/block characters table for testing.
mglb edited the summary of this revision. (Show Details)
mglb edited the test plan for this revision. (Show Details)

Fix suggested problems

mglb updated this revision to Diff 51296.Sun, Feb 10, 12:15 AM
mglb retitled this revision from Add line/block characters table for testing to Improve built-in line/block characters drawing.
mglb edited the summary of this revision. (Show Details)

Update description

mglb edited the summary of this revision. (Show Details)Sun, Feb 10, 12:16 AM
mglb updated this revision to Diff 51297.Sun, Feb 10, 12:30 AM

autobool

mglb updated this revision to Diff 51299.Sun, Feb 10, 12:54 AM
mglb marked 7 inline comments as done.

More more descriptive names

mglb added inline comments.Sun, Feb 10, 12:54 AM
src/LineBlockCharacters.cpp
450

I've added comments with respective characters and swapped 3rd and 4th field, so the character name/description is almost directly seen in first three fields

src/TerminalDisplay.cpp
783

The thing to do here is to store some values using getters, and restore them later using respective setters. const auto is nice in such cases - types are not important, as the variables should not be touched. However, there were no const, so this was not so obvious.
Anyway, changed first one to bool.

ngraham edited the summary of this revision. (Show Details)Sun, Feb 10, 6:43 PM
hindenburg added inline comments.
src/TerminalDisplay.cpp
1

Accidentally deleted?

mglb updated this revision to Diff 51559.Tue, Feb 12, 10:39 PM

Restore/fix copyrights

mglb marked an inline comment as done.Tue, Feb 12, 10:40 PM

Thanks for working on this - I think perhaps this should be committed to master after the 19.04 is branched next month. Objections?

fvogt added a comment.EditedSun, Feb 17, 9:13 PM

Thanks for working on this - I think perhaps this should be committed to master after the 19.04 is branched next month. Objections?

Speaking as user and downstream packager - this is a fairly important fix for a regression which got introduced in a bugfix release.
It really makes sense to add this to the next possible release.