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

[WIP] Feature/combi bang anywhere #1124

Draft
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

jchtt
Copy link

@jchtt jchtt commented May 12, 2020

This PR adds the ability to use the bang matching anywhere in combi mode as described here.

Same as the other PR below:

I tried using uncrustify but a) got an error (unknown option 'align_number_left'), and b) noticed that it changed a bunch of other files, so left it like this for now.

It's basically done, but I added WIP because I’m not super familiar with C (would like some code review, it’s pretty short) and I’d still have to write the documentation for it.

@sardemff7 sardemff7 marked this pull request as draft May 12, 2020 06:48
Copy link
Collaborator

@sardemff7 sardemff7 left a comment

Choose a reason for hiding this comment

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

I was writing a review of your changes when I just ended rewriting all of this slightly differently.
I am even considering removing half of the code as it seems unnecessary to me, but I need to check.

For the record, even if we don’t merge your PR, I didn’t spot any big issue with it. I wrote a few comments so you can improve it a bit, would you feel like it.

Comment on lines 229 to 233
unsigned offset = 0;
for ( unsigned i = 0; i < pd->num_switchers; i++ ) {
if ( pd->switchers[i].disable ) {
offset += pd->lengths[i];
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t see offset actually used anywhere?

Copy link
Author

Choose a reason for hiding this comment

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

This was in fact a leftover from when I first started working on it. In fact, all the previous code that was there to handle the bang seems superfluous. And actually not correct if there are multiple matching modes.

static void split_bang( const char *input, char **bang, char **rest ) {
// Splits string input into a part containing the bang and the part containing the rest,
// saved in the pointers bang and rest.
if ( input != NULL) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

An early return would be nicer here. Let bang be NULL if there is none, doesn’t saves much, but having an empty string is hardly needed at all here. (g_free, just as free is a no-op on NULL.)

Not sure on the exact policy concerning rofi, but it’s usually considered safe to use a goto in those cases.
You put *bang = NULL; early, to always set it (it’s cheap anyway, won’t hurt if you overwrite it) and use goto end; in the condition. The label being put just before the g_strdup(input).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to avoid the use of goto.

Copy link
Author

Choose a reason for hiding this comment

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

Setting *bang = NULL means I need to do checks afterward, so for now, I left it as an empty string. I did however rework the function with a lot of early returns.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, you change strlen(bang) > 0 to bang != NULL, it’s that big of a change, is it? :-)

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough, changed that now. Code got slightly more complicated because I found a bug: I was only looking for the first '!', while in fact, the first one could be part of a matching token, so now, we’re looking for the first one that is preceded by a space.

char *sob = g_utf8_strchr ( input, -1, '!' );
char *prev_char = g_utf8_find_prev_char( input, sob );

if (sob != NULL && (prev_char == NULL || ( config.combi_bang_anywhere && prev_char[0] == ' ' ))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we could reverse the condition with an early return, but it’ll turn a bit ugly, though I’d consider it anyway, would it be my code.
However, keeping the condition would need a twist: use g_unichar_isspace(g_utf8_get_char(prev_char)), it is safer.
Rule of thumb: use g_unichar_* functions to manipulate any character, use g_utf8_* to manipulate any string.

// Splits string input into a part containing the bang and the part containing the rest,
// saved in the pointers bang and rest.
if ( input != NULL) {
char *sob = g_utf8_strchr ( input, -1, '!' );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, it wouldn’t be bad to have a check for sob being NULL using goto end; for an early return.

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.

3 participants