Project

General

Profile

Feature #12630

Light Table / Image collection: added second order sorting

Added by Robert R 6 days ago. Updated 5 days ago.

Status:
Patch attached
Priority:
Low
Assignee:
-
Category:
Lighttable
Target version:
-
Start date:
03/14/2019
Due date:
% Done:

70%

Estimated time:
1.00 h
Affected Version:
git master branch
System:
all
bitness:
64-bit
hardware architecture:
amd64/x86

Description

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"

0001-Added-second-order-sorting-functionality-to-image-co.patch Magnifier - Second Order Sorting for Image Collection (20.3 KB) Robert R, 03/14/2019 07:01 PM

History

#1 Updated by David Houlder 6 days 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.

#2 Updated by Robert R 5 days ago

You're absolutely right. I don't like this approach by myself but it seemed to be the easiest one to show functionality as first shot. I'll have a look if I can add some better logic around collection->params.sort.

Thank you very much for your feedback!

Also available in: Atom PDF