FS#8949 - Alphabetical directory listing reversed after "Error Accessing Directory"

Attached to Project: Rockbox
Opened by Marc Guay (Marc_Guay) - Wednesday, 30 April 2008, 13:20 GMT
Last edited by Nils Wallménius (nls) - Wednesday, 08 October 2008, 16:33 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


Sansa e200 & r17294.

- Set directory sorting to Alphabetical
- Begin playing a folder/playlist via the file browser
- Shutdown
- On your PC, delete the folder you were just listening to
- Restart player
- Press play/resume
- "Error Accessing Directory"
- Rockbox begins playing the first file it can find on the drive
- View Files has been changed to All and Alphabetical directory listing has been reversed, so that Z is at the top and A at the bottom. The reversal will remain until the setting Sort Directories is changed to something besides Alphabetical (and then returned to Alphabetical).

This task depends upon

Closed by  Nils Wallménius (nls)
Wednesday, 08 October 2008, 16:33 GMT
Reason for closing:  Fixed
Additional comments about closing:  I checked in pondlife's patch 'fix8949_v5' with a bit of long-line policeing ;)
Comment by Clément Pit--Claudel (CFP) - Saturday, 10 May 2008, 12:31 GMT
Problem reproduced on Gigabeat, trying to play a protected file.
Comment by Michael Sevakis (MikeS) - Saturday, 17 May 2008, 19:32 GMT
Confirmed on e200 r17557 - at least the sorting problem. It didn't return on reboot though nor can I get the settings to change.
Comment by Bertrik Sikken (bertrik) - Wednesday, 21 May 2008, 19:07 GMT
I think the cause of it can be in playlist.c, function get_next_dir. This function modifies the global sort order setting (not a very nice thing to do) and is also capable of producing the "Error accessing directory" splash.
Comment by Nils Wallménius (nls) - Monday, 06 October 2008, 21:44 GMT
I did a little digging and reproduced this on my h300 and in the sim.
Commenting out the block in get_next_dir that changes global_settings.sort_dir makes the problem go away.
In other places where we touch dirfilter etc we reload the current dir with either reload_directory() or ft_load, doing this doesn't help but causes more weirdness, probably because when this is triggered tc->currdir is pointing to a dir that doesn't exist.
Comment by Nils Wallménius (nls) - Tuesday, 07 October 2008, 18:02 GMT
Ok, I think I have uncovered the Real Problem (tm) now,

1) get_next_dir() stores a local copy of a global var, changes the global and restores it on return, as bertrik says
this is not nice but also part of the cause of this bug.

2) get_next dir is called like this:
but also:

in other words it is called from 2 different threads.
When resuming without error the calls to get_next_dir() returns either before another call is made or after another call has returned
so the global_settings.sort_dir is unmodified as intended, but when we encounter an error in get_next_dir() the order changes slightly
so the call comming from the audio thread is made first (saving the original setting), and before that returns, the call from the ui thread is made (saving the changed setting) but the call from the audio thread returns (restoring the original setting) before the
one from the ui thread (restoring the changed setting). The changed timing seems to be because get_next_dir() displays a splash on this error so it takes 2 seconds before it returns...

I'm no threaded programming ace but I can see a few possible solutions.
*) Fixing get_next_dir to not change settings (best imho)
*) Locking this setting or the whole global_settings struct with some kind of mutex thingy (seems overly complex).
*) Working/Hacking around the issue...

Edit: forgot to mention that the reversed alpha sorting is available as a setting internally just not in the menu so it really isn't that strange
that this happens and also that AFAICT this works in the normal case purely by luck :/
Comment by Steve Bavin (pondlife) - Wednesday, 08 October 2008, 08:16 GMT
I've not tested this at all, but maybe this will help?
Comment by Steve Bavin (pondlife) - Wednesday, 08 October 2008, 08:53 GMT
Slightly tidier...
Comment by Steve Bavin (pondlife) - Wednesday, 08 October 2008, 10:25 GMT
Added sort_dir to the tree context as per amiconn's suggestion... still not tested though!
Comment by Steve Bavin (pondlife) - Wednesday, 08 October 2008, 10:26 GMT
Hmm, my editor appears to have stripped trailing spaces, hence the whitespace mods in that last patch. Still, I guess they should have been stripped anyway.
Comment by Steve Bavin (pondlife) - Wednesday, 08 October 2008, 11:28 GMT
OK, here's one without the unrelated changes. I also made the sort algorithm use alphabetical order where two files/dirs have the exact same date/time.
Comment by Steve Bavin (pondlife) - Wednesday, 08 October 2008, 12:09 GMT
Hmm, if tc->sort_dir == &global_settings.sort_dir, then changing *(tc->sort_dir) is probably a bad idea. This one lets it stand alone.
Comment by Nils Wallménius (nls) - Wednesday, 08 October 2008, 15:37 GMT
Tested on sim and it works nicely, love enums for different sort settings instead of magic numbers too :)
Red delta on a recorder build is ~150 bytes, not bad imho.