Light Table / Image collection: added second order sorting
I added some lines to the Image Collection of Light Table module. This offers second order sorting functionality for images. It remembers previous sorting criteria as second choice.
For me this is most helpful in combination with groups of images, e.g. sort by GROUP, RATING
See also email in developer mailing list "[darktable-dev] [PATCH] Image collection: added second order sorting"
#1 Updated by David Houlder 4 months ago
Nice idea, and I've often wanted this feature.
However, I can't see how this implementation with those two static variables can work reliably. I see that you're checking against COLLECTION_QUERY_USE_LIMIT in order to limit the cases where the second sort order gets updated, but that doesn't alter the fact that your patch makes dt_collection_update() behave non-deterministically: calling it the first time can generate a query that sorts by the first sort key alone, calling it again can cause it to sort by both first and second order keys, and dt_collection_update() is called from a lot of places. You may have been lucky in your testing and observed that your patched version often works, but I can't see how it's going to work all the time.
Note that there's no guarantee that two successive dt_collection_update() calls will be called with the same collection, or even by the same thread, in which case you have thread safety problems too.
Essentially your code has behaviour that depends on call history that is opaque to the caller. I doubt that any implementation relying on static or global variables to track state can work reliably.
Same problem with second_order in dt_collection_get_sort_query(). You're relying on an exact pairing of calls to dt_collection_update() and dt_collection_get_sort_query() to keep those two statics in sync. It looks like that may currently be the case, but it's a pretty fragile thing to rely on.
Sorry to rain on your parade. I think it's a desirable feature. I think the state needs to be kept alongside collection->params.sort and only changed when that is changed.
as David proposed, I moved sort_second_order to collection->params.sortSecondOrder. This implementation is more straight-forward, too. Additionally I found and fixed a bug which occured when sorting by GROUP and TITLE/DESCRIPTON.
Please find updated patch attached.
I noticed that there were some changes at origin/master meanwhile which affect same SQL queries as my patch. Please find updated patch attached for easier take-over.