Re: [PATCH] weird parent window selection bug

[Alexander Larsson <alexl redhat com>]

On Tue, 2004-10-12 at 13:46 -0700, Jimmy Do wrote:
> Hey everyone,
> 
> I found this bug while working on improving the behavior of 'alt-up'
> (the patch is coming soon!)
> 
> This patch fixes a race condition in FMDirectoryView that causes an
> item in list view to sometimes not to be selected in its parent window
> when using 'alt-up' in a Nautilus navigator window.
> Without the patch, this happens:
> 	1. Open up a navigator window to '/home' and set it to 'List View'.
> 	2. Close the window.
> 	3. Re-launch Nautilus either by logging out and logging back in, or
> by typing 'nautilus --quit' in a terminal.
> 	4. Open up a navigator window to your home directory
> 	5. Now press 'alt-up' to go to the '/home' directory
> 	
> 	Expected result: your home folder should be selected in the window
> 	Actual result: your home folder is not selected
> 	
> 	Now, press the 'Back' button to go back to your home folder, and then
> press 'alt-up' again to go back to the '/home' folder.
> 	This time, and all subsequent times until you restart Nautilus, your
> home folder *is* selected.
> 
> I found that when doing an 'alt-up' into the /home folder for the
> first time, and the /home folder is in list view, FMDirectoryView's
> selection_changed_callback would get called *before* finish_loading()
> had a chance to set FMDirectoryView->FMDirectoryViewDetails->loading
> to TRUE. Since 'loading' was false, selection_changed_callback tried
> to immediately set the selection of the view, causing us to lose the
> selection. On subsequent uses of 'alt-up' however,
> selection_changed_callback occurs *after* the call to
> finish_loading(). My guess is that this has to do with differences in
> the length of time to load all file information and prepare the view
> in list view compared to icon view, and it might also have to do with
> the length of time for loading a directory not in the $HOME filesystem
> subtree (this bug shows itself if you open any non-$HOME directory
> such as /usr/local for the first time).
> 
> The fix was to set 'loading' to TRUE sooner, in load_directory(), so
> that it will be TRUE when selection_changed_callback() runs. I also
> removed unnecessary setting of
> FMDirectoryView->FMDirectoryDetails->loading in FMIconView.
> 
> Sorry for being so long-winded. Please let me know if I should cut
> down on the information! :)

Actually, it wasn't enough for me to understand why this was happening,
so I had to trace it down a bit more. I wanted to know why
selection_changed_callback was called at this time. 
Heres what I found:

After loading a view component into the view frame we run: 

load_new_location_in_one_view (NautilusViewFrame *view,
                               const char *new_location,
                               GList *new_selection)
{
        nautilus_view_frame_load_location (view, new_location);
        nautilus_view_frame_selection_changed (view, new_selection);
}

These calls both go through corba and an idle handler, but the order is
still guaranteed. FMDirectoryView handles selection_changed while
loading a directory by delaying setting the selection until the
directory is fully loaded. 

However, FMDirectoryView also delays starting to load the directory
until we have the metadata for the files and directory, so if these are
not in memory when fm-directory-view.c::load_directory() is called it
waits until they're read before starting to load the directory, and it
doesn't set the "loading" variable before then. This is wrong, since
loading needs to be set by the time the selection_changed call arrives
so we can delay setting the selection. 

The mistake is understandable, since there are two types of "loading"
going on here. Whether the view has had "load_directory" called, and
whether the view has started to load the directory (has called
load_underway).

An alternative solution would be to not set the selection until the view
reports load_underway, but that seems a bit overcomplicated. So, I've
commited your patch. Thanks a lot, and good catch!

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a one-legged small-town grifter with no name. She's a tortured 
antique-collecting mechanic with someone else's memories. They fight crime!