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

[RDY] menu_get: prettyprint special chars #7340

Merged
merged 4 commits into from Oct 16, 2017

Conversation

Projects
None yet
6 participants
@KillTheMule
Contributor

KillTheMule commented Sep 30, 2017

@teto asked me to take this over, since he has problems with his setup. As far as I can see, this solves it, I also added a new test to verify under different circumstances. If anyone can think of a situation that should be tested, I'd be happy to add more.

Ref #6322

Closes #7272.

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Sep 30, 2017

There isn't any mention of special chars in the docs right now, so there's nothing to change. Should I add a note about this?

@marvim marvim added the RFC label Sep 30, 2017

@KillTheMule KillTheMule force-pushed the KillTheMule:menuget branch from f5dea64 to 4fcd2e3 Sep 30, 2017

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Oct 1, 2017

The code is a tad inefficient, since str2special_save returns a freshly allocated instance, and then tv_dict_add_str copies it out again. There's not much one can do about it, other than to modify str2special_save to take an additional optional buffer argument so it can reuse that (with some logic to extend its size if needed), in which case we can do the allocation/deallocation outside of the loop. I don't think the added complexity is worth it, but if someone thinks it should be done, I'd go for it. Just let me know :)

Scrap that, I just found out about tv_dict_add_allocated_str. I'm gonna use that :)

KillTheMule added some commits Sep 30, 2017

menu_get: adjust tests for prettyprinting
... and add a bit of new testing

@KillTheMule KillTheMule force-pushed the KillTheMule:menuget branch from 4fcd2e3 to 9fb8b47 Oct 1, 2017

@@ -698,7 +698,9 @@ static dict_T *menu_get_recursive(const vimmenu_T *menu, int modes)
if (*menu->strings[bit] == NUL) {
tv_dict_add_str(impl, S_LEN("rhs"), (char *)"<Nop>");

This comment has been minimized.

@ZyX-I

ZyX-I Oct 1, 2017

Contributor

BTW, it is not <Nop> in case of nvim_get_keymap, it is an empty string. Given that :echo string("\<Nop") echoes '<Nop>' and nvim_input('<Nop>xxxxx') neither runs any of xxxxx or tries to treat < as an operator this if() is to be removed and not the other way around (i.e. fixing nvim_get_keymap).

I.e. while it is nice to be displayed, it is neither consistent with nvim_get_keymap nor can be fed to nvim_input() or feedkeys() (latter needs something liike feedkeys(eval('"' . substitute(string, "<", '\\<', 'g') . '"')) to work, this is why I used string("\<Nop>")). Though current treatment of <Nop> in nvim_input looks like a bug.

This comment has been minimized.

@KillTheMule

KillTheMule Oct 2, 2017

Contributor

If I understand you correctly, this shouldn't be removed, but made into

 tv_dict_add_str(impl, S_LEN("rhs"), (char *)"");

right?

}

eq(m, expected)
end)
end)

This comment has been minimized.

@ZyX-I

ZyX-I Oct 1, 2017

Contributor

BTW, whoever coded this test suite first needed to have a test for <Nop>.

This comment has been minimized.

@ZyX-I

ZyX-I Oct 1, 2017

Contributor

Would also be nice to have tests with spaces inside both rhs and lhs. And missing tests for right aligned text. May be combined to some extent: :menu not\ tested.spaces,\ nop,\ right\ aligned\ text<Tab>right\ aligned\ text <Nop> (still no spaces in rhs).

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Oct 2, 2017

I dealth with <Nop>, hopefully in the way @ZyX-I imagined. The right aligned text (added via <Tab>), was not accounted for, so I added it to the output. Is that the way it should be?

I went through the items of vimmenu_T to check what's accounted for in menu_get. We're missing en_name, en_dname, everything else at least appears in the code of menu_get. Might be ok because this was merged previously, but I thought I'd mention it.

@KillTheMule KillTheMule closed this Oct 2, 2017

@KillTheMule KillTheMule reopened this Oct 2, 2017

@jszakmeister jszakmeister removed the RFC label Oct 2, 2017

@marvim marvim added the RFC label Oct 2, 2017

