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

[Dmenu] Implement a select rows for dmenu multi-select. #1807

Closed
wants to merge 1 commit into from
Closed

[Dmenu] Implement a select rows for dmenu multi-select. #1807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions doc/rofi-dmenu.5
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,13 @@ Or any combination: '5,-3:,7:11,2,0,-9'
.PP
Urgent row, mark \fIX\fP as urgent. See \fB\fC-a\fR option for details.

.PP
\fB\fC-select-rows\fR \fIX\fP
Copy link

Choose a reason for hiding this comment

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

The term "select" can be confusing since it already used to mean highlighting the current item in the list before either confirm selection or check the box.

Then again, Rofi has already name the flag -multi-select so it could be that the maintainers do care about this term.

Copy link
Collaborator Author

@DaveDavenport DaveDavenport Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It is the term that is used everywhere else.. Using another term here would be confusing.
Multi-select is a nice to have and does not completely fit anyway.


.PP
If multi-select is enabled, pre-select rows, See \fB\fC-a\fR option for format details.
If same row is specified multiple times, it state is toggled on subsequential sets.
Copy link

Choose a reason for hiding this comment

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

I think this behavior is surprising. I prefer the box is checked if the row id appears more than once, and unchecked if the row id doesn't appear.

Is there any reason you think toggling based on odd/even number of occurence a superior behavior?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would make it so much easier to select all rows except a number of rows...
I am trying to make things easier for the user.

Copy link

@KSXGitHub KSXGitHub Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I see.

I would have suggested adding an option named -unselect-rows, but then I realized it cannot substitute -select-rows $FOO,$BAR,$BAZ to mean selecting $BAZ and $FOO except $BAR.

So now the question is: This python-like range syntax is also used in other options as well, do these options also support this toggling behavior? If they do, it is consistent. If they don't, it means that the range syntax is lacking, and instead of adding toggling behavior, we should improve the range syntax (in other PR/discussion ofc).

Copy link

Choose a reason for hiding this comment

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

This python-like range syntax is also used in other options as well, do these options also support this toggling behavior?

There is another option called -a to make "active rows". And it does not have the toggling behavior. Which means that -a 1,2,3,2 would make 1, 2, 3 active whilst -select-rows 1,2,3,2 only checks 1 and 3.

Copy link
Collaborator Author

@DaveDavenport DaveDavenport Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Neither '-a' or '-u' document that it has the toggling behaviour. I have no idea why you think it does?
I expect that developers of script that use advanced features like this will read the documentation and can understand the difference.
Its a feature I can support for select-row, but not (easily) for the others, I can see this as a benefit for the script developer.

Copy link
Collaborator Author

@DaveDavenport DaveDavenport Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(yes sorry, I misread and corrected my post already. It seems we did this in parallel).

I was saying about the consistency because the proposed solution ('&','!' ) or the new one now ( 'and' and 'not') is not the type of syntax we in rofi.
If this is so important we should not introduce another one.

To specify multiple items, we use ',' separator in several places.

entry, prompt {
}
window {
children: [inputbar, listview];
}

or

rofi -show drun -modes 'drun,window'

So if we list multiple ranges we should stick to ',' as list separator.

To negate we use '-' in the search entry box. But this is not ideal for this usecase.
(you can search for 'browser -chrome' to search all non chrome browser.)

Copy link

@KSXGitHub KSXGitHub Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

is not the type of syntax we in rofi.

You need to complete this sentence or I wouldn't understand.

If this is so important we should not introduce another one.

To specify multiple items, we use ',' separator in several places.

Reusing , wouldn't make sense because , is for adding items, not for intersecting set of items. In other words, , is an or operator, different from an and operator. 0:4,2:5 is 0,1,2,3,4 whereas 0:4 and 2:5 would be 2,3.

If this is so important

It's not important for my use cases right now, neither is the toggling feature, I only brought this up because I dislike the toggling behavior.

Copy link

