GNOME Bugzilla – Bug 722380
Empty rows remain in browser tree when parent directories are deleted externally
Last modified: 2014-02-13 15:35:53 UTC
Related to bug 706260, the handling of directories is poor when they are deleted outside of EasyTAG. When deleting a parent directory of a directory that is selected, an empty row remains in the browser tree. To reproduce: 1. mkdir -p a/b 2. easytag a/ 3. select b in the browser tree There is a GLib critical, on which it should be possible to break: (easytag:2208): GLib-GObject-CRITICAL **: g_object_unref: assertion 'G_IS_OBJECT (object)' failed ** (easytag:2208): WARNING **: Error while querying path information: Error when getting information for file '/home/david/a': No such file or directory ** (easytag:2208): WARNING **: Error while querying path information: Error when getting information for file '/home/david/a': No such file or directory
Created attachment 269002 [details] [review] Fixed patch by allowing "collapse_cb" to create a dummy node only if directory is readable and exists.
Here's what happening: 1. Let us perform the same steps as given in the Bug Report. Taking "a/b" as an example. 2. Select "b". 3. Delete directory "a". 4. When collapsing the row, "collapse_cb" function of browser.c is called, which at 3059 line inserts a dummy node. 5. When "a" row is expanded, then "expand_cb" is called and as "a" is unreadable, "if ( (dir=opendir(parentPath)) )" block of 2897 is not entered. So, the empty dummy row inserted by "collapse_cb" remains and it is shown.
Review of attachment 269002 [details] [review]: This avoids the dummy rows, but (in the example a/b, when deleting a) an 'a' row is left in the browser, with an "unreadable" emblem on it. I wonder if we can detect the case that the directory does not exist, and sitinguish that from an unreadable directory? ::: src/browser.c @@ +3060,3 @@ + /* Insert dummy node only if directory exists*/ + if (error && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_NOT_FOUND)) I suppose that this can be collapsed down to just an if (error) {} else {}, as the error is not used. Also, please use braces, even around single-statement blocks.
(In reply to comment #3) > can detect the case that the directory does not exist, and sitinguish that from Oops, "distinguish"!
(In reply to comment #3) > Review of attachment 269002 [details] [review]: > > This avoids the dummy rows, but (in the example a/b, when deleting a) an 'a' > row is left in the browser, with an "unreadable" emblem on it. I wonder if we > can detect the case that the directory does not exist, and distinguish that from > an unreadable directory? Yes, we can detect whether directory exists or not, as I have used "error" and checked it with G_IO_ERROR_NOT_FOUND. I also think that directory should be deleted (which can also be done) as it doesn't exists on the file system so there is no point in showing that directory also. What do you think?
Indeed, I think that deleting the directory makes sense in the case that it is not found (as opposed to unreadable due to incompatible permissions). Please update the patch and I will take another look.
Created attachment 269010 [details] [review] Collapsing a directory which doesn't exists will be deleted Here, I have used if (error) {} else { /*insert dummy*/ if (error) {}} because it may happen that directory is earlier readable and contains subdirectories but it now becomes unreadable (may due to some access permissions). As it has become unreadable due to access permissions but it still may subdirectories, hence "error != NULL" and dummy node must be inserted,
Review of attachment 269010 [details] [review]: Thanks, that seems to work fine. I pushed a slightly modified patch to master as 81a1e7cf4bcb7897abd7c7ac4f6bfe0f0a038395.