Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

collection: fix group filtering #2052

Merged
merged 1 commit into from
Feb 5, 2019

Conversation

LiranV
Copy link
Contributor

@LiranV LiranV commented Feb 3, 2019

The previous logic in lines 208-210 did not work as intended.
This is due to the IN selection not being specific for each group_id, which leads to cases where more than one image can be displayed per group_id even when in "collapse groups" mode.

Basically in this part of the query we want to ask for each image if the image has the lowest calculated min value (MIN(ABS...)) between other images with the same group_id (that passed the filters).

How to reproduce / test:

  1. Assume we filter by unstarred only, and that the filtered images are divided as groups containing pairs of JPG + RAW images, with the RAW image being the representative of each group.
    If we are in collapsed group mode (G button is active), at this stage we expect to see a list containing only the RAW representatives.
  2. Now we take one such group, and give only it's RAW representative a star rating.
    You might need to manually unstar the corresponding JPG as the new behavior star all the group members.
  3. We will suddenly see that all the images are showing (except the 1 RAW we starred), this includes the JPGs and the RAWs (although the G button is still active).

I've built and tested release 2.6 including this change and got the original intended behavior.

@TurboGit TurboGit self-requested a review February 3, 2019 21:44
@TurboGit TurboGit self-assigned this Feb 3, 2019
@TurboGit TurboGit added the bugfix pull request fixing a bug label Feb 3, 2019
@TurboGit TurboGit added this to the 2.6.1 milestone Feb 3, 2019
@TurboGit
Copy link
Member

TurboGit commented Feb 3, 2019

We will suddenly see that all the images are showing (except the 1 raw we starred), this includes the jpg's and the raw's (although the G button is still active).

Maybe I don't understand. But this is the expected behavior, no? You star the RAW then it not shown (filter unstarred only) and giving a start to a group leader (G active) will set one start to all picture in the group.

So, I've not been able to reproduce or understand the issue here.

@LiranV
Copy link
Contributor Author

LiranV commented Feb 3, 2019

@TurboGit
Just to make sure, when I refer to the G button, it's the one that controls all groups and not a specific group.

After these steps I've mentioned you should see:

  1. The JPG with the same group_id as the raw we starred, as it's now its own group representative (because its pair RAW was filtered out, which is the original representative).
  2. The JPG and RAW of each of the other groups.

But group collapsing is still on (G button is active`) so we would expect to see:

  1. same as 1 above
  2. All the rest of the RAW (no JPGs) images which are the rest of the representatives.

Is it any clearer?

@LiranV
Copy link
Contributor Author

LiranV commented Feb 3, 2019

@TurboGit
Also, sorry, but I meant that you star only the RAW image and unstar the JPG if needed.
This mimics coming from older versions of DT where the starring was not effecting the entire group (which led me to experience the bug in the first place).

@TurboGit
Copy link
Member

TurboGit commented Feb 4, 2019

To me, your code:

id IN
  (SELECT id FROM
     (SELECT id, MIN(ABS(id-group_id)*2 + CASE WHEN (id-group_id) < 0 THEN 1 ELSE 0 END)
      FROM main.images WHERE %s GROUP BY group_id)
  )

Is equivalent to:

id IN
  (SELECT id FROM
     (SELECT id FROM main.images WHERE %s GROUP BY group_id)
  )

Which is equivalent to:

id IN (SELECT id FROM main.images WHERE %s GROUP BY group_id)

And seems not correct. Or am I reading this wrongly?

@LiranV
Copy link
Contributor Author

LiranV commented Feb 4, 2019

@TurboGit They are actually not equal.

The following SELECT output is a table with two columns where each row is an image that has the lowest MIN() value calculated. Which basically means that in the id column we have only representative images or the 'closest' (by our terms) image that can be used as the representative for the current filtration.
SELECT id, MIN(ABS(id-group_id)*2 + CASE WHEN (id-group_id) < 0 THEN 1 ELSE 0 END) FROM main.images WHERE %s GROUP BY group_id

So now we want to do an IN operation to check if a given image is in this selection, but we need to have only one column for the IN operator to work, thus the additional (SELECT id FROM, just to get rid of the 2nd column that held the minimum values (all we care about are the IDs at this point).

@TurboGit
Copy link
Member

TurboGit commented Feb 4, 2019

Ah ah! I failed to see that in the MIN() id is used. Indeed, sorry for the noise the select are not equivalents at all. I'll review again with this in mind.

@TurboGit
Copy link
Member

TurboGit commented Feb 5, 2019

Looks ok to me. Thanks.

@TurboGit TurboGit merged commit 7928025 into darktable-org:master Feb 5, 2019
@LiranV LiranV deleted the fix-grouping-filtration branch February 5, 2019 11:48
@TurboGit TurboGit modified the milestones: 2.6.1, 2.6.2 Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants