<p></p>
<p><b>@techee</b> requested changes on this pull request.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/1126#discussion_r746696905">projectorganizer/src/prjorg-sidebar.c</a>:</p>
<pre style='color:#555'>> + 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)
</pre>
<p dir="auto">g_free() works on NULL pointers, the if is unnecessary here</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/1126#discussion_r746698772">projectorganizer/src/prjorg-sidebar.c</a>:</p>
<pre style='color:#555'>> +}
+
+
+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);
+ }
</pre>
<p dir="auto">Personally I'd just avoid the static variable and would do</p>
<pre><code>open_command = prj_org ? g_strdup(prj_org->command_terminal) : g_strdup(PRJORG_COMMAND_TERMINAL);
</code></pre>
<p dir="auto">and would free this at the end of the function.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/1126#discussion_r746703031">projectorganizer/src/prjorg-sidebar.c</a>:</p>
<pre style='color:#555'>> +}
+
+
+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);
+ }
</pre>
<p dir="auto">like in on_open_terminal(), avoid the static and extra allocation.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/1126#discussion_r746726900">projectorganizer/src/prjorg-sidebar.c</a>:</p>
<pre style='color:#555'>> + }
+ }
+
+ /* 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;
+}
</pre>
<p dir="auto">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.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/1126#discussion_r746728242">projectorganizer/src/prjorg-sidebar.c</a>:</p>
<pre style='color:#555'>> @@ -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"));
</pre>
<p dir="auto">The name should be an action, so better to call it "Open File Manager".</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/1126#discussion_r746728464">projectorganizer/src/prjorg-sidebar.c</a>:</p>
<pre style='color:#555'>> @@ -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"));
</pre>
<p dir="auto">"Open Terminal"</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/1126#discussion_r746733301">projectorganizer/src/prjorg-project.c</a>:</p>
<pre style='color:#555'>> + 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);
+
</pre>
<p dir="auto">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.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/1126#discussion_r746701241">projectorganizer/src/prjorg-sidebar.c</a>:</p>
<pre style='color:#555'>> +}
+
+
+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);
+ }
</pre>
<p dir="auto">In fact, the g_strdup() seems to be unnecessary so it could be just</p>
<pre><code>open_command = prj_org ?prj_org->command_terminal : PRJORG_COMMAND_TERMINAL;
</code></pre>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/geany/geany-plugins/pull/1126#pullrequestreview-802740554">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAIOWJ4LQ5BJJOYV2EQVKBTULKJD3ANCNFSM5GTVPZ4Q">unsubscribe</a>.<br />Triage notifications on the go with GitHub Mobile for <a href="https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675">iOS</a> or <a href="https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub">Android</a>.
<img src="https://github.com/notifications/beacon/AAIOWJZICE2OBXHMCWL4EXTULKJD3A5CNFSM5GTVPZ42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOF7MNSSQ.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/geany/geany-plugins/pull/1126#pullrequestreview-802740554",
"url": "https://github.com/geany/geany-plugins/pull/1126#pullrequestreview-802740554",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>