Choose a reason for hiding this comment

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

To negate we use '-' in the search entry box. But this is not ideal for this usecase.
(you can search for 'browser -chrome' to search all non chrome browser.)

Because negative numbers are a thing.

This gives me an idea: Many websites have a search syntax similar to GitHub issue (k1:v1 k2:v2 pattern). So rofi could potentially introduce a similar syntax: 0:100 not:(10:90 not:40:50); or, if you want the not keyword to be more easily discernible from normal text: 0:100 @not:(10:90 @not:40:50).

Copy link
Collaborator Author

@DaveDavenport DaveDavenport Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

is not the type of syntax we in rofi.

You need to complete this sentence or I wouldn't understand.

is not the type of syntax we use in rofi.

Reusing , wouldn't make sense because , is for adding items, not for intersecting set of items. In other words, , is an or operator, different from and and operator. 0:4,2:5 is 0,1,2,3,4 whereas 0:4 and 2:5 would be 2,3.

eeuh what?? I was talking about adding items not intersecting, so ',' makes sense for having a list of ranges that should be selected. (As it is now). We use ',' to separate in a list already. How we specify what we then exclude then is not clear.

If we start describing thing in set theory.. then we should use the right syntax for that.. I would be fine with using ∪,∩, \ (union, intersection, set diff if i remember correctly) But I think we will have lost most of the developers/users by now.

Anyway I give up. If anybody wants to pick this up and create a PR , I'll reconsider.
I spend more time then I care to spend on/have for this already

Copy link

@KSXGitHub KSXGitHub Feb 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OK. I have created #1809 which removes the toggling behavior.

I only proposed a variety of different syntaxes to replace toggling because I dislike it.


.PP
\fB\fC-only-match\fR

Expand Down
5 changes: 5 additions & 0 deletions doc/rofi-dmenu.5.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ Active row, mark *X* as active. Where *X* is a comma-separated list of python(1)

Urgent row, mark *X* as urgent. See `-a` option for details.

`-select-rows` *X*

If multi-select is enabled, pre-select rows, See `-a` option for format details.
If same row is specified multiple times, it state is toggled on subsequential sets.

`-only-match`

Only return a selected item, do not allow custom entry.
Expand Down
5 changes: 3 additions & 2 deletions doc/rofi-theme.5
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1615,7 +1615,7 @@ property.
These cannot be changed using the \fB\fCchildren\fR property.

.PP
Each entries displayed by listview are captured by a \fB\fCbox\fR called \fB\fCelement\fR\&.
Each Entry displayed by listview is captured by a \fB\fCbox\fR called \fB\fCelement\fR\&.
An \fB\fCelement\fR widget can contain the following special child widgets:

.RS
Expand All @@ -1630,7 +1630,8 @@ An \fB\fCelement\fR widget can contain the following special child widgets:

.PP
By default the \fB\fCelement-icon\fR and \fB\fCelement-text\fR child widgets are added to the
\fB\fCelement\fR\&. This can be modified using the \fB\fCchildren\fR property.
\fB\fCelement\fR\&. This can be modified using the \fB\fCchildren\fR property or the
\fB\fC[no]-show-icons\fR option.

.PP
A child added with another name is seen as a \fB\fCbox\fR, this can be used as dynamic
Expand Down
82 changes: 77 additions & 5 deletions source/modes/dmenu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,66 @@ typedef struct {
DmenuModePrivateData *pd;
} Block;


