Solarer/split culling mode

[Solarer]

This is the second attempt to split culling in a mode that is movement restricted to selection and one mode that is not restricted. This is a prerequisite to get #6025 done.

No Change:

• x enters culling. If images were selected, movement is restricted to selected images. If no images were selected, movement is not restricted.

Changes:

• implemented a lock button in culling that shows up on the right side of the current zoom level. This button visualizes whether movement is restricted to selected images or not. Pressing the button toggles the restriction on and off.
• Keybind c toggles the movement restriction on and off. If you are not in culling mode already, enter culling.

Issues:

• Layout buttons shift a little left and right because of center alignment when restriction button appears/disappears.

(a first draft was attempted in PR #11112. I will leave that PR open for reference until this one has been merged.)

[Solarer]

@AlicVB it has been some time but I found some time to code recently. Can you have a look at this?

[github-actions[bot]]

This pull request did not get any activity in the past 60 days and will be closed in 365 days if no update occurs. Please verify it has no conflicts with the master branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work.

[Solarer]

@AlicVB just a friendly ping to ask for your review.

[AlicVB]

OK, I've done a first try, after a looonng wait (sorry for that). I fear that I will ask you some questions already answered before, but well...

note that this has needed some rebasing (that's quite normal due to the age of the PR) also there's lot of gtk-critical warnings, certainly related to the new button.

imho, we have 1 main issue here : There's no more "automatic" startup switch between restricted and unrestricted mode. That means that users need to remember 2 shortcuts (and use theme accordingly) for the same feature, depending if a selection has been done or not... not really good imho. I wonder why you need to remove this feature ? Even in the case of a "selection-first" act-on algo, you may want to keep the "automatic" startup mode, no ? If you absolutely need to have a way to enter non-automatic modes, maybe we can have something like :

• c = unrestricted
• ctrl-c = restricted
• x = automatic

I fear that I still fail to understand the reason behind this change... (what is blocking exactly for your new act-on algo ?)

[AlicVB]

To go further with my difficulty to understand, I've read again PR #11112 where you say :

The changes in this PR are a prerequisit to everything in https://github.com/darktable-org/darktable/pull/10728 and https://github.com/darktable-org/darktable/issues/6025 because the new images_to_act_on algorithm prioritizes selected images over everything else and that would be a problem in culling because here a selection can act like a sub-collection (restricting movement). If we were to always act on all selected images, culling and culling dynamic would not work anymore.

esp. the last sentence : what exactly would not work anymore ? I can only see one : Once in "restricted mode" all the images are selected, which means that any action will be applied on the whole set of images... in this case, yes it's a big problem that would void completely actual culling concept. But I wonder how the changes proposed here can solve it... Let's take the context of the "classic" workflow : filter set to "all except rejected", and culling done by rejecteing "bad" images.

• Do you plan to "unselect" all images as soon as you enter in culling ? Then effectively "restricted" mode is void (and dynamic too). In this case, if we want to keep this modes, the only thing I can think of is to put in memory the initial selection when entering culling, before the unselection, and then to refer to this list and not to the selection one...
• Do you plan to have a "special case" for culling in order to not act on the whole selection ? But if I remember correctly that's not the case...

I really think I miss something...

[Solarer]

There's no more "automatic" startup switch

That was unintended, my bad. I will fix that. x should still have the automatic behavior whereas c is always unrestricted. We do not need an explicit button for restricted since restricted does not make sense when there is no image selected and in that case you can just use x to enter restricted mode.

esp. the last sentence : what exactly would not work anymore ? I can only see one : Once in "restricted mode" all the images are selected, which means that any action will be applied on the whole set of images... in this case, yes it's a big problem that would void completely actual culling concept. But I wonder how the changes proposed here can solve it...

That is the point and your second bullet point is how I want to solve it:

Do you plan to have a "special case" for culling in order to not act on the whole selection ? But if I remember correctly that's not the case...

For culling restricted I will ignore selection in images_to_act_on. Therefore I need to split the modes. (This is not included in this PR since we wanted to split this in separate PRs.)

[AlicVB]

great ! Thanks for the details... So I will wait for your rebase and fix, and test again.

[LebedevRI]

This PR should probably be rebased, the diff is, uh,

[Solarer]

Oh, I didn't mean to push to this repo. So I did not check it. Yeah, will rebase

[Solarer]

• Did a rebase of the code with master.
• Fixed the issue of button x not behaving as expected.
• Made some comments more verbose to better explain what each section of the code does

[Solarer]

... forgot to squash my commits

[github-actions[bot]]

This pull request has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please verify it has no conflicts with the master branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work.

[Solarer]

still active and waiting for review from @AlicVB

[AlicVB]

I've just done another try... and I think there's still some issues :

1. I've still some rebasing issues. I think that this is due to the fact that the git history before your commit is not clean for some reason. You can solve it by creating a new branch based on current master and adding your commit on top of it...
2. both x and c switch to unrestricted mode
3. if you select 4 images, with the cursor hovering one of the 4 selected images, and use ctrl-x you don't seems to enter the right mode... (culling with 2 images is shown). Everything is ok if you don't hover a selected image

I've not looked in the code to find where the issues come from, but I can if you need...

[Solarer]

Thanks for finding the time.

1. The rebasing issue is to be expected since other devs are not sitting by idle and pushing new code. I will do a final clean-up and rebase when functionality is ok.

2. x still works the same "smart" way as it does on master: If images are selected it enters restricted mode and if no images are selected it goes to unrestricted. c is always unrestricted. I think this was what we agreed on but I can disable the logic so that x is always restricted if that makes more sense to you.

3. Sorry but I was not able to reproduce this. From filemanager I always enter culling-dynamic no matter where I place the mouse. I did notice that I can not move from the other culling modes to culling dynamic via Ctrl-X - it returns me to filemanager layout. I added a new commit to address that, but this is not what you mean, right?

[AlicVB]

1. The rebasing issue is to be expected since other devs are not sitting by idle and pushing new code. I will do a final clean-up and rebase when functionality is ok.

I'm far from a git specialist, but I fear the issue here is a little bit different, as I have to solve conflict in files absolutely not touched by your PR. I think the easiest way (at least it's what I'm doing) is to start from a copy of darktable/master and cherry-pick your 2 commits. This way, there's no conflict : it's straightforward !

For point 2 and 3, I've spotted the issue : it's a shortcut conflict with previous settings : if you remove shortcutrc files, no problem anymore (but that point needs to be solved anyway, as we can't ask users to delete their shortcutrc files ;))

Oh, and there's also still a bunch of Gtk-CRITICAL... (but I'm sure you are aware of this)

[Solarer]

@AlicVB in fact I did not see the GTK errors because I did not look into the console output. Thank you for explaining what the issues were, they are fixed now.

I did a git rebase with master. That removes all commits from my branch, fast-forwards the branch to latest master and then reapplies all commits. Faster than cherry picking ;) I hope all git issues are resolved now.

Does your darktablerc have an existing key binding for Ctrl-X which conflicts? If that is the case, what can I do to fix this?

[AlicVB]

Does your darktablerc have an existing key binding for Ctrl-X which conflicts? If that is the case, what can I do to fix this?

yes, I have :

x=views/lighttable/toggle restricted culling mode
x;ctrl=views/lighttable/toggle culling dynamic mode

Which are the default shortcuts... So we'll have for sure to handle them, as the vaste majority of users will face the same issue. @dterrahe may help us on that point, as he's the shortcut guru ;)

[dterrahe]

There's no safe way for us to switch away from old default key shortcuts, because users may like them that way (and if we change them, they may want to change them back and we'll end up fighting the user).

What happens in somewhat recent master (since #14991) is that if a new default key is specified but all users have an old mapping (the previous default) in their shortcutsrc file, then it will override the new default, which will be listed under "disabled defaults" in the shortcuts dialog. The user can then activate the new default by selecting it and pressing Del (delete the override). You should mention this in the release notes.

[Solarer]

Wait a moment, there is a misunderstanding. I did not touch the x and ctrl-x key bindings. At least not from a functional point of view. They do the same thing as before but I might have changed the name of the function - if there is a conflict, it is because of that.

c is a new key binding that was not used before (to the best of my knowledge).

So we might have the situation that

1. Some users created a custom key binding for c that I would like to use now. And
2. Some users might have the replaced the defaults x and ctrl-x with something they like better.

In case of (1) the custom key binding of the user should overrule my binding, correct? I don't see an issue with that In case of (2) would I double the binding so that my and the users bindings trigger the same action?

Where does the conflict come from?

[dterrahe]

3. Everybody has "x" as "toggle culling mode" (because it used to be the default). That will override the new meaning you're trying to assign to it. So after upgrading, both c and x will trigger "toggle culling mode" and the new "toggle restricted" shortcut will show as disabled.

Have you tried simulating this upgrade process so you can verify this yourself?

[Solarer]

No I have not. Good idea, I will try that

[github-actions[bot]]

This pull request has been marked as stale due to inactivity for the last 60 days. It will be automatically closed in 300 days if no update occurs. Please verify it has no conflicts with the master branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work.

[Solarer]

So... I was finally able to fix the issue with the shortcuts :+1: it works now fine when updating from a previous version. From my perspective this PR can be pushed. I will update the first message in this thread to reflect the latest change.

[wpferguson]

Testing... Everything appears to be fine so far except use < to enter zoom mode. The shortcut system recognizes the keystroke as shift+,.

[Solarer]

Thanks, I totally missed that. Should work now

[Solarer]

Please let me squash all commits before merging.

[wpferguson]

Merging probably wont happen until July at the earliest. We're in feature freeze right now with 4.8 scheduled to release June 21st.

[Solarer]

:+1: Anyways, if the code is fine and ready for merge without any further changes - please let me know. I will then squash the commits and begin working on the act-on code based on this PR.

[AlicVB]

I've done a quick test. Just to be sure, I've spotted 2 changes :

1. the new "c" shortcut to enter culling in "unrestricted" mode.
2. the automatic mode (shortcut "x") as lost one aspect of the the automatic detection : now it switch between restricted/unrestricted based of the presence of a selection > 1 only. Before, it also check if the mouse hovered the selection or not...

So the question : was that mandatory for the next steps ? If not , I would prefer to keep that point as this...

Another question : full preview works the exact same way as culling for restricted/unrestricted. I've not spotted any change there, but do you plan to do some at some point ?

I'll try to have a look at the code asap, but I can't promise any delay........