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

Terminfo #197

Merged
merged 4 commits into from
Jun 26, 2017
Merged

Terminfo #197

merged 4 commits into from
Jun 26, 2017

Conversation

mptre
Copy link
Owner

@mptre mptre commented Jun 24, 2017

Read keys defined as terminal capabilities from terminfo

A first step towards making pick truly portable. Defining a key using
the new CAP() macro will read the corresponding value for the given
capability using tigetstr(), the result will be stored in the keys
structure in order to only invoke tigetstr() once per capability.

@DBOTW could you verify that this PR works as intended on your machine?

@ghost
Copy link

ghost commented Jun 24, 2017

@mptre
Copy link
Owner Author

mptre commented Jun 25, 2017

@DBOTW great! I would like to do a release before we migrate the rest of
the keys just make any regression easier to bisect. Feel free to open a
new PR with your KEY -> CAP changes.

/cc @calleerlandsson

Copy link
Collaborator

@calleluks calleluks left a comment

Choose a reason for hiding this comment

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

I had one question and one general comment. Other than that, looks good to merge! I can create a release after this has been merged.

pick.c Outdated
for (i = 0; keys[i].key != UNKNOWN; i++) {
if (keys[i].str == NULL)
keys[i].str = tty_getcap(keys[i].cap);
len = strlen(keys[i].str);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about caching this in keys like we do with the string?

static struct {
enum key key;
const char *s;
size_t length;
char *cap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was wondering why this wasn't const char but then I saw tigetstr takes a char *. I'm not quite sure why though...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, that's exactly why it can't be const. I don't have an
explanation either.

@mptre
Copy link
Owner Author

mptre commented Jun 26, 2017

Great feedback! The length is now cached.

A first step towards making pick truly portable. Defining a key using
the new CAP() macro will read the corresponding value for the given
capability using tigetstr(), the result will be stored in the keys
structure in order to only invoke tigetstr() once per capability.
Also, overwrite any existing value for the mandatory environment
variables. This is of important in order make the test environment
consistent.
@mptre mptre merged commit 19ff2ea into master Jun 26, 2017
@mptre mptre deleted the terminfo branch June 26, 2017 19:28
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.

2 participants