I'm not sure about that. Probably you think about a common use case for you, but I don't think is applicably globally. For example pictures from a phone: IMG_23_whatever.

The default name is "Untitled folder" having it be "IMG_23_" is marginally better than "Untitled folder". It's plenty useful when number items are selected, and, in the worst case, you fallback to "Untitled folder".

Fair enough.

Bug marked as gnome-love since what is needed is more or less just copy code from the link Bastien posted

Could be good to keep in mind the two features https://bugzilla.gnome.org/show_bug.cgi?id=747752

Created attachment 325260 [details] [review]
files-view: change "New Folder with Selection" to offer a better default name

It now looks for a common filename prefix of the selected files, and pre-populates the folder name entry with that. If no common prefix is found that is greater than 3 characters long, the folder name entry will be left blank

I decided to re-write get_filename_from_list (nautilus-sendto) in the end. A large chunk of it wasn't necessary, and it also called g_filename_from_uri, g_path_get_basename and g_filename_to_utf8 for each file, at every character. That's fine for Send To when you wouldn't normally send many files, but for this case it doesn't hurt to be a bit more efficient. E.g. in Bastien's use case, you're likely to have many photos selected.

Review of attachment 325260 [details] [review]:

The code works, but you should try not to have the string finishes with either a portion of the suffix:
foo.dsl + foo.dat = foo.d
Or punctuation:
foo-test.txt + foo-log.txt = foo-

::: src/nautilus-files-view.c
@@ +22,3 @@
  *          Pavel Cisler <pavel@eazel.com>,
+ *          David Emory Watson <dwatson@cs.ucr.edu>,
+ *          Neil Herald <neil.herald@gmail.com>

This isn't updated. Your name will already be in the commit log, and the generated ChangeLog, no need to add it here.

@@ +2084,3 @@
+ * get_common_prefix_size:
+ *
+ * @str_a first string

If you're going to document your code, please follow the doc style, there's a ":" after the variable name.

@@ +2097,3 @@
+                        int   min_required_len)
+{
+        int a_len = g_utf8_strlen (str_a, -1);

Don't mix declarations with function calls.

@@ +2103,3 @@
+                return -1;
+
+        int matching_chars = 0;

Declarations at the top of the function.

@@ +2132,3 @@
+get_filename_common_prefix (GList *file_list)
+{
+        GList *l = file_list;

That's not how we usually iterate over lists.

@@ +2147,3 @@
+        g_free (name);
+
+        while ((l = l->next) != NULL) {

for (l = file_list; l != NULL; l = l->next) {
...

And you can check if it's the first in the list with:
if (list->prev == NULL) {

@@ +2156,3 @@
+                }
+
+                int matching_bytes = get_common_prefix_size (common_part->str, name, 4);

Put the constant as a #define at the top of the file, with a clear name.

@@ +2209,3 @@
+        selection = nautilus_view_get_selection (NAUTILUS_VIEW (view));
+        common_prefix = get_filename_common_prefix (selection);
+        if (common_prefix != NULL) {

no need for braces for a one-line conditional.

@@ +2210,3 @@
+        common_prefix = get_filename_common_prefix (selection);
+        if (common_prefix != NULL) {
+                gtk_entry_set_text (GTK_ENTRY(widget_data->name_entry), common_prefix);

spaces before parens.

Review of attachment 325260 [details] [review]:

Thanks Bastien for reviewing, just a correction:
"no need for braces for a one-line conditional."
Nautilus does it, but I'm fine either way.

Also, I think you can avoid to use g_string at all, and the code will look clearer with the corrections below:

::: src/nautilus-files-view.c
@@ +331,3 @@
+int             get_common_prefix_size                         (char *str_a,
+                                                                char *str_b,
+                                                                int   min_required_len);

Align the parameters with previous function parameters as well.

@@ +2104,3 @@
+
+        int matching_chars = 0;
+        char *a = str_a, *b = str_b;

One declaration per line

@@ +2118,3 @@
+                return -1;
+
+        int matching_bytes = a - str_a;

new line before return

@@ +2119,3 @@
+
+        int matching_bytes = a - str_a;
+        return matching_bytes;

I think would clearer if you return the position, not the size.

@@ +2164,3 @@
+                }
+
+                g_string_truncate (common_part, matching_bytes);

If you return the position, you can use g_utf8_substring here, which looks clearer and will allow you to not use g_string at all.

Also the commit message doesn't follow the guidelines. Please refer to:
https://wiki.gnome.org/Newcomers/CodeContributionWorkflow#Commit_guidelines

Created attachment 325510 [details] [review]
files-view: change "New Folder with Selection" to offer a better default name

New Folder with Selection currently doesn't offer a folder name. It
would be better if it suggested a folder name based on the files that
are selected.

With this change, it now looks for a common filename prefix of the
selected files, and pre-populates the folder name entry with that. If no
common prefix is found that is greater than 3 characters long, the
folder name entry will be left blank.

Thanks for the feedback folks, I've made those changes. In general is GString something that should be avoided, or was it just for this case? I'm not disagreeing with the decision, it's just so I know in future (I agree with using offsets rather than number of bytes in this case). 

E.g. this seems a bit neater and less error prone: 

GString *str;
str = g_string_new ("xyz");
...
g_string_something (...);


Than:
char *str;
char *temp;
str = "xyz";
...
temp = g_utf8_something (str, ...);
g_free (str);
str = temp;

Review of attachment 325510 [details] [review]:

A beautiful patch. Congrats and thanks!

You are right, with all the interaction happening here is probably nicer to use GString.

Usually we avoid it if not necessary, like in the previous patch I believe.
In this case, with the addition of subtracting the file type etc, I agree it's better GString.

Also a small tip:

::: src/nautilus-files-view.c
@@ +2204,3 @@
+        for (l = file_list; l != NULL; l = l->next) {
+
+                return g_strdup (filename);

probably we don't want to crash the whole nautilus for this. You can do something like:

g_return_val_if_fail (NAUTILUS_IS_FILE (l->data), NULL);

Created attachment 325625 [details] [review]
files-view: change "New Folder with Selection" to offer a better default name

New Folder with Selection currently doesn't offer a folder name. It
would be better if it suggested a folder name based on the files that
are selected.

With this change, it now looks for a common filename prefix of the
selected files, and pre-populates the folder name entry with that. If no
common prefix is found that is greater than 3 characters long, the
folder name entry will be left blank.

Cheers :) I've changed that and re-uploaded the patch, in case you want to pull it in

Review of attachment 325625 [details] [review]:

Hey! Cool.
You didn't come back to use GString then? In case I was not clear I agreed with you that with all these interactions it will look better with GString.
Looks fine either way though.
Also some small nitpicks pointed below.

::: src/nautilus-files-view.c
@@ +130,3 @@
 #define FLOATING_BAR_LOADING_DELAY 200 /* ms */
 
+#define MIN_COMMON_FILENAME_PREFIX_LEN 4

MIN_COMMON_FILENAME_PREFIX_LENGTH

@@ +2142,3 @@
+
+        return g_utf8_substring (filename, 0, g_utf8_pointer_to_offset (filename, dot_pos));
+}

Forgot to mention we already have a function for this: 
https://git.gnome.org/browse/nautilus/tree/eel/eel-vfs-extensions.c#n159
An example of use:
https://git.gnome.org/browse/nautilus/tree/src/nautilus-files-view.c#n2060

Created attachment 325747 [details] [review]
files-view: change "New Folder with Selection" to offer a better default name

New Folder with Selection currently doesn't offer a folder name. It
would be better if it suggested a folder name based on the files that
are selected.

With this change, it now looks for a common filename prefix of the
selected files, and pre-populates the folder name entry with that. If no
common prefix is found that is greater than 3 characters long, the
folder name entry will be left blank.

No problem - it's not nitpicking, I'd rather get it right too :)

I thought there might be something to strip the extension, I spotted another eel one but it didn't support utf8.

Have stuck with char*. With gstring in this case I would end up having to free the pointer returned by the string functions, so it's the same number of lines.

Review of attachment 325747 [details] [review]:

LGTM now! Great patch, thanks!

Could we also get a regression test for the directory name computation? I used this patch yesterday to tidy videos, and there seems to have been a few weird cases, and I'd like us to be able to support those without introducing regressions.

Sounds like a good idea. Code looks good to me, but it's hard to catch the corner cases...

I've got a big bunch of files I used to test all of the main/corner cases, but that's not very useful. 

Normally I'd write unit tests for code like this, as it can be tested in isolation. If someone can give me some pointers on how I'd get unit tests to run with the build, I'll happily write my test cases as unit tests. I noticed there are some tests under the test folder, but it looked like these are manually run - or is that how it's done in C?

(In reply to Neil Herald from comment #21)
> I've got a big bunch of files I used to test all of the main/corner cases,
> but that's not very useful. 
> 
> Normally I'd write unit tests for code like this, as it can be tested in
> isolation. If someone can give me some pointers on how I'd get unit tests to
> run with the build, I'll happily write my test cases as unit tests. I
> noticed there are some tests under the test folder, but it looked like these
> are manually run - or is that how it's done in C?

make distcheck runs them. But can also run them manually from the test folder.

Review of attachment 325747 [details] [review]:

-> reviewed, let's commit it when we have a regression test.

Neil, any progress on that? Do you need any hint?

We can chat on IRC #nautilus at irc.gnome.org in case you do.

Sure. Had a quick look yesterday, seems pretty straightforward with g_test_run etc. Will sort it out when I've got some time, and grab you on irc if necessary.

Created attachment 326154 [details] [review]
files-view: change "New Folder /w Selection" to offer a name

New Folder with Selection currently doesn't offer a folder name. It
would be better if it suggested a folder name based on the files that
are selected.

With this change, it now looks for a common filename prefix of the
selected files, and pre-populates the folder name entry with that. If no
common prefix is found that is greater than 3 characters long, the
folder name entry will be left blank.

Ignore that one - I hadn't pulled the latest changes beforehand. Will resubmit

Created attachment 326155 [details] [review]
files-view: change "New Folder /w Selection" to offer a name

New Folder with Selection currently doesn't offer a folder name. It
would be better if it suggested a folder name based on the files that
are selected.

With this change, it now looks for a common filename prefix of the
selected files, and pre-populates the folder name entry with that. If no
common prefix is found that is greater than 3 characters long, the
folder name entry will be left blank.

There you go. Had to extern the functions to test them, so I've moved them to more sensible places (string ones to eel-string.h, and file level ones to nautilus-file-utilities.h). It didn't make sense to leave them in files-view.h as they have nothing to do with the files view (from an API point of view). 

BTW, there seems to be a change in the one of the recent commits that's broken 'make distcheck' - it started failing after I pulled the latest changes. Also the check-nautilus tests (under src/) didn't pass to start with. I needed to comment those out in Makefile.am to run my tests - otherwise distcheck bombs out at those.

Review of attachment 326155 [details] [review]:

Oh... we are going to have more coverage for the multiple renaming than the whole nautilus :)

Anyway, did you find any regression with this like Bastien pointed out?

Hah, a _bit_ too thorough with the tests :) 

I thought he just meant to have something to regression test with in future. 

I'm aware there are a few odd cases, e.g:
xyza.something.txt + xyza.something.tar => xyza.something
xyza.something.txt + xyza.some => xyza

(inconsistency in what's regarded as the extension) Didn't think it was worth coding for a case like that, as don't often get multiple dots. 

Bastien - was there anything specific you'd like me to add/change?

(In reply to Neil Herald from comment #30)
> Hah, a _bit_ too thorough with the tests :) 
> 
> I thought he just meant to have something to regression test with in future. 
> 
> I'm aware there are a few odd cases, e.g:
> xyza.something.txt + xyza.something.tar => xyza.something
> xyza.something.txt + xyza.some => xyza
> 
> (inconsistency in what's regarded as the extension) Didn't think it was
> worth coding for a case like that, as don't often get multiple dots. 
> 
> Bastien - was there anything specific you'd like me to add/change?

From the top of my head:
- "extensions" on directories are getting clipped, eg. xyza.something.foo
- some mixed case names are a little bizarre, eg. Xyza.Something.txt and Xyza.something-2.txt

I'd need to test again to give you a more complete list. This shouldn't stop this code from landing though. I was planning on filing new bugs for those cases.

I'm fine with that - I think there's more than enough in this commit already.

Review of attachment 326155 [details] [review]:

Excellent patch, this is how it should always done.

There are some small style nitpicks, feel free to commit otherwise. Do you have commit rights? If not, I think it's time to have them.
Also, I need to fix distcheck for merging this patch, so it will take me some time :/

::: libnautilus-private/nautilus-file-utilities.c
@@ +1177,3 @@
+char*
+nautilus_get_common_filename_prefix_from_filenames (GList *filenames,
+                                                    int min_required_len)

argument aligment missing

::: src/nautilus-files-view.c
@@ +2035,3 @@
 }
 
+

extra line

Sounds good :) I don't have commit rights yet, just let me know what I need to do

I've made those changes locally, will commit them when distcheck is happy

Ah, I spotted this: https://wiki.gnome.org/AccountsTeam/NewAccounts, so have followed the steps there. Ta

Comment on attachment 326155 [details] [review]
files-view: change "New Folder /w Selection" to offer a name

Attachment 326155 [details] pushed as ca0e00b - files-view: change "New Folder /w Selection" to offer a name

Thanks for fixing distcheck. Thought it was time to commit this (it wasn't commit-now, but you said it was OK to commit)

This commit broke the build in Continuous because it's using C99:

../../test/test-file-utilities-get-common-filename-prefix.c: In function 'test_many_strings':
../../test/test-file-utilities-get-common-filename-prefix.c:366:9: error: 'for' loop initial declarations are only allowed in C99 or C11 mode
         for (int i = 0; i < 500; ++i) {
         ^
../../test/test-file-utilities-get-common-filename-prefix.c:366:9: note: use option -std=c99, -std=gnu99, -std=c11 or -std=gnu11 to compile your code
../../test/test-file-utilities-get-common-filename-prefix.c: In function 'test_many_strings_last_differs':
../../test/test-file-utilities-get-common-filename-prefix.c:385:9: error: 'for' loop initial declarations are only allowed in C99 or C11 mode
         for (int i = 0; i < 500; ++i) {
         ^
../../test/test-file-utilities-get-common-filename-prefix.c: In function 'test_many_strings_first_differs':
../../test/test-file-utilities-get-common-filename-prefix.c:409:9: error: 'for' loop initial declarations are only allowed in C99 or C11 mode
         for (int i = 0; i < 500; ++i) {
         ^
Makefile:726: recipe for target 'test-file-utilities-get-common-filename-prefix.o' failed

Created attachment 331160 [details] [review]
tests: Fix build failure caused by C99 use

Pushed this to master.

Thanks Emmanuele

Thanks Neil and Emmanuelle