D18735: Improve built-in line/box characters drawing

[Martin Tobias Holmedahl Sandsmark]

sandsmark added a comment.


  thanks for doing this, been dreading doing something like it myself. :-)
  
  looks good in general to me.

INLINE COMMENTS

> LineBlockCharacters.cpp:166
> +template <typename T>
> +inline static T rotl(T value, quint8 amount)
> +{

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

> LineBlockCharacters.cpp:248
> +
> +    QPainterPath ppl; // PainterPath for light lines (Painter Path Light)
> +    QPainterPath pph; // PainterPath for heavy lines (Painter Path Heavy)

lightPath, heavyPath, maybe?

> LineBlockCharacters.cpp:273
> +    // largest packedLineTypes value.
> +    uint iT = 0; // top index
> +    quint8 normalizedPackedLineTypes = packedLineTypes;

topIndex?

> LineBlockCharacters.cpp:282
> +    }
> +    uint iR = (iT+1) % LinesNum; // right index
> +    uint iB = (iT+2) % LinesNum; // bottom index

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

> LineBlockCharacters.cpp:449
> +
> +    switch (code) {
> +    case 0x4C: lp = {2, Horizontal, halfGapH  , lightPen}; break;

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

> TerminalDisplay.cpp:761
> +    auto origClipping = painter.hasClipping();
> +    auto origClipRegion = painter.clipRegion();
> +    painter.setClipRect(rect);

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

REPOSITORY
  R319 Konsole

REVISION DETAIL
  https://phabricator.kde.org/D18735

To: mglb, #konsole, #vdg, fvogt
Cc: sandsmark, fvogt, konsole-devel, maciejn, thsurrel, ngraham, maximilianocuria, hindenburg
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/konsole-devel/attachments/20190208/ad5dc86a/attachment.html>