Feature request for a "shelf" option ( #51) by DevHyperCoder · Pull Request #52 · mwh/dragon

DevHyperCoder · 2022-06-05T06:39:48Z

In relation with #51

N-R-K · 2022-06-05T09:03:49Z

+// https://stackoverflow.com/a/23497087
+  GtkWidget*
+    find_child(GtkWidget* parent, const gchar* name)
+    {


Indentation of this entire function is severely off.

N-R-K · 2022-06-05T09:11:20Z

+                    }
+                    while ((children = g_list_next(children)) != NULL);


} and while typically go on the same line for do .. while loops so that it's not confused as a regular while loop.

Suggested change

}

while ((children = g_list_next(children)) != NULL);

} while ((children = g_list_next(children)) != NULL);

N-R-K · 2022-06-05T09:13:08Z

                || strcmp(argv[i], "--and-exit") == 0) {
            and_exit = true;
+        } else if (strcmp(argv[i], "-X") == 0
+                || strcmp(argv[i], "--individual-exit") == 0) {


Kind of a long name for a cli arg, something simpler might be better. But I guess not that big of a deal since there's the short form -X.

N-R-K · 2022-06-05T09:13:51Z

+
+# clangd
+compile_commands.json
+.cache/


IMO these should go into user's global ignore file rather than on the project's one.

N-R-K · 2022-06-05T09:15:10Z

        gtk_main_quit();
+    }
+
+    if(individual_exit) {


Use spaces before and after the parens. e.g if (x) {

N-R-K · 2022-06-05T09:16:38Z

+            return;
+        }
+
+        GtkWidget* button = find_child(vbox,dd->text);


Space after the comma.

Suggested change

GtkWidget* button = find_child(vbox,dd->text);

GtkWidget* button = find_child(vbox, dd->text);

N-R-K · 2022-06-05T09:21:31Z

    }