static void dmenu_parse_multi_select_range ( DmenuModePrivateData *pd, const char *entries)
{
if ( entries == NULL ) {
return;
}
// Pre-alloc array.
if (pd->selected_list == NULL) {
pd->selected_list =
g_malloc0(sizeof(uint32_t) * (pd->cmd_list_length / 32 + 1));
}
char *entries_cp = g_strdup ( entries );
char *endp;
const char *const sep = ",";
for (char *token = strtok_r(entries_cp, sep, &endp); token != NULL;
token = strtok_r(NULL, sep, &endp)) {
const char *sep[] = {"-", ":"};
int pythonic = (strchr(token, ':') || token[0] == '-') ? 1 : 0;
int index = 0;

int start = -1;
int stop = -1;
for (char *inner_token = strsep(&token, sep[pythonic]); inner_token != NULL;
inner_token = strsep(&token, sep[pythonic])) {
if (index == 0) {
start = stop = (int)strtol(inner_token, NULL, 10);
index++;
continue;
}

if (inner_token[0] == '\0') {
stop = -1;
continue;
}

stop = (int)strtol(inner_token, NULL, 10);
if (pythonic) {
--stop;
}
}
// Fix negative numbers.
if ( start < 0 ) {
start = pd->cmd_list_length + start;
}
if ( stop < 0 ) {
stop = pd->cmd_list_length + stop;
}
// Fix starting
for ( int index = start; index <= stop; index++ ){
if ( index < 0 ) {
index = pd->cmd_list_length - index;
}
if ( index < (int)pd->cmd_list_length ) {
bittoggle(pd->selected_list, index);
}
}
}
g_free ( entries_cp );
}

static void read_add_block(DmenuModePrivateData *pd, Block **block, char *data,
gsize len) {

Expand Down Expand Up @@ -514,14 +574,20 @@ static int dmenu_mode_init(Mode *sw) {
DmenuModePrivateData *pd = (DmenuModePrivateData *)mode_get_private_data(sw);

pd->async = TRUE;
pd->multi_select = FALSE;

// For now these only work in sync mode.
if (find_arg("-sync") >= 0 || find_arg("-dump") >= 0 ||
find_arg("-select") >= 0 || find_arg("-no-custom") >= 0 ||
find_arg("-only-match") >= 0 || config.auto_select ||
find_arg("-selected-row") >= 0) {
find_arg("-selected-row") >= 0 ) {
pd->async = FALSE;
}
// In multi-select mode we should disable async mode.
if ( find_arg("-multi-select") >= 0 ) {
pd->async = FALSE;
pd->multi_select = TRUE;
}

pd->separator = '\n';
pd->selected_line = UINT32_MAX;
Expand Down Expand Up @@ -634,6 +700,14 @@ static int dmenu_mode_init(Mode *sw) {

read_input_sync(pd, -1);
}

if ( pd->multi_select ) {
char *entries = NULL;
if ( find_arg_str ( "-select-rows", &entries ) >= 0 ) {
dmenu_parse_multi_select_range(pd, entries);
}
}

gchar *columns = NULL;
if (find_arg_str("-display-columns", &columns)) {
pd->columns = g_strsplit(columns, ",", 0);
Expand Down Expand Up @@ -907,14 +981,10 @@ int dmenu_mode_dialog(void) {
DmenuScriptEntry *cmd_list = pd->cmd_list;

pd->only_selected = FALSE;
pd->multi_select = FALSE;
pd->ballot_selected = "☑ ";
pd->ballot_unselected = "☐ ";
find_arg_str("-ballot-selected-str", &(pd->ballot_selected));
find_arg_str("-ballot-unselected-str", &(pd->ballot_unselected));
if (find_arg("-multi-select") >= 0) {
pd->multi_select = TRUE;
}
if (find_arg("-markup-rows") >= 0) {
pd->do_markup = TRUE;
}
Expand Down Expand Up @@ -1006,6 +1076,8 @@ void print_dmenu_options(void) {
is_term);
print_help_msg("-a", "[list]", "List of row indexes to mark active", NULL,
is_term);
print_help_msg("-select-rows", "[list]", "List of row indexes to select when multi-select is enabled", NULL,
is_term);
print_help_msg("-l", "[integer] ", "Number of rows to display", NULL,
is_term);
print_help_msg("-window-title", "[string] ", "Set the dmenu window title",
Expand Down