[Github-comments] [geany/geany] Context menu update (#2550)

[Matthew Brush]

@codebrainz requested changes on this pull request.

Seems reasonable.

Aside from the inline comments, I find the labels to be a little ambiguous, I had to look at the code to see how they were different.

Maybe something like these?
* Copy Basename to Clipboard
* Copy Full Path to Clipboard
* Copy Directory Path to Clipboard

Also the labels should probably have "mnemonics" like the existing items have, like for the above suggested labels, one possibility is to add underscores before B, F and D, respectively.

Only other comment, and maybe it's just how Github is displaying it, but the indentation seems to be too far (ie. 2 tabs where 1 would do), but I haven't checked out the code locally to see if that's actually a problem

> @@ -455,6 +455,30 @@ static void on_close_documents_right_activate(GtkMenuItem *menuitem, GeanyDocume
 }
 
 
+static void on_copy_filename_to_clipboard_activate(GtkMenuItem *menuitem, GeanyDocument *doc)
+{
+		g_return_if_fail(doc->is_valid);
+
+		gtk_clipboard_set_text(gtk_clipboard_get(GDK_NONE), g_path_get_basename(doc->real_path), -1);

`g_path_get_basename()` returns an allocated string that needs to be `g_free()`'d.

> +}
+
+
+static void on_copy_file_path_to_clipboard_activate(GtkMenuItem *menuitem, GeanyDocument *doc)
+{
+		g_return_if_fail(doc->is_valid);
+
+		gtk_clipboard_set_text(gtk_clipboard_get(GDK_NONE), doc->real_path, -1);
+}
+
+
+static void on_copy_file_dir_to_clipboard_activate(GtkMenuItem *menuitem, GeanyDocument *doc)
+{
+		g_return_if_fail(doc->is_valid);
+
+		gtk_clipboard_set_text(gtk_clipboard_get(GDK_NONE), g_path_get_dirname(doc->real_path), -1);

Same as above, need to free result of `g_path_get_dirname()`.

> @@ -509,6 +533,37 @@ static void show_tab_bar_popup_menu(GdkEventButton *event, GeanyDocument *doc)
 	gtk_container_add(GTK_CONTAINER(menu), menu_item);
 	g_signal_connect(menu_item, "activate", G_CALLBACK(on_close_all1_activate), NULL);
 
+	menu_item = gtk_separator_menu_item_new();
+	gtk_widget_show(menu_item);
+	gtk_container_add(GTK_CONTAINER(menu), menu_item);
+
+		menu_item = ui_image_menu_item_new(GTK_STOCK_PASTE, _("Copy filename to clipboard"));

I notice the existing menu items use Title Case for the labels, like "Copy Filename to Clipboard".

-- 
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/pull/2550#pullrequestreview-451009724
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20200717/639804fa/attachment.htm>