2018-02-19 1:33 GMT+03:00 Nikolay Aleksandrovich Pavlov <[email protected]>: > 2018-02-19 0:30 GMT+03:00 Bram Moolenaar <[email protected]>: >> >> As mentioned in the discussion about the popup menu fix, it's not so >> easy to write a test to check that something shows up in the right >> place, with the right highlighting, etc. Basically, that Vim displayes >> the right text. And when the check fails, it's not so easy to figure >> out what changed. >> >> If have just submitted patch 8.0.1523, which adds functionality to work >> with terminal screen dumps. The idea is to write a test that puts Vim >> in a certain state, create a screen dump with term_dumpwrite() and check >> that it is equal to a reference screen dump. I added assert_equalfile() >> to make it easier to check if two files are equal. >> >> This should also make it a lot easier to check syntax highlighting: Load >> text for the language, enable syntax highlighting and create a screen >> dump. Any differences in highlighting from a reference dump will be >> easily found. >> >> If there is a difference, you can use term_dumpdiff() to find out where >> it is. It shows the text that is different, and pressing "s" swaps >> between the two versions, so you can see the context of the change. >> You can use term_dumpload() to view a screen dump. >> >> I haven't yet had time to actually write a test with this, thus things >> might still change. And it probably breaks in some cases. >> >> I also need to write an explanation of how to use this properly. It's >> not going to replace the existing tests. To use it in a portable way >> the size of the dump needs to be limited. >> >> Let me know what you think. > > Looks like overly complicated to use. Neovim has screen tests for > quite some time now and it has key properties regarding those tests > that make them convenient to use: > > 1. Tests use screen of a certain known dimensions, and creating a > screen with those dimensions is a matter of writing two lines: one to > create a screen saving object into a variable and second to attach > Neovim instance to that screen. This convenience is only a matter of > creating an utility library and needs not actually be done in C code > for Vim as well, even though Vim is going to use :terminal and Neovim > is using remote UI written specifically for tests. I do not know what > does max-height and max-width do, but supplying them to > term_dumpwrite() of all things looks like something too late to > matter: screen needs to be formed before calling this function and it > needs to be formed with certain dimensions. > 2. Another thing is that Neovim utility library handles waiting for > specific screen state with a timeout, and should catch that screen > state even if it is intermediate and not final one (though this > functionality is normally not needed). I do not know whether it would > be possible to catch intermediate states in Vim and in any case using > files is not going to make waiting fast which goes for the third > problem. > 3. Screen state is described completely in test code. Format is easy > to read and write by humans and it does not need documentation, but > even if term_dumpwrite() file format was easy to read (it looks like > it is not actually, described in next point) ***it dumps contents to a > separate file***. Don’t do this for tests, new style tests are better > not because they are in the same Vim instance (only good for > performance and bad for anything else), but because they keep expected > result close to the code which is intended to produce it what is > easier to track and debug. Separate file is a step back. > 4. File format is unreadable garbage. Format of the diff buffer is > neither readable unless you have thrice the screen height and terminal > height is small in first place. Compare the format from Vim (also in > test.tdump as I am pretty sure gmail will mangle text): > > > |/+0#d0d0d0255#1c1c1c255|h|o|m|e|/|z|y|x|/|.|l|o|c|a|l|/|b|i|n|/|v|i|r|t|u|a|l|e|n|v|w|r|a|p@1|e|r|.|s|h| > @77 > > |/+0#ff404010&|h|o|m|e|/|z|y|x|/|.|z|s|h|/|f|a|s|t|-|s|y|n|t|a|x|-|h|i|g|h|l|i|g|h|t|i|n|g|/|f|a|s|t|-|s|y|n|t|a|x|-|h|i|g|h|l|i|g|h|t|i|n|g|.|p|l|u|g|i|n|.|z|s|h|:|2|6|5|:| > |s|c|a|l|a|r| |p|a|r|a|m|e|t|e|r| > |Z|S|H|_|H|I|G|H|L|I|G|H|T|_|M|A|X|L|E|N|G > |T|H| |c|r|e|a|t|e|d| |g|l|o|b|a|l@1|y| |i|n| |f|u|n|c|t|i|o|n| > |(|a|n|o|n|)| +0#d0d0d0255&@80 > | +2#ffffff255#0087af255|z|y|x| |+0#0087af255#585858255| > |~+0#bcbcbc255&|/|O|p|e|r|a|D|o|w|n|l|o|a|d|s|/|f|a|r|t|/|c+2#d0d0d0255&|o|s|p|l|a|y| > |+0#585858255#1c1c1c255| | +0#d0d0d0255&@79 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > @119 > > with Neovim equivalent: > > screen:expect([[ > ^/home/zyx/.local/bin/virtualenvwrapper.sh | > {2:/home/zyx/.zsh/fast-syntax-highlighting/fast-syntax-h}| > {2:ighlighting.plugin.zsh:265: scalar parameter ZSH_HIGH}| > {2:LIGHT_MAXLENGTH created globally in function (anon)} | > {3: zyx }{4: }{5:…/Proj/c/}{6:neovim }{7: } {8: }{9: > list-reimplement } | > | > | > | > | > | > | > | > {1:term://.//9394:zsh }| > | > ]], {[1] = {bold = true, reverse = true}, [2] = {foreground = > Screen.colors.Brown1, special = Screen.colors.Grey0}, [3]= {special = > Screen.colors.Grey0, background = 26265, bold = true, foreground = > Screen.colors.Grey100}, [4] = {background = 5789784, foreground = > 26265, special = Screen.colors.Grey0}, [5] = {background = 5789784, > foreground = Screen.colors.Gray78, special = Screen.colors.Grey0}, [6] > = {special = Screen.colors.Grey0, background = 5789784, bold = true, > foreground = 14540253}, [7] = {foreground = 5789784, special = > Screen.colors.Grey0}, [8] = {foreground = 2894892, special = > Screen.colors.Grey0}, [9] = {background = 2894892, foreground = > Screen.colors.Gray78, special = Screen.colors.Grey0}}) > > Note that while both are autogenerated in *actual* code this may look like > > screen:set_default_attr_ids({ > EOB={bold = true, foreground = Screen.colors.Blue1}, > T={foreground=Screen.colors.Red}, > RBP1={background=Screen.colors.Red}, > RBP2={background=Screen.colors.Yellow}, > RBP3={background=Screen.colors.Green}, > RBP4={background=Screen.colors.Blue}, > }) > > … > > screen:expect([[ > | > {EOB:~ }| > {EOB:~ }| > {EOB:~ }| > {RBP1:(}{RBP2:()}{RBP1:)}^ | > ]]) > > : nothing could prevent you from using more human-readable names (EOB > is End-Of-Buffer abbreviation, RBP1 is RainBowParenthesis1 > abbreviation: not using abbreviations is possible, but that will bloat > code too much for my taste). Test suite also allows not emitting > `{group:…}` for some specific highlighting (mostly used for Normal) > and caret marks cursor position. > > The key points here: > > 1. No visual garbage for each character. What is more readable > `|z|y|x` or `zyx`? > 2. Highlighting is kept separately from text. > 3. Highlighting may be given readable names. > 4. Readable names for colors if they match some predefined ones. > 5. Cursor position is easy to find. > > (BTW, after seeing this nonsense for such complex case created by > Neovim’s screen:snapshot_util (equivalent to term_dumpwrite) I wrote > two patches so that autogenerated text will look like > > screen:expect([[ > ^/home/zyx/.local/bin/virtualenvwrapper.sh | > {2:/home/zyx/.zsh/fast-syntax-highlighting/fast-syntax-h}| > {2:ighlighting.plugin.zsh:265: scalar parameter ZSH_HIGH}| > {2:LIGHT_MAXLENGTH created globally in function (anon)} | > {3: zyx }{4: }{5:…/Proj/c/}{6:neovim }{7: } {8: }{9: > list-reimplement } | > | > | > | > | > | > | > | > {1:term://.//27928:zsh }| > | > ]], { > [1] = {bold = true, reverse = true}, > [2] = {foreground = Screen.colors.Brown1, special = > Screen.colors.Grey0}, > [3] = {special = Screen.colors.Grey0, background = 0x006699, > bold = true, foreground = Screen.colors.Grey100}, > [4] = {background = 0x585858, foreground = 0x006699, special = > Screen.colors.Grey0}, > [5] = {background = 0x585858, foreground = Screen.colors.Gray78, > special = Screen.colors.Grey0}, > [6] = {special = Screen.colors.Grey0, background = 0x585858, > bold = true, foreground = 0xdddddd}, > [7] = {foreground = 0x585858, special = Screen.colors.Grey0}, > [8] = {foreground = 0x2c2c2c, special = Screen.colors.Grey0}, > [9] = {background = 0x2c2c2c, foreground = Screen.colors.Gray78, > special = Screen.colors.Grey0}, > }) > > after they will be merged.) > > 5. I did not see documentation of the dumps format in the latest patch > which means that new functionality *must not be used* outside of Vim > own test suite. Neovim has this functionality available only in its > own test suite now and does not attempt to export this, but if new > functionality is to be used by third-party plugin developers it needs > to be subject to backwards compatibility, which needs creating > documentation.
Some notes which I think are not clear from my rather lengthy (and probably too angry) post: 1. Point 1 and 2 are just something that is worth writing VimL library for. Would not suggest to solve them in C. 2. Have not wrote that, but I expect point 5 to be fixed in the future, after feature is stabilized. 3. Points 3 (where screen state should be kept) and 4 (in what format) are actual complains about suboptimal design. 4. And the whole post is written from the POV of a person thinking “Bram again wrote suboptimal feature without asking anybody (or Neovim developers specifically) regarding how they solve the issue”. Why? We do have similar problems, we are not against sharing experience and asking *before* writing feature would make design better from the beginning. > >> >> -- >> My girlfriend told me I should be more affectionate. >> So I got TWO girlfriends. >> >> /// Bram Moolenaar -- [email protected] -- http://www.Moolenaar.net \\\ >> /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ >> \\\ an exciting new programming language -- http://www.Zimbu.org /// >> \\\ help me help AIDS victims -- http://ICCF-Holland.org /// >> >> -- >> -- >> You received this message from the "vim_dev" maillist. >> Do not top-post! Type your reply below the text you are replying to. >> For more information, visit http://www.vim.org/maillist.php >> >> --- >> You received this message because you are subscribed to the Google Groups >> "vim_dev" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> For more options, visit https://groups.google.com/d/optout. -- -- You received this message from the "vim_dev" maillist. Do not top-post! Type your reply below the text you are replying to. For more information, visit http://www.vim.org/maillist.php --- You received this message because you are subscribed to the Google Groups "vim_dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
