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.

Sign up for GitHub

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

Jump to bottom

[WIP] Feature/combi bang anywhere #1124

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

[WIP] Feature/combi bang anywhere #1124

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e60ad92
Implemented bang anywhere option for combi mode
jchtt May 10, 2020
0a3e720
Clean up
jchtt May 10, 2020
ea0d52e
Fixed indent
jchtt May 12, 2020
7b625be
Revert "Fixed indent"
jchtt May 12, 2020
d08b6bf
Cleaned up results code
jchtt May 13, 2020
8c005e0
Reworked split_bang function.
jchtt May 13, 2020
c2c973b
More cleanup
jchtt May 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions config/config.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ Settings config = {
.plugin_path = PLUGIN_PATH,
.max_history_size = 25,
.combi_hide_mode_prefix = FALSE,
/** Combi bang anywhere */
.combi_bang_anywhere = FALSE,

.matching_negate_char = '-',

Expand Down
2 changes: 2 additions & 0 deletions include/settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,8 @@ typedef struct
/** Maximum history length per mode. */
unsigned int max_history_size;
gboolean combi_hide_mode_prefix;
/** Combi mode bang anywhere */
gboolean combi_bang_anywhere;

char matching_negate_char;

Expand Down
133 changes: 87 additions & 46 deletions source/dialogs/combi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,41 +150,87 @@ static void combi_mode_destroy ( Mode *sw )
mode_set_private_data ( sw, NULL );
}
}

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, '!' );
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.

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.

glong sob_offset = g_utf8_pointer_to_offset( input, sob );
const char *eob = g_utf8_strchr ( sob, -1, ' ' );
if ( eob == NULL ) {
// Set it to end.
eob = &(input[strlen(input)]);
}
ssize_t bang_len = g_utf8_pointer_to_offset ( sob, eob );

if ( bang_len > 1 ) {
*bang = g_utf8_substring( input, sob_offset + 1, sob_offset + bang_len );

char *head = g_utf8_substring ( input, 0, ( eob[0] != ' ' && prev_char != NULL) ? sob_offset - 1 : sob_offset );
const char *tail = ( eob[0] == ' ' ) ? g_utf8_next_char( eob ) : eob;
*rest = g_strdup_printf ( "%s%s", head, tail );
g_free(head);
return;
}
}
}
*bang = g_strdup("");
*rest = g_strdup(input);
return;
}