-    if (and_exit)
+
+    if (and_exit){


What happens when user invokes both -x and -X ? Seems like -x will take priority. It's more typical that the latter argument provided via cli to override the previous one.

Instead of using two bools, I'd use a single enum enum { QUIT_NONE, QUIT_ALL, QUIT_ITEM } where QUIT_ALL would be the current -x and QUIT_ITEM would be -X.

Should do the trick. Though the names could be better.

diff --git a/dragon.c b/dragon.c index ae3bac0..4c253f6 100644 --- a/dragon.c +++ b/dragon.c @@ -35,13 +35,12 @@ char *progname; bool verbose = false; int mode = 0; int thumb_size = 96; -bool and_exit; -bool individual_exit; // TODO docs ; man bool keep; bool print_path = false; bool icons_only = false; bool always_on_top = false; +static enum { QUIT_NONE, QUIT_ALL, QUIT_ITEM } quit_mode; static char *stdin_files; #define MODE_HELP 1 @@ -142,11 +141,9 @@ void drag_end(GtkWidget *widget, GdkDragContext *context, gpointer user_data) { free(action_str); } - if (and_exit){ + if (quit_mode == QUIT_ALL) { gtk_main_quit(); - } - - if(individual_exit) { + } else if (quit_mode == QUIT_ITEM) { if (uri_count == 0) { gtk_main_quit(); return; @@ -412,7 +409,7 @@ drag_data_received (GtkWidget *widget, } else if (verbose) fputs("Received nothing\n", stderr); gtk_drag_finish (context, TRUE, FALSE, time); - if (and_exit) + if (quit_mode == QUIT_ALL) gtk_main_quit(); } @@ -538,10 +535,10 @@ int main (int argc, char **argv) { mode = MODE_TARGET; } else if (strcmp(argv[i], "-x") == 0 || strcmp(argv[i], "--and-exit") == 0) { - and_exit = true; + quit_mode = QUIT_ALL; } else if (strcmp(argv[i], "-X") == 0 || strcmp(argv[i], "--individual-exit") == 0) { - individual_exit = true; + quit_mode = QUIT_ITEM; } else if (strcmp(argv[i], "-k") == 0 || strcmp(argv[i], "--keep") == 0) { keep = true;

DevHyperCoder · 2022-06-05T09:47:35Z

Does the code formatting in this repo conform to any existing utility (like astyle). That would make the life easier for future contributiors

N-R-K

The --help output needs to be updated as well. Otherwise mostly seems okay, can't say for sure since I'm not too well versed on glib nor gtk.

N-R-K · 2022-06-05T18:47:04Z

+    }
+
+    if (GTK_IS_CONTAINER(parent)) {
+        GList *children = gtk_container_get_children(GTK_CONTAINER(parent));


Is it possible for this to return NULL ? If it is then it's going to lead to a null-deref down below.

N-R-K · 2022-06-05T18:48:52Z

 bool icons_only = false;
 bool always_on_top = false;

+static enum { QUIT_NONE, QUIT_ALL, QUIT_ITEM } quit_mode;


If the cli arg will be kept --individual-exit then probably should rename QUIT_ITEM to QUIT_INDIVIDUAL.

N-R-K · 2022-06-05T18:51:37Z

+    if (quit_mode == QUIT_ALL){
        gtk_main_quit();
+    }
+
+    if (quit_mode == QUIT_ITEM) {


Space after ). Also since they are mutually exclusive, else if makes more sense IMO.

Suggested change

if (quit_mode == QUIT_ALL){

gtk_main_quit();

}

if (quit_mode == QUIT_ITEM) {

if (quit_mode == QUIT_ALL) {

gtk_main_quit();

} else if (quit_mode == QUIT_ITEM) {

N-R-K · 2022-06-05T18:51:59Z

+        if (button != NULL) {
+            gtk_container_remove(GTK_CONTAINER(vbox), button);
+        } else {
+            fprintf(stderr, "Could not find button with label: %s",dd->text);


Suggested change

fprintf(stderr, "Could not find button with label: %s",dd->text);

fprintf(stderr, "Could not find button with label: %s", dd->text);

FichteFoll · 2022-06-10T08:42:42Z

-            and_exit = true;
+            quit_mode = QUIT_ALL;
+        } else if (strcmp(argv[i], "-X") == 0
+                || strcmp(argv[i], "--individual-exit") == 0) {


The naming isn't exactly intuitive to me. I wouldn't expect this option to cause the application to exit once all the individual items have been dragged out of it.

How about naming it --item-remove, --remove-individual, --once-each or similar and then use this option together with --and-exit to trigger an exit once all items have been moved out? Whereas if the option wasn't specified, --and-exit would still cause a quit after the first successful drag.

Yeah --once-each sounds like a better naming option to me as well.

FichteFoll · 2022-06-10T08:47:02Z

 }

 void drag_end(GtkWidget *widget, GdkDragContext *context, gpointer user_data) {
+    uri_count--;


This will break target mode together with shelf mode (or 'drag once mode') and cause URIs to be overridden as that uses the uri_count counter for indexing. Besides, imo it would be better to have this near the code that is responsible for handling individual entiries when quit_mode == QUIT_ITEM.

And finally, what if -X is used together with --all?

so you mean i should decrement uri_count near the if statement ? Or should i make it use a different counter

DevHyperCoder added 3 commits May 30, 2022 09:26

add individual exit option

d47e3a4

Add clangd files to .gitignore

6d453e8

Update manpage

4af8dd9

N-R-K suggested changes Jun 5, 2022

View reviewed changes

DevHyperCoder added 2 commits June 5, 2022 21:52

Refactor to quit_mode enum

aeb75d5

Format

9c5f708

N-R-K suggested changes Jun 5, 2022

View reviewed changes

FichteFoll reviewed Jun 10, 2022

View reviewed changes

	}
	while ((children = g_list_next(children)) != NULL);
	} while ((children = g_list_next(children)) != NULL);

	GtkWidget* button = find_child(vbox,dd->text);
	GtkWidget* button = find_child(vbox, dd->text);

	fprintf(stderr, "Could not find button with label: %s",dd->text);
	fprintf(stderr, "Could not find button with label: %s", dd->text);

Conversation

DevHyperCoder commented Jun 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DevHyperCoder commented Jun 5, 2022

Uh oh!

N-R-K left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FichteFoll Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FichteFoll Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FichteFoll Jun 10, 2022 •

edited

Loading

FichteFoll Jun 10, 2022 •

edited

Loading