Hi guys!
I found a bug in project creation (and save) check: if the project path is a writable directory, update_config() won't see that write will fail, and no error at all will be reported to the user (though write will fail on a directory).
I fixed this (in the attached patch) by:
1) check whether the path is not a directory, because utils_is_file_writeable() [1] returns TRUE in this case, but write will still fail. Perhaps it should rather be fixed in utils_is_file_writeable()?
2) check the return value of write_config(). This needed a little more tuning for later calls to update_config() to work correctly, but now if write actually fails, and even if update_config() reported no error, an error will be displayed to the user and she will have a chance to retry.
There is a last write_config() that may file silently though, the one in project_close(); not sure how this may be fixed.
Best regards, Colomban
[1] shouldn't it be renamed utils_is_file_writable(), without the "e"? Not sure though, but it looks weird to me and my spellchecker complains about it...
On Fri, 14 Jan 2011 17:11:35 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
Hi guys!
I found a bug in project creation (and save) check: if the project path is a writable directory, update_config() won't see that write will fail, and no error at all will be reported to the user (though write will fail on a directory).
While checking the hidden preferences usage, I found that a save error for anything except document will most likely generate only a geany_debug() message, which is useless with my setup. After some consideration, I think the following general approach should be implemented:
write_data_to_disk() is moved to utils.c as public gboolean function; it displays any save error with a popup message (the code from document_save_file).
utils_write_file() checks filename and text, invokes write_data_to_disk and returns ENOSPACE on failure (for compatibility).
This way the saving will be unified, so GIO, gio_unsafe_save_backup, the full POSIX-error checks and any future improvements will work for all files, not documents only, and the save errors will always be clearly displayed.
On Fri, 14 Jan 2011 17:11:35 +0100, Colomban wrote:
Hey,
I found a bug in project creation (and save) check: if the project path is a writable directory, update_config() won't see that write will fail, and no error at all will be reported to the user (though write will fail on a directory).
I fixed this (in the attached patch) by:
- check whether the path is not a directory, because
utils_is_file_writeable() [1] returns TRUE in this case, but write will still fail. Perhaps it should rather be fixed in utils_is_file_writeable()?
Could be done. Right now, utils_is_file_writable() has some logic to check whether the directory can be written if the given filename doesn't exist. And we use this check even in main.c:create_config_dir(). But I'd agree to split this code into two functions: utils_is_file_writable() and utils_is_directory_writable()
- check the return value of write_config(). This needed a little more
tuning for later calls to update_config() to work correctly, but now if write actually fails, and even if update_config() reported no error, an error will be displayed to the user and she will have a chance to retry.
Thanks for the patch, finally committed.
There is a last write_config() that may file silently though, the one in project_close(); not sure how this may be fixed.
I added a console warning at least for this case.
[1] shouldn't it be renamed utils_is_file_writable(), without the "e"? Not sure though, but it looks weird to me and my spellchecker complains about it...
Oops. You are right. The funny thing is, usually I'm using two different dictionaries for German<->English. The one, freedict, knows about writeable but not writable. The other one (it's just called german-english) knows only about writable but not writeable. And my spellchecker also knows only writable so that one won :D. Maybe Nick or Lex as native speakers can shed some light. For the meantime, I changed the function name to utils_is_file_writable() hoping this is more correct.
Regards, Enrico
2011/2/7 Enrico Tröger enrico.troeger@uvena.de:
On Fri, 14 Jan 2011 17:11:35 +0100, Colomban wrote:
Hey,
I found a bug in project creation (and save) check: if the project path is a writable directory, update_config() won't see that write will fail, and no error at all will be reported to the user (though write will fail on a directory).
I fixed this (in the attached patch) by:
- check whether the path is not a directory, because
utils_is_file_writeable() [1] returns TRUE in this case, but write will still fail. Perhaps it should rather be fixed in utils_is_file_writeable()?
Could be done. Right now, utils_is_file_writable() has some logic to check whether the directory can be written if the given filename doesn't exist. And we use this check even in main.c:create_config_dir(). But I'd agree to split this code into two functions: utils_is_file_writable() and utils_is_directory_writable()
- check the return value of write_config(). This needed a little more
tuning for later calls to update_config() to work correctly, but now if write actually fails, and even if update_config() reported no error, an error will be displayed to the user and she will have a chance to retry.
Thanks for the patch, finally committed.
There is a last write_config() that may file silently though, the one in project_close(); not sure how this may be fixed.
I added a console warning at least for this case.
[1] shouldn't it be renamed utils_is_file_writable(), without the "e"? Not sure though, but it looks weird to me and my spellchecker complains about it...
Oops. You are right. The funny thing is, usually I'm using two different dictionaries for German<->English. The one, freedict, knows about writeable but not writable. The other one (it's just called german-english) knows only about writable but not writeable. And my spellchecker also knows only writable so that one won :D. Maybe Nick or Lex as native speakers can shed some light.
I suppose you were looking for a single consistent answer, for something in the English language?? :-)
English UK uses writable
English US uses both
Cheers Lex
For the
meantime, I changed the function name to utils_is_file_writable() hoping this is more correct.
Regards, Enrico
-- Get my GPG key from http://www.uvena.de/pub.asc
Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On Mon, 7 Feb 2011 11:25:06 +1100, Lex wrote:
2011/2/7 Enrico Tröger enrico.troeger@uvena.de:
On Fri, 14 Jan 2011 17:11:35 +0100, Colomban wrote:
Hey,
I found a bug in project creation (and save) check: if the project path is a writable directory, update_config() won't see that write will fail, and no error at all will be reported to the user (though write will fail on a directory).
I fixed this (in the attached patch) by:
- check whether the path is not a directory, because
utils_is_file_writeable() [1] returns TRUE in this case, but write will still fail. Perhaps it should rather be fixed in utils_is_file_writeable()?
Could be done. Right now, utils_is_file_writable() has some logic to check whether the directory can be written if the given filename doesn't exist. And we use this check even in main.c:create_config_dir(). But I'd agree to split this code into two functions: utils_is_file_writable() and utils_is_directory_writable()
- check the return value of write_config(). This needed a little
more tuning for later calls to update_config() to work correctly, but now if write actually fails, and even if update_config() reported no error, an error will be displayed to the user and she will have a chance to retry.
Thanks for the patch, finally committed.
There is a last write_config() that may file silently though, the one in project_close(); not sure how this may be fixed.
I added a console warning at least for this case.
[1] shouldn't it be renamed utils_is_file_writable(), without the "e"? Not sure though, but it looks weird to me and my spellchecker complains about it...
Oops. You are right. The funny thing is, usually I'm using two different dictionaries for German<->English. The one, freedict, knows about writeable but not writable. The other one (it's just called german-english) knows only about writable but not writeable. And my spellchecker also knows only writable so that one won :D. Maybe Nick or Lex as native speakers can shed some light.
I suppose you were looking for a single consistent answer, for something in the English language?? :-)
Haha.
English UK uses writable
English US uses both
Ok, so it was ok'ish before and now is correct for both languages :D. So, I'd say everything is fine, at least with the spelling of the function name.
Regards, Enrico