[Github-comments] [geany/geany-plugins] Project Organizer: Add option to open current folder in file manager and terminal (PR #1126)

[Jiří Techet]

@techee requested changes on this pull request.



> +	static gchar *open_command = NULL;
+
+	if (!open_command)
+		open_command = g_strdup(PRJORG_COMMAND_TERMINAL);
+
+	if (prj_org)
+	{
+		g_free(open_command);
+		open_command = g_strdup(prj_org->command_terminal);
+	}
+
+	locale_path = get_folder_for_selection();
+	if (!spawn_async(locale_path, open_command, NULL, NULL, NULL, NULL))
+		msgwin_status_add("Unable open terminal: %s", open_command);
+
+	if (locale_path)

g_free() works on NULL pointers, the if is unnecessary here

> +}
+
+
+void on_open_terminal(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data)
+{
+	gchar *locale_path = NULL;
+	static gchar *open_command = NULL;
+
+	if (!open_command)
+		open_command = g_strdup(PRJORG_COMMAND_TERMINAL);
+
+	if (prj_org)
+	{
+		g_free(open_command);
+		open_command = g_strdup(prj_org->command_terminal);
+	}

Personally I'd just avoid the static variable and would do
```
open_command = prj_org ? g_strdup(prj_org->command_terminal) : g_strdup(PRJORG_COMMAND_TERMINAL);
```
and would free this at the end of the function.

> +}
+
+
+void on_open_file_manager(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data)
+{
+	gchar *locale_path = NULL;
+	static gchar *open_command = NULL;
+
+	if (!open_command)
+		open_command = g_strdup(PRJORG_COMMAND_OPEN);
+
+	if (prj_org)
+	{
+		g_free(open_command);
+		open_command = g_strdup(prj_org->command_file_manager);
+	}

like in on_open_terminal(), avoid the static and extra allocation.

> +		}
+	}
+
+	/* Try home folder */
+	if (!locale_path)
+		locale_path = g_strdup(g_get_home_dir());
+
+	/* Make sure path exists */
+	if(locale_path && !g_file_test(locale_path, G_FILE_TEST_IS_DIR))
+	{
+		g_free(locale_path);
+		locale_path = NULL;
+	}
+
+	return locale_path;
+}

I think both get_fullpath_for_selection() and get_folder_for_selection() are an overkill. I think you could just simply get the path like in on_delete() and then test whether the resulting path is a file or directory, remove the file part if it's a file and use the resulting path to open file manager or terminal. I don't think any super-strict checks are necessary here - in the worst case the terminal or the file manager get opened at a wrong path.

> @@ -1653,6 +1847,20 @@ void prjorg_sidebar_init(void)
 	gtk_widget_show(item);
 	gtk_container_add(GTK_CONTAINER(s_popup_menu.widget), item);
 
+	item = menu_item_new("folder-open", _("File Manager"));

The name should be an action, so better to call it "Open File Manager".

> @@ -1653,6 +1847,20 @@ void prjorg_sidebar_init(void)
 	gtk_widget_show(item);
 	gtk_container_add(GTK_CONTAINER(s_popup_menu.widget), item);
 
+	item = menu_item_new("folder-open", _("File Manager"));
+	gtk_container_add(GTK_CONTAINER(s_popup_menu.widget), item);
+	g_signal_connect((gpointer) item, "activate", G_CALLBACK(on_open_file_manager), NULL);
+	s_popup_menu.find_in_directory = item;
+
+	item = menu_item_new("terminal", _("Terminal"));

"Open Terminal"

> +	ui_entry_add_clear_icon(GTK_ENTRY(e->command_file_manager));
+	gtk_widget_set_tooltip_text(e->command_file_manager,
+		_("The command used to open folders in the file manager."));
+	gtk_entry_set_text(GTK_ENTRY(e->command_file_manager), prj_org->command_file_manager);
+
+	label = gtk_label_new(_("Terminal:"));
+	gtk_misc_set_alignment(GTK_MISC(label), 0, 0);
+	e->command_terminal = gtk_entry_new();
+	ui_table_add_row(GTK_TABLE(table), 2, label, e->command_terminal, NULL);
+	ui_entry_add_clear_icon(GTK_ENTRY(e->command_terminal));
+	gtk_widget_set_tooltip_text(e->command_terminal,
+		_("The terminal emulator."));
+	gtk_entry_set_text(GTK_ENTRY(e->command_terminal), prj_org->command_terminal);
+
+	gtk_box_pack_start(GTK_BOX(vbox), table, FALSE, FALSE, 6);
+

I'd really like this to be a plugin configuration instead of a project configuration or not configurable at all. If you don't want to make it part of this pull request, I'd actually prefer if you could remove the configuration code from this pull request.

> +}
+
+
+void on_open_terminal(G_GNUC_UNUSED GtkMenuItem * menuitem, G_GNUC_UNUSED gpointer user_data)
+{
+	gchar *locale_path = NULL;
+	static gchar *open_command = NULL;
+
+	if (!open_command)
+		open_command = g_strdup(PRJORG_COMMAND_TERMINAL);
+
+	if (prj_org)
+	{
+		g_free(open_command);
+		open_command = g_strdup(prj_org->command_terminal);
+	}

In fact, the g_strdup() seems to be unnecessary so it could be just
```
open_command = prj_org ?prj_org->command_terminal : PRJORG_COMMAND_TERMINAL;
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1126#pullrequestreview-802740554
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20211110/cb276afc/attachment-0001.htm>