Created attachment 315075 [details] [review]
Patch that add custom shortcuts support to launch scripts

Review of attachment 315075 [details] [review]:

Thanks for the revised patch; please find my comments inline.

::: src/nautilus-view.c
@@ +4229,3 @@
+static void
+nautilus_load_custom_accel_for_scripts(){
+	gchar *path = g_strconcat(g_get_home_dir(),SHORTCUTS_PATH,NULL);

Coding style: add a space before parenthesis for function calls (multiple occurrences)

@@ +4233,3 @@
+	GError *error = NULL;
+	if (g_file_get_contents(path, &contents, NULL, &error)) {
+		gchar **lines = g_strsplit(contents, "\n", -1);

lines is leaked

@@ +4239,3 @@
+		for(i=0; strstr(lines[i]," ") > 0; i++) {
+			gchar **result = g_strsplit(lines[i], " ", 2);
+	if (g_file_get_contents(path, &contents, NULL, &error)) {

You should declare the hash table with g_hash_table_new_full() and pass g_free() as GDestroyNotify functions for both the keys and the values.

@@ +4243,3 @@
+		}
+	} else
+		g_free(contents);

error is guaranteed to be != NULL if g_file_get_contents() returns FALSE

@@ +4268,3 @@
 
+	if(script_accels == NULL) script_accels = g_hash_table_new(g_str_hash,g_str_equal);
+	nautilus_load_custom_accel_for_scripts();

As far as I can see, here you're populating the hash table every time this function is called. Instead, it should only ever be parsed once.

@@ +4270,3 @@
+	nautilus_load_custom_accel_for_scripts();
+	
+    file_list = nautilus_directory_get_file_list (directory);

No indentation changes please

Ok, I fixed the patch as you suggested

Created attachment 315117 [details] [review]
Previous patch with coding style and bugs fixed

Review of attachment 315117 [details] [review]:

Thank you for the updated patch, we're getting there!
I have a few more minor comments.

::: src/nautilus-view.c
@@ +115,3 @@
 #define TEMPLATE_LIMIT 30
 
+#define SHORTCUTS_PATH "/.config/nautilus/shortcuts"

The file should be called "scripts-accels" or something like that. "shortcuts" is too generic.

@@ +4228,3 @@
 
+static void
+nautilus_load_custom_accel_for_scripts (){

Missing void parameter; also missing space before curly brace

@@ +4229,3 @@
+static void
+nautilus_load_custom_accel_for_scripts (){
+	gchar *path = g_strconcat (g_get_home_dir (), SHORTCUTS_PATH, NULL);

Should use g_build_filename() and g_get_user_config_dir()

@@ +4231,3 @@
+	gchar *path = g_strconcat (g_get_home_dir (), SHORTCUTS_PATH, NULL);
+	gchar *contents;
+	GError *error = NULL;

Nitpick: add space here

@@ +4235,3 @@
+		gchar **lines = g_strsplit (contents, "\n", -1);
+		g_free (contents);
+	gchar *contents;

Why do you need the max length at all?

@@ +4236,3 @@
+		g_free (contents);
+		const int max_len = 100;
+	GError *error = NULL;

Nitpick: move variable declarations at the top

@@ +4237,3 @@
+		const int max_len = 100;
+		int i;
+	GError *error = NULL;

Missing space before and after =, and between the two parameters of strstr().
Also I think we need a check for lines[i] != NULL too.

@@ +4239,3 @@
+		for(i=0; strstr (lines[i]," ") > 0; i++) {
+			gchar **result = g_strsplit (lines[i], " ", 2);
+	if (g_file_get_contents (path, &contents, NULL, &error)) {

I was imagining that the accel file was of the format

script_name accel
script_name2 another_accel

instead of

accel script_name
another_accel script_name2

Any reason to prefer one over the other? In any case, there should be a comment before this function that explains the expected format.

@@ +4267,3 @@
 	g_return_val_if_fail (NAUTILUS_IS_DIRECTORY (directory), NULL);
 
+	if(script_accels == NULL) {

Missing space after if

Created attachment 315169 [details] [review]
[Fixed] Patch that add custom shortcuts support to launch scripts

Ok, I followed your precious suggestions again, but I can't understand "Nitpick: add space here", where?

I think it's better to use:
accel script_name
another_accel script_name2

because the script names can have a big length difference, instead the accels have often a small length.
Example format 1:
<control>h script_with_a_very_long_long_long_long_long_long_name
F2 short_name

Example format 2:
script_with_a_very_long_long_long_long_long_long_name <control>h
short_name F2

I prefer the appearance of format 1.

Review of attachment 315169 [details] [review]:

Looking almost ready to go - see minor comments below.

::: src/nautilus-view.c
@@ +4193,3 @@
+    
+	gchar *shortcut = NULL;
+	if (shortcut = g_hash_table_lookup(script_accels,name)) {

Missing space before parenthesis, and missing space after comma between the parameters.

@@ +4194,3 @@
+	gchar *shortcut = NULL;
+	if (shortcut = g_hash_table_lookup(script_accels,name)) {
+		nautilus_application_add_accelerator(g_application_get_default(),detailed_action_name,shortcut);       

See above

@@ +4234,3 @@
+	gchar *contents;
+	GError *error = NULL;
+	const int max_len = 100;

Why do you need a max length?

@@ +4239,3 @@
+		gchar **lines = g_strsplit (contents, "\n", -1);
+		g_free (contents);
+	gchar *contents;

Missing space after "for"

Created attachment 315216 [details] [review]
[Fixed] Patch that add custom shortcuts support to launch scripts

Ok I fixed the spaces.

I use max length because I think it's better don't keep strings longer than required both to avoid a waste of memory and to avoid problems.

If the hashtable contains a long string (potentially very long) and someone in the future will add a new feature that will use this hashtable, maybe this could bring problems (like a buffer overflow).

So I used max_len because there isn't the need to keep strings with a potentially infinite length.

Review of attachment 315216 [details] [review]:

This looks good to me now, thanks for your work!

thank you for your support :)

Pushed to master.

commit: https://git.gnome.org/browse/nautilus/commit/?id=9fe1335

Cool, thanks Devim!