Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Font Ligature Support :GuiRenderLigatures #727

Merged
merged 5 commits into from
Sep 29, 2020

Conversation

jgehrig
Copy link
Collaborator

@jgehrig jgehrig commented Jul 29, 2020

Issue #166: Font ligatures support request.
Issue #723: Font ligatures support request.
Issue #705: Underline + Undercurl rendering incorrect.

Pull #470: Change printing to add ligature support

First, let me say... Thanks @Nuclearo!
This work would not have happened without your code as reference. The underlying rendering scheme is still being used.

The general approach here is very similar to Pull #470. We update the buffer on a line-by-line basis, and we draw blocks of similarly styled cells together. However, we now use QTextLayout and QGlyphRun instead of drawText. The new API should provide the extra control needed to adjust the spacing of non-monospace fonts and perform ligature detection.

I have tried to structure this code so there is a known-good "old codepath" for :GuiRenderLigatures 0. Keeping the code exactly as-is would require large segments to be copy-pasted, so I have tried to decompose rendering tasks into functions. Each function should have code that is almost identical to segments of the old codepath.

Algorithm:

  1. ShellWidget modifications now update() an entire line.
  2. On a given line, we look at the first Cell's style.
  3. Cell text is appended until a Cell style change is detected.
  4. Text layout is calculated via QTextLayout, resulting in a QGlyphRun.
  5. The QGlyphRun is a list of glyph indices and positions.
  6. Ligature are detected from differences in the glyph index of individual characters vs the glyph run.
  7. The glyph run values are modified where necessary (cursor on glyph, position, etc)
  8. Render the QGlyphRun
  9. Back to Step 3 until the line is fully rendered.

Qt Documentation:
QTextLayout
QGlyphRun

@jgehrig
Copy link
Collaborator Author

jgehrig commented Jul 29, 2020

I am posting this early because I assume users will be interested.

There are code cleanup, update() performance improvements, and benchmarking still left to do. It would also be nice to have some ShellWidget test coverage given such extensive modification.

I may work on integrating this with #690...

The current logic prevents both of these modifiers from being rendered at the
same time. If both are present, both should be rendered.

While here, refactoring for pending font ligature changes.
Several distinct rendering tasks exist within paintEvent, and these should
each be moved to their own function. This will prove useful for adding
ligature rendering support.

Functions:
 - Render Neovim Cursor Text
 - Render Neovim Cursor Background
 - Render Cell Text
 - Render Cell Background
This function checks if two cells have identical style, and is useful for
ligature support where blocks of text must be rendered together.

NOTE: Two cells may render identically, but not be stylistically equivalent.
Differences in default colors vs explicit colors and reverse are treated as
Differences.
@jgehrig jgehrig force-pushed the jg-ligatures branch 2 times, most recently from b38fd78 to 6fab4d2 Compare September 28, 2020 17:22
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2020

Codecov Report

Merging #727 into master will increase coverage by 0.17%.
The diff coverage is 41.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #727      +/-   ##
==========================================
+ Coverage   20.80%   20.98%   +0.17%     
==========================================
  Files          72       73       +1     
  Lines       27900    28100     +200     
==========================================
+ Hits         5804     5896      +92     
- Misses      22096    22204     +108     
Impacted Files Coverage Δ
src/gui/shell.cpp 43.62% <0.00%> (-0.09%) ⬇️
src/gui/shellwidget/cell.h 100.00% <ø> (ø)
src/gui/shellwidget/shellwidget.cpp 52.36% <30.49%> (-13.78%) ⬇️
src/gui/shellwidget/cell.cpp 85.71% <66.66%> (-7.62%) ⬇️
src/gui/shellwidget/shellwidget.h 75.00% <100.00%> (+25.00%) ⬆️
src/gui/shellwidget/test/bench_paint.cpp 100.00% <100.00%> (ø)
src/gui/shellwidget/test/test_cell.cpp 100.00% <100.00%> (ø)
src/gui/shellwidget/shellcontents.cpp 86.40% <0.00%> (+12.00%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a59d6b...e057308. Read the comment docs.

@jgehrig
Copy link
Collaborator Author

jgehrig commented Sep 28, 2020

@equalsraf

I think this one is ready for merge. Let me know if you have any final comments.

The GuiRenderLigatures 0 state should be very safe, it uses the same logic (refactored) as today.

For GuiRenderLigatures 1 I am confident in the basic functionality and not aware of any issues. Time for an initial release.

As a side note, the Clazy Pipeline already caught a subtle Qt issue :)

@equalsraf
Copy link
Owner

Looking glorious, go forth and press the merge button.

As a side note, the Clazy Pipeline already caught a subtle Qt issue :)

ooops :D

Nuclearo and others added 2 commits September 28, 2020 23:35
Adds support for a :GuiRenderLigatures command, which will change the
underlying ShellWidget render logic to paint segments of text with the same
styling. Previously text was rendered character-by-character on the grid,
preventing font ligatures from rendering.

Code re-based from:
equalsraf#470
This work would not have been done without @Nucearo 's efforts:
        equalsraf#470

Uses QTextLayout to render segments of similarly styled cells. QTextLayout
provides greater control over Qt's font-rendering engine via QGlyphRun. With
QGlyphRun, we are able to identify individual characters and their positions.

QGlyphRun should provide a means of correcting the spacing for non-perfect
monospace fonts. It also allows for identification of when a character
sequence is converted to a glyph.
@jgehrig jgehrig merged commit 905e636 into equalsraf:master Sep 29, 2020
@jgehrig jgehrig deleted the jg-ligatures branch September 29, 2020 21:10
@jgehrig jgehrig mentioned this pull request Dec 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants