[vim/vim] gvim ligatures support with optimized screen output (#8933)
dusanx
PR: gvim ligatures support with optimized screen output
-
In gvim, mixed content strings (ascii / multibyte utf-8) that are printed to screen, instead of falling down to slower Pango output are now split into ascii and utf-8 parts. That way we print as much as possible in a much more optimized way, trough pre-cached glyphs. This is speed and code optimization on it's own.
-
Some programming fonts offer ligatures, combinations of plain ascii characters, that combined create different, usualy better looking shapes. This PR introduces
guiligaturesglobal option. Default value is empty string, ligatures are not shown, so this makes non-breaking change. -
User can
:set guiligaturesto an empty string or add as many characters as needed, depending on the font capabilities and computer speed. This is very flexible compared to other editor that offer ligatures support. -
PR should fully respect
:h coding-stylebut I am ready to make amends if something slipped.
Example:
:set guiligatures=!\"#$%&()*+-./:<=>?@[]^_{\|~
Recap: Being speed optimization and non-breaking change, this PR should be strong candidate for merging.
Ready to answer any question. Thank you.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/8933
Commit Summary
File Changes
- M runtime/doc/options.txt (11)
- M src/gui.c (24)
- M src/gui.h (2)
- M src/gui_gtk_x11.c (109)
- M src/option.h (3)
- M src/optiondefs.h (13)
- M src/optionstr.c (5)
- M src/proto/gui.pro (1)
- M src/proto/gui_gtk_x11.pro (1)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub.
Triage notifications on the go with GitHub Mobile for iOS or Android.
lgtm-com[bot]
This pull request introduces 1 alert when merging dbb4232 into be01090 - view on LGTM.com
new alerts:
- 1 for Declaration hides parameter
dusanx
Amending this one warning. one sec.
Christian Brabandt
thank you, I am looking forward for that change.
dusanx
That's it, ready to merge. Thanks @chrisbra for your help to quickly find where is what.
dusanx
Hold on, I found edge case where this PR creates weird artifacts with utf-8 characters in statusline. Debugging.
dusanx
Fixed, sorry about that, it was last minute edge case catch...
dusanx
Just to confirm, nothing else to do here, ready to merge.
dusanx
Since it is my first vim PR, question: Are this failed checks something I should look at or usual to be ignored stuff? Looking at few of them looks like it is not me related?
matveyt
@dusanx gui_set_ligatures() must be inside #if defined(FEAT_GUI_GTK). Currently it's not and for this reason Windows build fails.
Also, there's some problem with one of GUI tests (testdir/test_gui.vim::Test_encoding_conversion). Not sure why but it's SIGSEGV.
dusanx
@matveyt thanks, I'll take a look. Since I don't have windows and this is my first PR, maybe better that somebody jumps in and makes this accepted by windows? I can of course apply #if defined(FEAT_GUI_GTK) but only in a blind mode and I can't trigger CI tests myself so may be slow cycle to get it right.
Re (testdir/test_gui.vim::Test_encoding_conversion) I'll certainly try to see if I did something unexpected.
dusanx
set encoding=latin1... I'll need a day to test this. Thanks for your help, I'll commit soon.
dusanx
Updated, ready for tests:
# ifdef FEAT_GUI_GTKhopefully done right way, blind edit since I don't have windows to test.- Segfault completely my fault, with latin1 string goes trough
if (output_conv.vc_type != CONV_NONE), I was releasingvim_free(conv_buf);too early.
This two should be clear now. Let's see what tests say.
matveyt
Code in optionstr.c also must be guarded. I've created a PR into your repo.
dusanx
dusanx
@matveyt thanks for helping me, AppVeyor has finally passed. Of course next PR will require less help, still learning where is what in vim.
matveyt
@dusanx Don't mention it. We still have to wait for Bram to review & accept this.
codecov[bot]
Codecov Report
Merging #8933 (ce1dae9) into master (be01090) will decrease coverage by
87.67%.
The diff coverage is0.00%.
❗ Current head ce1dae9 differs from pull request most recent head 185510a. Consider uploading reports for the commit 185510a to get more accurate results
@@ Coverage Diff @@ ## master #8933 +/- ## =========================================== - Coverage 90.12% 2.45% -87.68% =========================================== Files 151 149 -2 Lines 169783 165955 -3828 =========================================== - Hits 153022 4077 -148945 - Misses 16761 161878 +145117
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | ? |
|
| huge-gcc-none | ? |
|
| huge-gcc-testgui | ? |
|
| huge-gcc-unittests | 2.45% <0.00%> (-0.01%) |
⬇️ |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/gui.c | 0.20% <0.00%> (-71.83%) |
⬇️ |
| src/gui_gtk_x11.c | 0.44% <0.00%> (-62.92%) |
⬇️ |
| src/menu.c | 0.00% <0.00%> (-92.59%) |
⬇️ |
| src/optionstr.c | 21.35% <0.00%> (-73.83%) |
⬇️ |
| src/version.c | 0.86% <ø> (-91.48%) |
⬇️ |
| src/float.c | 0.00% <0.00%> (-99.22%) |
⬇️ |
| src/gui_gtk_f.c | 0.00% <0.00%> (-97.54%) |
⬇️ |
| src/sound.c | 0.00% <0.00%> (-97.12%) |
⬇️ |
| src/crypt_zip.c | 0.00% <0.00%> (-97.06%) |
⬇️ |
| src/cmdhist.c | 0.00% <0.00%> (-97.03%) |
⬇️ |
| ... and 142 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 be01090...185510a. Read the comment docs.
dusanx
Hmhm, I don't like this one failing test but I'll take a look immediately.
Btw when this PR is merged someone can teach me if and how I can run this tests locally, before sending PR?
dusanx
Got it, sorry about this, I'll push in 10-20 minutes...
dusanx
Ok, so:
- I am really starting to love this tests
- This last bugfix was really embarrassing... please don't look anybody. I maybe had to expand buffer to 256 chars in order to test whole range of char_u ascii but that is most probably just a rumour.
dusanx
How green it is? Super green 😄
Bram Moolenaar
Please! No Chris Tucker imitations! :-)
dusanx
Haha good catch :)
dusanx
Short question since I am new here: my master is now five commits behind. Since I don't think there is anything else to do here and only waits for review, should I pull master, rebase, push this branch again or just wait? What is usually expected for PR?
Yegappan Lakshmanan
Short question since I am new here: my master is now five commits behind. Since I don't think there is anything else to do here and only waits for review, should I pull master, rebase, push this branch again or just wait? What is usually expected for PR?
vim-dev ML
Bram Moolenaar
So long as the pull request says "This branch has no conflicts with the base branch" there is no need to update.
—
You are receiving this because you commented.
Bram Moolenaar
A few remarks:
- Instead of using vim_memset() you can use CLEAR_FIELD(). Then the size is automatic.
- We have the style rule that below declarations there is an empty line.
- declarations must go at the start of a block, not after a statement. Not all compilers handle the C99 style (MSVC for example, even though this code won't be used on windows it is still good to keep this rule)
- in gui_set_ligatures() you don't need the "ch" variable, just use *p directly.
- the comment above gui_set_ligatures() mentions 128, the array is now 256 long. The top 128 are always zero, but I think that's a nice way to stay on the safe side, so long as the index is an unsigned char.
- in gui_set_ligatures() when a non-ASCII character is encountered this should result in an error, and the old option value should be kept.
- We need a test for the option. You can't check the effect, but at least check some valid values and some invalid values (with the error just added).
- the declaration of gui_gtk2_draw_string_ext() is more than 80 characters, in that case we use one argument per line.
dusanx
Thanks for the review. I'll push update in day or two, everything is easy to follow. I was also on the edge of using smaller 96 char map (since we are only interested in ascii 32-127) but that requires few more if statements in a tight loop, so as you say 256 keeps us on the safe side.
should result in an error,
How do we assign new error numbers?
We need a test for the option. You can't check the effect, but at least check some valid values and some invalid values (with the error just added).
This is the only thing I don't know (yet) how to implement and not sure how to test test locally. Is there any doc/guide how to add test? Or in simpler terms what proper existing test file where I should add this one?
Thanks!
dusanx
Updated, should be correct.
- gui.c returns E999 error which is just random test number. Teach me how to pick unused error number that makes sense, or tell me the number so I can edit and push, or just change my PR prior to merge.
- I can see error messages translated in different language files. After assigning proper error number do I need to do anything with language files to make this PR ready to merge?
- I had no idea how to write proper test.
set guiligaturesreturn error for<>=šaband no error for<>=ab. If somebody can help with this and write me a test I'll probably know how to do in on my own with next PR.
Anything else needed, I am ready to change and push. Thank you.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub.
dusanx
This should be general question but since it is tightly related to my PR, I am asking here.
declarations must go at the start of a block, not after a statement.
I did move most of the declarations to the top but some inner loop stuff remains inline, which is probably wrong. I can move that up too of course. However while doing that I was looking at the existing code and it has many inline declarations -- for example gui_gtk2_draw_string_ext (which was gui_gtk2_draw_string prior to this PR), which has stuff like char_u *p; or bunch of variables right below not_ascii: or several of them right inside while loop. I can follow the rules as long as I understand them, so I am bit unsure what is the correct approach? Is current code in gui_gtk2_draw_string_ext not up to standard?
I am asking because I have few more PRs in plans and some of them incude gui_gtk2_draw_string_ext modifications -- if variables in existing code aren't where they should be I can fix that too while working on other stuff.
matveyt
I can see error messages translated in different language files. After assigning proper error number do I need to do anything with language files to make this PR ready to merge?
The messages are translated with libintl in a pretty standard way. That is, you're only expected to use "underscore" macro around string literals. The actual translation will be made (when possible) on-the-fly using English string literal as a dictionary key.
If you wish you can also provide PR for a few translations into languages you know well. But that's not required.
I had no idea how to write proper test. set guiligatures return error for <>=šab and no error for <>=ab. If somebody can help with this and write me a test I'll probably know how to do in on my own with next PR.
All tests are under src/testdir. In this particular case it's src/testdir/test_gui.vim. It's just VimScript file, so you look into it and see how it's supposed to be done. As usual, make sure also to read Vim's help pages at :h testing.txt.
I can follow the rules as long as I understand them, so I am bit unsure what is the correct approach?
The main rule is simple as that: support for old C compilers. Concerning variable declarations, this means "all local declarations must stand on the top of {}-block". This is how C was defined by K&R.
dusanx
Yes! I have single-handedly vanquished the beast!
- All variable declarations where they should be.
- E1240 was empty so I took it, if there is better method than picking first empty Exxx value please change or let me know.
- Testing, asserting, throwing errors if needed.
Assuming that E1240 can be used this is ready to merge.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub.
dusanx
Can I ask somebody to take a look at
and tell me why Test_set_guiligatures() fails on one windows and one mac test? Do I need to wrap all of that into
if !g:x11_based_gui
let skipped = g:not_supported . 'guiligatures'
else
then test for gtk? I don't have windows or mac so I can only blind edit, push, wait for hours too see results -- not really efficient debug cycle.
Christian Brabandt
The test looks good, as far as I can tell. Do you have a failing logfile somewhere?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub.
dusanx
The test looks good, as far as I can tell. Do you have a failing logfile somewhere?
Sadly no, I just pushed another try with if !g:x11_based_gui and it was a bad idea -- I can see tests falling all over. Let me wait for failing tests to finish, revert change to previous one, push, and I'll @ you for help. I am not sure I can get tests right without help on first PR.
dusanx
I am probably wrong but all I can tell is that tests don't work consistently. Last night I pushed commit which failed on two tests (one windows and one mac). Then I tried commit with !g:x11_based_gui, which produces much more errors. Now reverting PR to exact copy from last night, there are 7 failures instead of 2. That's not consistent.
I won't push any more before somebody more experienced can take a look -- some of the failing reports show things like error: alternative path /usr/bin/llvm-cov-11 doesn't exist which doesn't suggest error on my side.
In short any help is welcome. @chrisbra I won't commit or touch logs so you can take a look. Thanks.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub.
Christian Brabandt
yeah, there seems to have been a change in the dependencies for the clang-11 package, that causes this failure. It's not your fault and I have just created #8993 to see if this helps. However the rest looks fine I think
dusanx
#8993 seems to fix the problems on initial tests. As a first time contributor I can't trigger rest of the tests myself but I expect them to pass now.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub.
dusanx
Guys, something is wrong with CI tests again -- please check 'failures' above. I don't think there is anything I can do on my side.
Christian Brabandt
Hm, I see one failing run: https://github.com/vim/vim/pull/8933/checks?check_run_id=3880403518
That is a flaky test I believe. The windows failure look strange, they didn't even start. Let me re-run the CI jobs
dusanx
Thanks @chrisbra for running tests again. Weird that now only one fails. I am trying to find a reason in logs but no luck so far -- can you spot anything wrong in that one failing log? Since the only two lines change in last few commits is test itself I would expect that if it is not working correctly it would fail everywhere.
dusanx
Failures:
From test_shell.vim:
Found errors in Test_shell_options():
command line..script D:/a/vim/vim/src2/testdir/runtest.vim[486]..function RunTheTest[44]..Test_shell_options line 83: Failed to run shell command, shell: pwsh, caught Vim(read):E485: Can't read file C:/Users/RUNNER~1/AppData/Local/Temp/VVZC5C4.tmp
This looks like CI problem but maybe I need to change something? I did follow tests nearby to create mine.
dusanx
If somebody can flip switch to run this one failed test I would appreciate the gesture, I can't do it myself and pretty sure it was/is CI problem. Alternatively if it is something I caused I would be thankful for an explanation how not to do it again. Anyway I have some spare time and few more improvements I would like to work on but they are based on this one so waiting to see how first one ends. Thank you.
Christian Brabandt
I cannot run just one single failing ci instance, I can only re-run all the CI systems and possibly we have another flaky test failing.
So I think you are fine currently.