static ModeMode combi_mode_result ( Mode *sw, int mretv, char **input, unsigned int selected_line )
{
CombiModePrivateData *pd = mode_get_private_data ( sw );

if ( input[0][0] == '!' ) {
int switcher = -1;
// Implement strchrnul behaviour.
char *eob = g_utf8_strchr ( input[0], -1,' ' );
if ( eob == NULL ) {
eob = &(input[0][strlen(input[0])]);
}
ssize_t bang_len = g_utf8_pointer_to_offset ( input[0], eob ) - 1;
if ( bang_len > 0 ) {
for ( unsigned i = 0; switcher == -1 && i < pd->num_switchers; i++ ) {
const char *mode_name = mode_get_name ( pd->switchers[i].mode );
size_t mode_name_len = g_utf8_strlen ( mode_name, -1 );
if ( (size_t) bang_len <= mode_name_len && utf8_strncmp ( &input[0][1], mode_name, bang_len ) == 0 ) {
switcher = i;
}
}
}
gboolean return_from_fun = FALSE;
ModeMode retv;
char *bang;
char *rest;
split_bang(*input, &bang, &rest);

ssize_t bang_len = strlen(bang);
if ( bang_len > 0 ) {
int switcher = -1;
for ( unsigned i = 0; switcher == -1 && i < pd->num_switchers; i++ ) {
const char *mode_name = mode_get_name ( pd->switchers[i].mode );
size_t mode_name_len = g_utf8_strlen ( mode_name, -1 );
if ( (size_t) bang_len <= mode_name_len && utf8_strncmp ( bang, mode_name, bang_len ) == 0 ) {
switcher = i;
}
}
if ( switcher >= 0 ) {
if ( eob[0] == ' ' ) {
char *n = eob + 1;
return mode_result ( pd->switchers[switcher].mode, mretv, &n,
if ( strlen( rest ) > 0) {
retv = mode_result ( pd->switchers[switcher].mode, mretv, &rest,
selected_line - pd->starts[switcher] );
return_from_fun = TRUE;
}
return MODE_EXIT;
else {
retv = MODE_EXIT;
return_from_fun = TRUE;
}
}
}
}

g_free( bang );
g_free( rest );
if ( return_from_fun ) {
return retv;
}

if ( mretv & MENU_QUICK_SWITCH ) {
return mretv & MENU_LOWER_MASK;
}

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.

if ( selected_line >= pd->starts[i] &&
selected_line < ( pd->starts[i] + pd->lengths[i] ) ) {
return mode_result ( pd->switchers[i].mode, mretv, input, selected_line - pd->starts[i] );
Expand Down Expand Up @@ -274,36 +320,31 @@ static cairo_surface_t * combi_get_icon ( const Mode *sw, unsigned int index, in
return NULL;
}


static char * combi_preprocess_input ( Mode *sw, const char *input )
{
CombiModePrivateData *pd = mode_get_private_data ( sw );
for ( unsigned i = 0; i < pd->num_switchers; i++ ) {
pd->switchers[i].disable = FALSE;
}
if ( input != NULL && input[0] == '!' ) {
// Implement strchrnul behaviour.
const char *eob = g_utf8_strchr ( input, -1, ' ' );
if ( eob == NULL ) {
// Set it to end.
eob = &(input[strlen(input)]);
}
ssize_t bang_len = g_utf8_pointer_to_offset ( input, eob ) - 1;
if ( bang_len > 0 ) {
for ( unsigned i = 0; i < pd->num_switchers; i++ ) {
const char *mode_name = mode_get_name ( pd->switchers[i].mode );
size_t mode_name_len = g_utf8_strlen ( mode_name, -1 );
if ( !( (size_t) bang_len <= mode_name_len && utf8_strncmp ( &input[1], mode_name, bang_len ) == 0 ) ) {
// No match.
pd->switchers[i].disable = TRUE;
}
}
if ( eob[0] == '\0' || eob[1] == '\0' ) {
return NULL;
}
return g_strdup ( eob + 1 );
}
}
return g_strdup ( input );

char *bang;
char *rest;
split_bang(input, &bang, &rest);

ssize_t bang_len = strlen(bang);
if ( strlen(bang) > 0 ) {
for ( unsigned i = 0; i < pd->num_switchers; i++ ) {
const char *mode_name = mode_get_name ( pd->switchers[i].mode );
size_t mode_name_len = g_utf8_strlen ( mode_name, -1 );
if ( !( (size_t) bang_len <= mode_name_len && utf8_strncmp ( bang, mode_name, bang_len ) == 0 ) ) {
// No match.
pd->switchers[i].disable = TRUE;
}
}
}
g_free(bang);
return rest;
}

Mode combi_mode =
Expand Down
2 changes: 2 additions & 0 deletions source/xrmoptions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,8 @@ static XrmOption xrmOptions[] = {
"Max history size (WARNING: can cause slowdowns when set to high).", CONFIG_DEFAULT },
{ xrm_Boolean, "combi-hide-mode-prefix", { .snum = &config.combi_hide_mode_prefix }, NULL,
"Hide the prefix mode prefix on the combi view.", CONFIG_DEFAULT },
{ xrm_Boolean, "combi-bang-anywhere", { .snum = &config.combi_bang_anywhere }, NULL,
"Allow bang (!) to be used anywhere in the string if it occurs after a space character.", CONFIG_DEFAULT },
{ xrm_Char, "matching-negate-char", { .charc = &config.matching_negate_char }, NULL,
"Set the character used to negate the matching. ('\\0' to disable)", CONFIG_DEFAULT },
{ xrm_String, "cache-dir", { .str = &config.cache_dir }, NULL,
Expand Down