Change printing to add ligature support #470

Nuclearo · 2018-10-01T08:38:15Z

Solves #166

I've changed the text drawing function to

Give QPainter::drawText strings of text instead of individual characters to make use of Qt's built in ligature support.
Redraw whole lines instead of just changed cells. This makes sure we're drawing correct ligatures as new text is entered.

The new printing flow is similar to Konsole's. It collects new characters with the same font and colors, then draws that chunk all at once.

Testing:
I tested with code using the Fira Code font:, and some sample Chinese and Korean texts to test double width characters and all seemed to render fine.

equalsraf · 2018-10-02T11:21:56Z

src/gui/shellwidget/shellwidget.cpp

@@ -294,7 +285,7 @@ int ShellWidget::put(const QString& text, int row, int column,
 	int cols_changed = m_contents.put(text, row, column, fg, bg, sp,
 				bold, italic, underline, undercurl);
 	if (cols_changed > 0) {
-		QRect rect = absoluteShellRect(row, column, 1, cols_changed);
+		QRect rect = absoluteShellRect(row, 0, 1, m_contents.columns());


So every update turns into a full line update. Any idea on much slower this will make things?

I'm unsure. On the one hand it will make single character changes significantly longer but on the other hand, we're now calling one fillRect and one drawText for what could be entire lines, so in some cases it could make performance faster. I'm not sure how to reliably test it, but am willing to if you have a suggestion.

There are a couple of tests in shellwidgets/test. The ones starting with bench_ should run repeated benchmarks.

For example to run the bench_scroll test you can call

$ ctest -R bench_scroll -VV UpdateCTestConfiguration from :/home/raf/Code/neovim-qt/build/DartConfiguration.tcl UpdateCTestConfiguration from :/home/raf/Code/neovim-qt/build/DartConfiguration.tcl Test project /home/raf/Code/neovim-qt/build Constructing a list of tests Done constructing a list of tests Updating test list for fixtures Added 0 tests to meet fixture requirements Checking test dependency graph... Checking test dependency graph end test 11 Start 11: bench_scroll 11: Test command: /home/raf/Code/neovim-qt/build/bin/bench_scroll 11: Test timeout computed to be: 10000000 11: ********* Start testing of Test ********* 11: Config: Using QtTest library 5.10.1, Qt 5.10.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 8.2.0) 11: PASS : Test::initTestCase() 11: PASS : Test::benchScrollRegion() 11: RESULT : Test::benchScrollRegion(): 11: 0.42 msecs per iteration (total: 54, iterations: 128) 11: PASS : Test::cleanupTestCase() 11: Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 219ms 11: ********* Finished testing of Test ********* 1/1 Test #11: bench_scroll ..................... Passed 0.24 sec The following tests passed: bench_scroll 100% tests passed, 0 tests failed out of 1

Which gives the average time for one test execution.

We could try something similar using the ShellWidget class directly. For comparison here what we would want is two tests, one with using ligatures and one without.

If you check test_shellcontents.cpp you'll see that you can load data from a file instead of doing it programaticaly.

Something like this

diff --git a/src/gui/shellwidget/test/CMakeLists.txt b/src/gui/shellwidget/test/CMakeLists.txt index 92ff991..7fe254b 100644 --- a/src/gui/shellwidget/test/CMakeLists.txt +++ b/src/gui/shellwidget/test/CMakeLists.txt @@ -15,3 +15,4 @@ add_xtest(test_shellcontents) add_xtest(test_shellwidget) add_xtest(bench_scroll) add_xtest(bench_cell) +add_xtest(bench_widgetpaint) diff --git a/src/gui/shellwidget/test/bench_widgetpaint.cpp b/src/gui/shellwidget/test/bench_widgetpaint.cpp new file mode 100644 index 0000000..f220450 --- /dev/null +++ b/src/gui/shellwidget/test/bench_widgetpaint.cpp @@ -0,0 +1,23 @@ +#include <QtTest/QtTest> +#include "shellwidget.h" + +#if defined(Q_OS_WIN) && defined(USE_STATIC_QT) +#include <QtPlugin> +Q_IMPORT_PLUGIN (QWindowsIntegrationPlugin); +#endif + +class Test: public QObject +{ + Q_OBJECT +private slots: + void benchPaint() { + QBENCHMARK { + auto s = ShellWidget::fromFile("../test/buffer.c"); + s->show(); + s->repaint(); + } + } +}; + +QTEST_MAIN(Test) +#include "bench_widgetpaint.moc"

In this example I'm using one of the files in the repo which might not have many ligatures.

Heads up on that example

that path is probably wrong

before forcing a repaint you will likely want to resize

It actually has a decent amount of them, but I think this is more about rendering regular code anyway.
Time on master:

13: PASS : Test::initTestCase() 13: PASS : Test::benchPaint() 13: RESULT : Test::benchPaint(): 13: 3.5 msecs per iteration (total: 56, iterations: 16) 13: PASS : Test::cleanupTestCase() 13: Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 256ms

Time on ligature branch:

13: PASS : Test::initTestCase() 13: PASS : Test::benchPaint() 13: RESULT : Test::benchPaint(): 13: 4.6 msecs per iteration (total: 74, iterations: 16) 13: PASS : Test::cleanupTestCase() 13: Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 230ms

This isn't necessarily representative. I've run both a bunch of times and got higher and lower results with both. In general it would seem the new code has less consistent draw time. I'll add a few more lines to the test and try some more testing after adding the property.

A slightly better test case. Sorry about the formatting mess. The jpg file is just so I can examine what the test ended up doing.

#include <QtTest/QtTest> #include <QScreen> #include "shellwidget.h" #include "helpers.h" #if defined(Q_OS_WIN) && defined(USE_STATIC_QT) #include <QtPlugin> Q_IMPORT_PLUGIN (QWindowsIntegrationPlugin); #endif class Test: public QObject { Q_OBJECT ShellWidget *s; private slots: void init() { // FIXME: path QFileInfo fi("../../../../../src/gui/shellwidget/test/shellcontents/shell2.txt"); s = ShellWidget::fromFile(fi.absoluteFilePath()); s->resizeShell(80, 120); s->show(); } void cleanupTestCase() { // save a image of the widget after the last test s->grab().save("widgetpaint.png"); } void benchPaint() { QBENCHMARK { s->repaint(); } } }; QTEST_MAIN(Test) #include "bench_widgetpaint.moc"

Ok I've included a working benchmark with fixes for the path here 736f57a

equalsraf · 2018-10-02T11:24:40Z

We should also check what happens when fonts are not monospace. I am very tempted to put this behind some kind of feature flag/option, I suspect the performance impact will be significant.

Nuclearo · 2018-10-02T17:01:23Z

If the font is not monospace this will most likely be problematic. I assumed those weren't supported. A configurable flag sounds reasonable. If it's off we'll only update the changed sections as in the past and the loop will always stop after one step, otherwise we keep the new flow?

equalsraf · 2018-10-02T17:31:12Z

If the font is not monospace this will most likely be problematic. I assumed those weren't supported.

We kind of do, via :Guifont! fontname. On one hand some people really insist on it, on another some fonts are incorrectly detected as not being monospace, so the user needed a way to force it.

Nuclearo · 2018-10-02T17:44:06Z

I could make the property be

default = on iff font is detected as monospace
on = print with ligatures even if it could come out broken
off = print the old way

Where would this flag go?

equalsraf · 2018-10-02T17:54:54Z

I could make the property be

Lets start with off by default.

The actual setting can be stored in the ShellWidget class. We would need a way for users change this at run time. Similar to what is done with setGuiFont.

equalsraf · 2018-10-02T22:44:50Z

I'm no exactly sure what is its expected behaviour with this particular font

Fira Mono

Nothing happens really.

Fira Code

Ligatures work, but some cases dont work exactly as expected.

Nuclearo · 2018-10-04T15:43:12Z

Fira Mono doesn't have ligatures afaik. They're a font feature added by Fira Code.
I get that same behavior in Konsole and nvim-gtk. It seems to be related to Fira Code itself, since using FuraCode Nerd Font it comes as many single arrows.

equalsraf · 2018-10-05T16:00:12Z

Ok at least its consistent. If you get a feature flag going I'm ok with merging this.

equalsraf · 2018-10-06T17:40:09Z

src/gui/shellwidget/test/CMakeLists.txt

@@ -6,6 +6,8 @@ endif ()

 function(add_xtest SOURCE_NAME)
 	add_executable(${SOURCE_NAME} ${SOURCE_NAME}.cpp)
+	file(TO_NATIVE_PATH ${CMAKE_CURRENT_SOURCE_DIR} TEST_SOURCE_DIR)
+	add_compile_definitions(${SOURCE_NAME} TEST_SOURCE_DIR="${TEST_SOURCE_DIR}")


It seems add_compile_definitions does not work with older versions of cmake. This is why the tests are failing.

I think (untested) the alternative is to use add_definitions(${SOURCE_NAME} -DTEST_SOURCE_DIR="${TEST_SOURCE_DIR}")

There's that, but also bench_widgetpaint segfaults on win64.

That add_definitions line failed on linux. The following works fine, outside of the function:
add_definitions(-DTEST_SOURCE_DIR="${TEST_SOURCE_DIR}")
Since add_definitions only affects the directory and its subdirs I think that's fine.

equalsraf · 2018-10-06T21:49:51Z

src/gui/shellwidget/test/CMakeLists.txt

@@ -6,12 +6,16 @@ endif ()

 function(add_xtest SOURCE_NAME)
 	add_executable(${SOURCE_NAME} ${SOURCE_NAME}.cpp)
+	file(TO_NATIVE_PATH ${CMAKE_CURRENT_SOURCE_DIR} TEST_SOURCE_DIR)


Move this line out of this function, before the add_definition.

The segfault happens because the test fails to open the file. Since TEST_SOURCE_DIR is no defined.

Nuclearo · 2018-11-23T19:43:57Z

TO_NATIVE_PATH results in malformed path strings in VS (the backslashes aren't escaped). Changing it to TO_CMAKE_PATH did the trick.

Nuclearo · 2019-01-29T21:38:26Z

@equalsraf, the last commit I pushed added an RPC option to enable ligature rendering via
:call rpcnotify(0, 'Gui', 'Option', 'RenderLigatures', 1)
Is that alright as a feature flag?

equalsraf · 2019-01-31T08:23:33Z

src/gui/shellwidget/shellwidget.cpp

-						} else {
-							pen.setColor(m_fgColor);
+		// end_row is inclusive
+		QChar *rowStr = new QChar[m_contents.columns()];


I have two nitpicks with how this is done here

I would really like to avoid the rowStr copy when ligaturemode is off

Part of this loop is used to fill rowStr from cell contents. Maybe we can just add a method ShellContents::getRow(n) that allocates the array(s) and fills it up return all the pieces that need to be draw.

equalsraf · 2019-01-31T08:25:03Z

Sorry @Nuclearo I dropped out the face of the earth. The option looks fine. I've left a comment about the painting.

Nuclearo · 2019-02-03T18:37:07Z

No problem, I'm looking into what can be done about the copying.

vasalf · 2019-05-30T16:56:16Z

That's exactly the patch I've been looking for. Now I use nvim-qt version with ligature support locally. Hope it gets merged soon enough!

x1314aq · 2019-09-02T07:48:26Z

any update?

Nuclearo · 2019-09-11T18:45:59Z

Sorry about that first one, some git mess ups on my end.
I've fixed the merge issues, but I've found an issue with printing in my version which was either fixed or never present in master.

Top is master, bottom is mine. Note how the three lines character (right of 100%) is clipped.

Nuclearo · 2019-10-11T08:10:31Z

I think this is done, circleci failure seems to be at the server...
Let me know if there's anything stopping this from being merged.

stavlocker · 2019-10-13T14:51:49Z

I'd love to see this feature! Any updates on this?

0xADD1E · 2019-10-28T02:25:05Z

@Nuclearo it looks like the build failure was stuff with the base image that got fixed with b25f0c6. It looks like a rebase to that commit or newer should resolve the circleci failure

I'm also super excited to see this happening. Thank you for your work on this

Nuclearo · 2019-11-04T19:55:59Z

and there we go, squashed because it was getting too full of merge commits and passing tests nicely.

pitbuster · 2019-11-07T00:10:26Z

I noticed that undercurl (like the one in spell checking) seems to be broken with this PR 😔

Nuclearo · 2019-11-07T18:10:37Z

@hellbuster Can you write a test or give me a config that causes this issue?

pitbuster · 2019-11-08T15:18:55Z

@Nuclearo I could reproduce with -u NONE and I am not sure how to write a test for this, but I could try to run in a debugger to investigate what is going on.

pitbuster · 2019-11-12T21:44:02Z

I tried to get something useful by running through gdb. When I do :set spell, the program goes to the code that is supposed to draw the undercurl, so I don't have ideas on where the problem is 😞

Nuclearo · 2019-11-20T16:01:47Z

Yeah, I moved the background painting outside of the loop as a bit of an optimization and it backfired, silly me... This new patch should fix it. Thanks again for testing, @hellbuster!

skyegecko · 2020-04-20T20:01:23Z

What's the status of this PR right now? It's been a while since I did C++ but the merge conflicts seem non-trivial.

pianocomposer321 · 2020-04-24T15:00:55Z

Any particular reason this hasn't been merged yet? I seem to remember @equalsraf saying he was ok with merging.

pianocomposer321 · 2020-04-24T15:08:21Z

BTW @dutchgecko, FVim is a frontend for neovim written in F# + Avalonia, and it supports ligatures. The latest three versions are broken on windows though, so you're gonna want v0.2-192-gb6a3318 if you're on windows. See yatli/fvim#132 and yatli/fvim#132 (comment).

Nuclearo · 2020-05-02T17:26:33Z

I've been trying to keep the PR mergeable for a while, but refactoring in master makes these merges progressively more difficult. After all, this changes the entire flow of paintEvent, including indentations. At this point I might even prefer to just rewrite it from scratch, implementing a couple of improvements that I left for after it was merged.

famiu · 2020-06-15T14:39:04Z

I'd like this to be merged soon, only thing stopping me from using neovim-qt is lack of ligature support.

jgehrig · 2020-06-21T19:09:07Z

This feature is on my radar... We should support ligatures. Clearly there is enough user demand to justify the development work! 😁

Sorry for all of the merge conflicts, I am (mostly) the one responsible.

My plan is to modularize various rendering tasks into functions. This should at the least make re-writing the paintEvent code less work and easier to understand/follow.

I would also like to do some research on the QT Font Engine to see if ligatures can be pre-detected. The current rendering scheme would break certain non-monospace fonts and features (guifontwide). Not a big problem for now, since the change is gated in ginit.vim...

@Nuclearo

This is your feature, let me know how you would like to handle the work.

I am happy to do any of the following:

All of the re-base work, giving you commit authorship/credit. I will handle everything.
Do all of the refactor work, and leave this PR to you.
Nothing, let you handle everything.

Thanks!

xihnik · 2020-07-15T00:54:23Z

Ligatures
I may be blind, but this article did not help me solve the problem. In general, there is a solution at the moment, and if so, then how to make it work?

diefans · 2020-07-15T17:50:46Z

@RealXIHNIK

This branch has conflicts that must be resolved

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.

Nuclearo · 2020-08-02T06:45:34Z

Hi everyone, sorry for being so late, lost access to my account for a while.
Either way, it's great to see that the open source community always goes ahead with progressing. Thank you for picking up my slack @jgehrig :)
Since there's a new PR for this, should I close this one?

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.

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.

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.

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.

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: #470

This work would not have been done without @Nucearo 's efforts: #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 · 2020-09-29T21:17:16Z

Thanks @Nuclearo!

Ligatures are now available in master, run :GuiRenderLigatures 1.

The feature should be available for release 0.2.17. Marking as closed.

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/neovim-qt#470

equalsraf reviewed Oct 2, 2018

View reviewed changes

equalsraf reviewed Oct 6, 2018

View reviewed changes

equalsraf reviewed Jan 31, 2019

View reviewed changes

equalsraf mentioned this pull request Mar 21, 2019

Font scaling is different when used under a GTK DE #525

Closed

Nuclearo force-pushed the ligatures branch from 913d563 to 5722447 Compare September 11, 2019 18:41

Nuclearo force-pushed the ligatures branch from b7eb44e to 469e4c7 Compare November 4, 2019 19:36

jgehrig mentioned this pull request Jul 14, 2020

Ligatures not worked? #723

Closed

This was referenced Jul 29, 2020

Add Font Ligature Support :GuiRenderLigatures #727

Merged

Problems with redering Tamil characters #412

Open

jgehrig closed this Sep 29, 2020

Change printing to add ligature support #470

Change printing to add ligature support #470

Conversation

Nuclearo commented Oct 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsraf commented Oct 2, 2018

Nuclearo commented Oct 2, 2018

equalsraf commented Oct 2, 2018

Nuclearo commented Oct 2, 2018 • edited Loading

equalsraf commented Oct 2, 2018

equalsraf commented Oct 2, 2018

Fira Mono

Fira Code

Nuclearo commented Oct 4, 2018

equalsraf commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nuclearo commented Nov 23, 2018

Nuclearo commented Jan 29, 2019

Choose a reason for hiding this comment

equalsraf commented Jan 31, 2019

Nuclearo commented Feb 3, 2019

vasalf commented May 30, 2019

x1314aq commented Sep 2, 2019

Nuclearo commented Sep 11, 2019

Nuclearo commented Oct 11, 2019

stavlocker commented Oct 13, 2019

0xADD1E commented Oct 28, 2019

Nuclearo commented Nov 4, 2019

pitbuster commented Nov 7, 2019

Nuclearo commented Nov 7, 2019

pitbuster commented Nov 8, 2019

pitbuster commented Nov 12, 2019 • edited Loading

Nuclearo commented Nov 20, 2019

skyegecko commented Apr 20, 2020

pianocomposer321 commented Apr 24, 2020

pianocomposer321 commented Apr 24, 2020

Nuclearo commented May 2, 2020

famiu commented Jun 15, 2020

jgehrig commented Jun 21, 2020

xihnik commented Jul 15, 2020 • edited Loading

diefans commented Jul 15, 2020

Nuclearo commented Aug 2, 2020

jgehrig commented Sep 29, 2020

Nuclearo commented Oct 2, 2018 •

edited

Loading

pitbuster commented Nov 12, 2019 •

edited

Loading

xihnik commented Jul 15, 2020 •

edited

Loading