Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category User Interface
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by Marc_Guay - 2008-04-30
Last edited by nls - 2008-10-08

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

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).

Closed by  nls
2008-10-08 16:33
Reason for closing:  Fixed
Additional comments about closing:  

I checked in pondlife's patch 'fix8949_v5' with a bit of long-line policeing ;)
Thanks!

CFP commented on 2008-05-10 12:31

Problem reproduced on Gigabeat, trying to play a protected file.

MikeS commented on 2008-05-17 19:32

Confirmed on e200 r17557 - at least the sorting problem. It didn't return on reboot though nor can I get the settings to change.

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.

nls commented on 2008-10-06 21:44

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.

nls commented on 2008-10-07 18:02

Ok, I think I have uncovered the Real Problem ™ 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:
audio_thread()→audio_fill_file_buffer()→audio_current_track()→playlist_next()→create_and_play_dir()→get_next_dir()
but also:
main()→app_main()→root_menu()→do_menu()→wpsscrn()→gui_wps_show()→audio_current_track()→playlist_next()→create_and_play_dir()→get_next_dir()

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 :/

I've not tested this at all, but maybe this will help?

Slightly tidier…

Added sort_dir to the tree context as per amiconn's suggestion… still not tested though!

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.

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.

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.

nls commented on 2008-10-08 15:37

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.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing