GNOME Bugzilla – Bug 776228
Extracting a file, that contains a "." in it's name, twice adds number preceding the "." found before the "." that separates the extension
Last modified: 2021-06-18 15:49:11 UTC
Generally when an archive file is extracted twice a number is added to the end of the folder created to remove ambiguity. When that archive file has a "." in it's name. (e.g.) test.1.2.3.zip The number added to the remove ambiguity is done before the end of the name (e.g.) test.1.2. (1).3 The name should actually should have been test.1.2.3 (1)
Created attachment 342721 [details] [review] file-utilities: refactor nautilus_generate_unique The problem is when you extract a folder like "test.1.2.3" twice, nautilus thinks that ".3" is an extension, and extracts the folder as "test.1.2 (1).3". The correct name should be "test.1.2.3 (1)". To fix this, add a new parameter "GFileType type" in the nautilus_generate_unique_file_in_directory function, to ensure to generate the correct name, based on the type of file.
Review of attachment 342721 [details] [review]: A few nitpicks, but looks good to me, thanks! ::: src/nautilus-file-operations.c @@ +8175,3 @@ GFile *decided_destination; g_autofree char *basename = NULL; + GFileType type_of_file; No need to be so verbose, “file_type” would do fine here. ::: src/nautilus-file-utilities.c @@ +651,3 @@ nautilus_generate_unique_file_in_directory (GFile *directory, + const char *basename, + const GFileType type) Align the parameter names (by their first letters). @@ +661,3 @@ g_return_val_if_fail (basename != NULL, NULL); g_return_val_if_fail (g_file_query_exists (directory, NULL), NULL); + g_return_val_if_fail (type != NULL, NULL); GFileType is not a pointer and I don’t think a check here is necessary at all. ::: src/nautilus-file-utilities.h @@ +72,3 @@ GFile * nautilus_generate_unique_file_in_directory (GFile *directory, + const char *basename, + const GFileType type); Alignment issues here as well.
(In reply to Ernestas Kulik from comment #2) > Review of attachment 342721 [details] [review] [review]: > > A few nitpicks, but looks good to me, thanks! > > ::: src/nautilus-file-operations.c > @@ +8175,3 @@ > GFile *decided_destination; > g_autofree char *basename = NULL; > + GFileType type_of_file; > > No need to be so verbose, “file_type” would do fine here. > > ::: src/nautilus-file-utilities.c > @@ +651,3 @@ > nautilus_generate_unique_file_in_directory (GFile *directory, > + const char *basename, > + const GFileType type) > > Align the parameter names (by their first letters). > > @@ +661,3 @@ > g_return_val_if_fail (basename != NULL, NULL); > g_return_val_if_fail (g_file_query_exists (directory, NULL), NULL); > + g_return_val_if_fail (type != NULL, NULL); > > GFileType is not a pointer and I don’t think a check here is necessary at > all. > > ::: src/nautilus-file-utilities.h > @@ +72,3 @@ > GFile * nautilus_generate_unique_file_in_directory (GFile *directory, > + const char *basename, > + const GFileType type); > > Alignment issues here as well. Thanks for the quick review. I'm new on BugZilla, so I have to submit the patch, another time with your corrections ?. Instead of that, I will take account alignments in functions calls, for the next patch.
(In reply to Kevin Lopez from comment #3) > Thanks for the quick review. I'm new on BugZilla, so I have to submit the > patch, another time with your corrections ? Yup, just submit as you did at first, but mark the old patch as obsolete (in the attachment details). You can also use git-bz to simplify your workflow a little (https://wiki.gnome.org/Newcomers/CodeContributionWorkflow).
Created attachment 342727 [details] [review] file-utilities: refactor nautilus_generate_unique The problem is when you extract a folder like "test.1.2.3" twice, nautilus thinks that ".3" is an extension, and extracts the folder as "test.1.2 (1).3". The correct name should be "test.1.2.3 (1)". To fix this add a new parameter "GFileType type" to the nautilus_generate_unique_file_in_directory function, to ensure generate the correct name based in the type of file.
Review of attachment 342727 [details] [review]: Almost there! ::: src/nautilus-file-operations.c @@ +8186,3 @@ decided_destination = nautilus_generate_unique_file_in_directory (extract_job->destination_directory, + basename, + file_type); No tabs. Expand them to four spaces. ::: src/nautilus-file-utilities.c @@ +650,3 @@ GFile * +nautilus_generate_unique_file_in_directory (GFile *directory, + const char *basename, Replace tabs here as well. ::: src/nautilus-file-utilities.h @@ +72,3 @@ +GFile * nautilus_generate_unique_file_in_directory (GFile *directory, + const char *basename, + const GFileType type); Tabs tabs tabs. ::: src/nautilus-link.c @@ +229,3 @@ file = nautilus_generate_unique_file_in_directory (directory, + link_name, + G_FILE_TYPE_UNKNOWN); More tabs.
(In reply to Ernestas Kulik from comment #6) > No tabs. Expand them to four spaces. This will be the correct configuration for vim ? from my init.vim set tabstop=4 " Number of visual spaces per TAB set softtabstop=4 " Number of spaces in tab when editing set shiftwidth=4 " When indenting with '>', use 4 spaces width set expandtab " On pressing tab, insert 4 spaces
(In reply to Kevin Lopez from comment #7) > This will be the correct configuration for vim ? Sure, that’s what I use, too. Many other projects (including some GNOME modules) use actual tabs, so be prepared to change those settings on the fly when working on something other than Nautilus. :)
(In reply to Ernestas Kulik from comment #8) > (In reply to Kevin Lopez from comment #7) > > This will be the correct configuration for vim ? > > Sure, that’s what I use, too. > > Many other projects (including some GNOME modules) use actual tabs, so be > prepared to change those settings on the fly when working on something other > than Nautilus. :) So, you can change the docs/style-guide.hmtl ? because It says "We use the most-recommended coding style from the GNOME Programming Guidelines.This means that we use the Linux kernel brace style with 8-character tabs", and for newcomers like me I think that will be useful know the correct guidelines.
(In reply to Kevin Lopez from comment #9) > So, you can change the docs/style-guide.hmtl ? because It says "We use the > most-recommended coding style from the GNOME Programming > Guidelines.This means that we use the Linux kernel brace style with > 8-character tabs", and for newcomers like me I think that will be useful > know the > correct guidelines. I’ll coordinate the changes in documentation with the maintainer once he’s available (many of the changes are actually fairly recent - we’re trying to work out a style for use throughout the codebase). Thanks for reminding me.
(In reply to Ernestas Kulik from comment #10) > we’re trying to work out a style for use throughout the codebase Well, a /new/ style.
Created attachment 342740 [details] [review] file-utilities: refactor nautilus_generate_unique The problem is when you extract a folder like "test.1.2.3" twice, nautilus thinks that ".3" is an extension, and extracts the folder as "test.1.2 (1).3". The correct name should be "test.1.2.3 (1)". To fix this add a new parameter "GFileType type" to the nautilus_generate_unique_file_in_directory function, to ensure generate the correct name based in the type of file.
This plugin that I forked, may be helpful for other vim users. Enables automatically: set tabstop=4 set softtabstop=4 set shiftwidth=4 set expandtab when you are in Nautilus directory. https://github.com/kevinlopezandrade/nautilus-coding-style
Review of attachment 342740 [details] [review]: Hey Kevin, thanks for the patch! Still needs some work: ::: src/nautilus-file-operations.c @@ +8182,3 @@ + file_type = g_file_query_file_type (destination, + G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS, + NULL); This is synchronous and we are in the UI thread. You need to do it asynchronous. You can look at examples in this same file, getting the file info. ::: src/nautilus-file-utilities.c @@ +651,3 @@ +nautilus_generate_unique_file_in_directory (GFile *directory, + const char *basename, + const GFileType type) one space more before parameters. Looks other functions (or run the code formatting script at /data called run-uncrustify.sh) ::: src/nautilus-file-utilities.h @@ +72,3 @@ +GFile * nautilus_generate_unique_file_in_directory (GFile *directory, + const char *basename, + const GFileType type); needs a space more to all parameters
Created attachment 342911 [details] [review] patch Obtain the GFileInfo was my first approachment to solve the bug. I saw the other implementations in the file but in this case, I'm not consider useful add a if checking the info pointer. Correct me if I wrong. Also I saw in the glib documentation a function "g_file_query_info_async()", but as you said that I have to see the source file, I didn't used.
Review of attachment 342911 [details] [review]: "I saw the other implementations in the file but in this case, I'm not consider useful add a if checking the info pointer. Correct me if I wrong. Also I saw in the glib documentation a function "g_file_query_info_async()", but as you said that I have to see the source file, I didn't used." Not sure I understand the comment, we cannot do any sync operation in the UI/main thread, so we need to make it async. Other parts of the patch looks fine now :)
A similar bug - https://bugzilla.gnome.org/show_bug.cgi?id=780326
`extract_job_on_decide_destination` is `static GFile*`, so this callback should return correct GFile object. `g_file_query_info_async` should take `void (*GAsyncReadyCallback)`, and if we will do callback inside the callback, it would be incorrect, isn't it? If `extract_job_on_decide_destination` would be `void`, then `g_file_query_info_async` would be work correctly, I think.
(In reply to Evgeny Shulgin from comment #18) Hi, thanks for commenting in this bug, I totally forgot it. > would be incorrect, isn't it? If `extract_job_on_decide_destination` would > be `void`, then `g_file_query_info_async` would be work correctly, I think. We cannot change the function signature to return a void signal, because as you can see in the Gnome Autoar source code https://github.com/GNOME/gnome-autoar/blob/master/gnome-autoar/autoar-extractor.c#L1376 the handler of this signal should return a GObject (in this case a GFile), which will be used to continue the execution of the extract process. Reading the source code to try fix the bug I realized that it's more complex than it seemed at first, at least for me (I'm a newcomer), because the g_file_query_async, in his implementation runs a new thread by means of g_task_run_in_thread, so we can't know when this new thread will finish since is concurrently executed. The problem is that the thread that calls g_file_query_async should continue his execution (we are in UI thread as Carlos Soriano said in comment 16), and the next step of this thread after handling the "decided-destination", is iterate among all the output_files. What can happens if the g_file_query_info_async function not ends before we iterate over all output_files ?. All of this leads to a non deterministic execution. Note: I'm not sure if the above explanation is correct, so I think that some help from maintainers would be good to try to solve this bug. Regards!
(In reply to Kevin Lopez from comment #19) > We cannot change the function signature to return a void signal, because as *to return void.
Created attachment 349236 [details] [review] Bugfix
Review of attachment 349236 [details] [review]: Seems to work fine, thanks for your work! I believe we could make the code a little neater, however. ::: src/nautilus-file-utilities.c @@ +669,3 @@ + { + nautilus_file = nautilus_file_get_existing (child); + ignore_extension = nautilus_file_is_directory (nautilus_file); I believe that g_file_query_type () would work better here. That would avoid having to get the NautilusFile. Additionally, this allows you to forget about having to check if the file exists, since for non-existent files that method returns G_FILE_TYPE_UNKNOWN.
(In reply to Ernestas Kulik from comment #22) > Review of attachment 349236 [details] [review] [review]: > > I believe that g_file_query_type () would work better here. That would avoid > having to get the NautilusFile. Additionally, this allows you to forget > about having to check if the file exists, since for non-existent files that > method returns G_FILE_TYPE_UNKNOWN. After reading the entire thread about this issue, I realized that we should not use g_file_query_type() in the UI thread. See 'Comment 16'. g_file_query_type_async() is also unacceptable, see 'Comment 19' (since we can't launch new thread, when we should return value right now in the current thread). So, this patch is unacceptable - https://bugzilla.gnome.org/attachment.cgi?id=342727&action=diff, but my patch could be acceptable, because I see at the existing NautilusFile-s.
(I meant, see Comment 14 about g_file_query_type () >This is synchronous and we are in the UI thread. You need to do it asynchronous. You can look at examples in this same file, getting the file info. ) And nautilus_file_get_existing() searches for this synchronously, using g_hash_table_lookup()
(In reply to Evgeny Shulgin from comment #23) > After reading the entire thread about this issue, I realized that we should > not use g_file_query_type() in the UI thread. See 'Comment 16'. > g_file_query_type_async() is also unacceptable, see 'Comment 19' (since we > can't launch new thread, when we should return value right now in the > current thread). Ah, yes, my bad. > So, this patch is unacceptable - > https://bugzilla.gnome.org/attachment.cgi?id=342727&action=diff, but my > patch could be acceptable, because I see at the existing NautilusFile-s. It should be noted that g_file_query_exists () also does blocking I/O. I’m very skeptical about using nautilus_file_get_existing () in a function that primarily deals with GFiles. Changing that to nautilus_file_get () would be even more troublesome, since the attributes might not be ready if this is a new NautilusFile. Carlos, am I crazy? (In reply to Evgeny Shulgin from comment #24) > (I meant, see Comment 14 about g_file_query_type () > >This is synchronous and we are in the UI thread. You need to do it asynchronous. You can look at examples in this same file, getting the file info. > ) > > And nautilus_file_get_existing() searches for this synchronously, using > g_hash_table_lookup() Hash table lookup is /at worst/ linear and will be noticeable with very large quantities. You shouldn’t confuse this with synchronous I/O.
(In reply to Ernestas Kulik from comment #25) > It should be noted that g_file_query_exists () also does blocking I/O. Yes, I just noticed, this is bad. I will rewrite part of the code to deal with it. > I’m very skeptical about using nautilus_file_get_existing () in a function > that primarily deals with GFiles. It's because "decide-destination" come from gnome-autoar https://github.com/GNOME/gnome-autoar/blob/master/gnome-autoar/autoar-extractor.c#L1376 Handler of this signal should return GFile, and, of course, since autoar does not know about NautilusFile and similar structures (but in Nautilus source code there are NautilusXXX structures in use in almost all cases) we get the mix of the code. > Hash table lookup is /at worst/ linear and will be noticeable with very > large quantities. You shouldn’t confuse this with synchronous I/O. Afaik from the theory of algorithms, elementary linear search will work absolutely ok. If we have 100000 strings (the names of the files) with average length = 20, we will find the entry in a few milliseconds. But I don't know if Nautilus can even open the folder with 100000 files without "explosions" :) So we can don't look at this hash table. Thanks for your answers!
Created attachment 349749 [details] [review] Patch (more async version) Removed g_file_query_exists(), now it's IO-blocking free (I guess) P.S. I can't find function like 'nautilus_file_is_exists(GFile*)', so I used this code to detect if file is exists: nautilus_file = nautilus_file_get_existing (child); if (nautilus_file != NULL) { ... } Of course I checked 'nautilus_file_get_existing' source and realized that if Nautilus don't know this file, this func will return NULL.
Review of attachment 349749 [details] [review]: Hey all So I have to rectify something. We were already blocking the UI thread, but in file-operation we are not in the UI thread so we didn't notice, but nautilus-link (the other using this function) actually does block. Since this was already the case, there is not need to modify it. What I would do is a similar patch as Kevin did, but doing the type query in file-utilities, rename the function to be nautilus_generate_unique_file_in_directory_sync, and that's it. You shouldn't use here the NautilusFile type here because that require querying the info from the disk, but it's not done in here. Instead we should query just what we are interested in (directory vs file) and using directly GFile.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version of Files (nautilus), then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/nautilus/-/issues/ Thank you for your understanding and your help.