Project

General

Profile

Feature #12630

Light Table / Image collection: added second order sorting

Added by Robert R 9 months ago. Updated 8 months ago.

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

100%

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"

History

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

#2 Updated by Robert R 9 months 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!

#3 Updated by Robert R 8 months ago

Hi all,
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.

Best Regards,
Robert

#4 Updated by Robert R 8 months ago

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.

#5 Updated by Pascal Obry 8 months ago

  • % Done changed from 70 to 100
  • Status changed from Patch attached to Fixed

Merged.

Also available in: Atom PDF

Go to top