GNOME Bugzilla – Bug 379778
Save All Files touches unmodified files
Last modified: 2013-11-23 14:22:41 UTC
Please describe the problem: When editing a large code project, I usually have a lot of files open. It's convenient to save all files at once when I want to build the project. However, gedit also saves unmodified files, causing my build system to build stuff unnecessarily. Steps to reproduce: 1. Open a file in gedit 2. Press Ctrl-Shift-L to save all files Actual results: The file's mtime changes to the current time Expected results: The file's mtime should remain unchanged since the file wasn't modified Does this happen every time? Yes Other information:
IIRC, "save all" does not save unmodified files, but I should check.
Created attachment 78036 [details] [review] patch
Created attachment 78041 [details] [review] patch better patch: this one is more paranoid and also saves files which have been modified externally[1]. To do that I added a _gedit_document_check_externally_modified utility: the function also takes a res_for_remotes argument so that we can use the same function also in bug 371188, which want a slightly different semantic (here we want to treat remote files as externally modified, while in that bug we want to treat them as not modified). [1] the saving procedure itself will then take care of displayng the confirmation message area
<bugsbot> pbor: Bug 379778 min, Normal, 2.18.0, gedit-maint@gnome.bugs, UNCO, Save All Files touches unmodified files <paolo> pbor: lemme give a look to it <paolo> pbor: I don't like very much the _gedit_document_check_externally_modified function (I mean the res_for_remote stuff) <paolo> note that since we already do the check in another places in the code <paolo> if we add such a function we should change the code to call it <paolo> we should also manage the "deleted" files <pbor> deleted files are already handled (you can't see that in the patch) <paolo> oh, ok <paolo> also readonly ones, right? <pbor> yep <pbor> about that function, I know the semantic is a bit strange... on the other hand I could not find a better solution <pbor> I really don't like the idea of checking if a file is remote caller-side <paolo> what about adding another function <pbor> since it's way more verbose, it opens the possibility of using the sync function on remote files by mistake, and it means copying the uri string and then freeing it every time <paolo> gedit_document_is_local <paolo> so you can check for <pbor> well, it still leaves open th possibility of using check_modified on remote uris by mistake <paolo> save_unmodified || gtk_text_buffer_get_modified (GTK_TEXT_BUFFER (doc)) || (gedit_document_is_local (doc) && gedit_document_get_externally_modified (doc)) <paolo> sure, but it this way we have a more flexible API <pbor> I would prefer to not make possible to do a sync stat on remote ursi <pbor> *uris <paolo> I'd say gedit_document_is_local can be public <paolo> while the other one not <paolo> being private, we can put a big warning in the doc <pbor> ok, if you prefer that it's fine by me <paolo> or simply make it return TRUE <paolo> for remote files <paolo> yep, I'd say <pbor> well, the whole point of my function is that returning TRUE (or FALSE) really depends on the context <paolo> sure, but having the is_local function we can use it for manage the context in the right way <pbor> sure <paolo> note that is_local can be useful for plugins, etc. <pbor> yep <paolo> and since we are using local files <paolo> may be we can use fstat <pbor> what you mean? <pbor> as opposed to stat? <pbor> or what? <pbor> we don't have the fd already open <pbor> or you mean using (f)stat instead of gnome_vfs_info? <paolo> I mean stat instead of gnome_vfs_info <paolo> but it is not a big problem <paolo> BTW, looking at the code it does not seems deleted documents are managed <paolo> may be check_externally modified should return TRUE when getting info fails <paolo> and so the document has been probably deleted <paolo> you should also manage the case of externally modified readonly files <paolo> hmmm... may be not
What's the status of this bug?? this sounds like a really important fix. Mikko, can you test this again with the latest Gnome??
As of gedit 2.20.4 (current version in Debian unstable), this bug still exists. The save_unmodified flag in the IRC discussion looks related, but I couldn't find a checkbox controlling it anywhere in the preferences dialog.
I confirm this bug with gedit 2.26.1. It doesn't affect only mtime, but also usability because when we have a lot of opened files, the "Save All" call takes a long time to run because all files are saved even if only some are modified.
I confirm this bug with gedit 2.29.5, and I can second #7's usability concern. Moreover, if one uses the incremental search as often as I do, and sometimes incorrectly press the default shortcut key for save-all (Ctrl-L) instead of the one to clear the incremental search query (Ctrl-K), it becomes a real usability issue if having e.g. 100 documents open.
(In reply to comment #8) > I confirm this bug with gedit 2.29.5, and I can second #7's usability concern. > Moreover, if one uses the incremental search as often as I do, and sometimes > incorrectly press the default shortcut key for save-all (Ctrl-L) instead of the > one to clear the incremental search query (Ctrl-K), it becomes a real usability > issue if having e.g. 100 documents open. I of course meant to write Ctrl-Shift-L and Ctrl-Shift-K in the above.
Created attachment 261261 [details] [review] Add gedit_document_get_needs_saving Use the new function in place of the previously used helper function document_needs_saving in gedit/gedit-commands-file.c and similar code in gedit/gedit-tab.c to simplify the code. Notice: document_needs_saving didn't check if the doc had been externally modified, which gedit_document_get_needs_saving now does. gedit_document_get_needs_saving does not check remote files for (remote) modification or deletion. The code it replaces didn't do that either. This change currently leaves the gedit_document_get_deleted function unused.
Created attachment 261262 [details] [review] FileSaveAll: Only save modified documents _gedit_cmd_file_save_documents_list: Move the check if saving is needed, so it applies to all documents, not just untitled and readonly ones.
After the code changed quite a bit since the last patches, here are two patches, trying to pick this up again. The first one isn't really necessary for this bug, but the second one, as it is now, applies on top of it. There is still missing a way to force saving unmodified files, especially to save remote files, for which deletion/modification on the remote side isn't detected. But always saving all remote files (when no changes in the gedit buffer have been made) by default seems a bit much(?).
Review of attachment 261261 [details] [review]: I'd say call it _gedit_document_needs_saving, where the leading _ means it is a "private" function not exported to the plugins. Other than that the refactoring looks good. We have to keep get_deleted since I am pretty sure there are plugins using it.
Review of attachment 261262 [details] [review]: Looks good once the prev patch has been fixed up and committed
Created attachment 261294 [details] [review] Add _gedit_document_needs_saving Use the new function in place of the previously used helper function document_needs_saving in gedit/gedit-commands-file.c and similar code in gedit/gedit-tab.c. Notice: document_needs_saving didn't check if the doc had been externally modified, which _gedit_document_needs_saving now does. _gedit_document_needs_saving does not check remote files for external modification or deletion. The code it replaces didn't do that either.
Created attachment 261295 [details] [review] FileSaveAll: Only save modified documents _gedit_cmd_file_save_documents_list: Move the check if saving is needed, so it applies to all documents, not just untitled and readonly ones.
Comment on attachment 261294 [details] [review] Add _gedit_document_needs_saving Attachment 261294 [details] pushed as acbf4d4 - Add _gedit_document_needs_saving
Comment on attachment 261295 [details] [review] FileSaveAll: Only save modified documents Attachment 261295 [details] pushed as a3120f3 - FileSaveAll: Only save modified documents
Thank you! fixed after only 7 years :)