-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Added multiple selections #299
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising! Flushed a first batch of comments. However, you're not matching the existing style of the code and no tests is unfortunately a no go.
@@ -32,6 +32,8 @@ Both parts will be displayed but only the first part will be used when | |||
searching. | |||
.It Fl h | |||
Output a help message and exit. | |||
.It Fl m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this feature opt-in to begin with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without -m (multiple choices), the caller waits for one line string. in 'multiple choices' mode the caller is prepared to receive a list of lines. So he had to specify which mode wants.
if (output_description) | ||
printf("%s\n", choice->description); | ||
} else { | ||
if ( opt_mark ) { /* if multiple choices, print out all selected strings */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If multiple choices are always enabled, you could drop the conditional and always iterate over all choices, printing the marked ones. In case ENTER
just set mark = 1
there as well then this logic should be applicable to all methods of chosing lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it cloud be and opt_mark = 0 case it could return immediately so no way to mess up with more marked lines.
@@ -851,7 +885,8 @@ print_choices(size_t offset, size_t selection) | |||
|
|||
if (i - offset < choices_lines) | |||
print_line(choice->string, choice->length, | |||
i == selection, choice->match_start, | |||
((i == selection) ? PL_STDOUT : 0) |\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant parens, odd indent, redundant line continuation and redundant spaces below...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont use spaces, only tabs, in a few cases spaces used to so more clear the one expression down of the other like this one. Try it with tab-width=4... Anyway, it is your code now rewrite it as you wish.
Note: the second commit it is just a 'make clean'; I think you have to update your .gitignore