@@ -696,9 +700,11 @@ static dict_T *menu_get_recursive(const vimmenu_T *menu, int modes)
if ((menu->modes & modes & (1 << bit)) != 0) {
dict_T *impl = tv_dict_alloc();
if (*menu->strings[bit] == NUL) {
tv_dict_add_str(impl, S_LEN("rhs"), (char *)"<Nop>");
tv_dict_add_str(impl, S_LEN("rhs"), (char *)"");

This comment has been minimized.

@ZyX-I

ZyX-I Oct 2, 2017

Contributor

No, just remove if(), what do you think str2special_save is going to do with the empty string and how exactly this is common to justify adding four more lines? nvim_get_keymap does not bother with treating empty string specially. Also the cast here is in any case useless.

This comment has been minimized.

@KillTheMule

KillTheMule Oct 2, 2017

Contributor

Eh right yeah, sorry for being dense :D I'll fix that.

priority = 500,
name = "Test4"
}
}

This comment has been minimized.

@ZyX-I

ZyX-I Oct 2, 2017

Contributor

By the way, wondering how do you create such lengthy dictionaries? For #7234 and previous PRs I used to write the whole thing by hand until I got too bored and finally added https://github.com/ZyX-I/neovim/blob/f2724f85ed32f20443cfaa3a636ec875c313b19f/test/helpers.lua#L310-L359 which may be used to dump lua tables and strings (no support for numbers yet because specifically for my PR I do not need them), but maybe there already is something existing.

This comment has been minimized.

@KillTheMule

KillTheMule Oct 2, 2017

Contributor

Steady hand and a needle...

Hrhr, no, I'm good, top of the file, first menuget test, contains a comment that helps with this. Thanks for asking though :)

@KillTheMule KillTheMule force-pushed the KillTheMule:menuget branch from b40c029 to 1f61387 Oct 2, 2017

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Oct 2, 2017

I think that en_name, en_dname and actext should be in output, but not sure whether it is needed to be in this PR. I only asked about <Tab> because I thought it is already dumped, just not tested; and I thought that because <Tab> is easy to read out of help for somebody not actually using menus, not a case for en_name and en_dname.

I think though that “untranslated name” is a thing which should be dumped by default and translated variant being optional and only should it differ from the untranslated value it should be included: it is normally more robust to rely on having specific English name then to rely on specific translation.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Oct 2, 2017

pl.pretty? Checked that, found that I am not going to use that for it being not flexible: first of all, double quotes are not consistent with most of the test code, second I can’t just add features here I may want to, some ideas I plan it to be implemented on as-needed basis:

  1. specify key sorting order
  2. some intrinsic to change string format to e.g. dedent([[…]]) should I need to
  3. allow to dump recursive tables
  4. allow to dump tables with e.g. NIL (AFAIR it is essentially an empty table which was assigned special meaning; even if it is not, I have other “special empty or not so empty tables” out there)
@ZyX-I

ZyX-I approved these changes Oct 2, 2017

@teto

This comment has been minimized.

Contributor

teto commented Oct 3, 2017

I don't see the point in sending the English menu names alongside with their translation. If one wants to the English version, he can change the locale.
I don't get what is "accelerator text", it seems like a default mnemonic ?

Something that is missing compared to vim is the menu icon but I delayed it for another PR. One of the goal here was to unblock release of 0.2.1, kudos to @KillTheMule . Thanks for spotting and fixing the "" problem too.
As for creating the array, I thought I had to input the command and :echo menu_get(...) would return sthg that can be prettified with vim gq. Glad to know there is a neovim function != pl.pretty for that though.

@KillTheMule

This comment has been minimized.

Contributor

KillTheMule commented Oct 3, 2017

I don't get what is "accelerator text", it seems like a default mnemonic ?

It's the right aligned text on a menu, you can set it by putting a <Tab> after the menu path. It's used for descriptive purposes (although I've never seen it in action really).

@KillTheMule KillTheMule changed the title from [RFC] menu_get: prettyprint special chars to [RDY] menu_get: prettyprint special chars Oct 3, 2017

@marvim marvim added RDY and removed RFC labels Oct 3, 2017

@justinmk justinmk merged commit a792c1f into neovim:master Oct 16, 2017

3 checks passed

QuickBuild Build pr-7340 finished with status SUCCESSFUL
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 76.476%
Details

@justinmk justinmk removed the RDY label Oct 16, 2017

@KillTheMule KillTheMule deleted the KillTheMule:menuget branch Oct 16, 2017

justinmk added a commit that referenced this pull request Oct 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment