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

Print keybindings starting with space correctly #1111

Merged
merged 1 commit into from Jul 18, 2023

Conversation

MaxGyver83
Copy link
Contributor

Bug fix for #1060.

@rnpnr
Copy link
Collaborator

rnpnr commented Jul 10, 2023

Hi Max, thanks for the patch!

This fixes one of the issues listed in #1060 but doesn't replace any additional spaces with . Are you able to address that as well? Maybe a local 18 char buffer to copy key into while replacing with ,

@MaxGyver83
Copy link
Contributor Author

Hi Randy, sure. I didn't think about this.

I have updated this pull request. I hope it's not too complicated. The main difficulty is that spaces (1 byte each) get replaced with which is 3 bytes but only 1 character. I have limited the number of space replacements to 3 because otherwise the destination string might grow in an unpredictable way.

@rnpnr
Copy link
Collaborator

rnpnr commented Jul 11, 2023

Definitely a little more complicated then I was expecting but its
partially because I forgot how to count bytes. It's on the right track
though so I'll leave some comments inline.

vis-cmds.c Outdated Show resolved Hide resolved
vis-cmds.c Outdated Show resolved Hide resolved
vis-cmds.c Show resolved Hide resolved
Copy link
Collaborator

@rnpnr rnpnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the minor comment above this looks good to me. If you want to
squash the commits I would be OK with merging.

I will just wait a couple days to see if anyone else has any comments.

vis-cmds.c Outdated Show resolved Hide resolved
@MaxGyver83 MaxGyver83 force-pushed the print-space-keybindings-correctly branch from 2b8c7e1 to 6ce9675 Compare July 11, 2023 17:09
Fixes martanne#1060 - :help doesn't display mappings starting with <Space>
correctly

Co-authored-by: Randy Palamar <palamar@ualberta.ca>
@rnpnr rnpnr force-pushed the print-space-keybindings-correctly branch from 6ce9675 to 6be370d Compare July 18, 2023 03:17
@rnpnr rnpnr merged commit 6be370d into martanne:master Jul 18, 2023
24 checks passed
@rnpnr
Copy link
Collaborator

rnpnr commented Jul 18, 2023

Applied! Thanks for your work getting this patch in shape!

@erf
Copy link
Contributor

erf commented Jul 18, 2023

I get the following warning when building the latest version with this change:

In file included from sam.c:1813:
./vis-cmds.c:663:34: warning: field width should have type 'int', but argument has type 'unsigned long' [-Wformat]
        return text_appendf(data, "  %-*s\t%s\n", 18+invisiblebytes, buf, (char*)value);
                                     ~~~^         ~~~~~~~~~~~~~~~~~
./vis-cmds.c:673:34: warning: field width should have type 'int', but argument has type 'unsigned long' [-Wformat]
        return text_appendf(data, "  %-*s\t%s\n", 18+invisiblebytes, buf, desc ? desc : "");

@MaxGyver83
Copy link
Contributor Author

I get the following warning when building the latest version with this change:

In file included from sam.c:1813:
./vis-cmds.c:663:34: warning: field width should have type 'int', but argument has type 'unsigned long' [-Wformat]
        return text_appendf(data, "  %-*s\t%s\n", 18+invisiblebytes, buf, (char*)value);
                                     ~~~^         ~~~~~~~~~~~~~~~~~
./vis-cmds.c:673:34: warning: field width should have type 'int', but argument has type 'unsigned long' [-Wformat]
        return text_appendf(data, "  %-*s\t%s\n", 18+invisiblebytes, buf, desc ? desc : "");

Oh, looks like I missed this. I'll change the type of invisiblebytes to int!

@rnpnr
Copy link
Collaborator

rnpnr commented Jul 18, 2023

Fixed in 599ced0. Sorry about that I don't know how I missed it.

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.

None yet

3 participants