act on selection

[AlicVB]

I've finally some time the next weeks to work again on dt.
Given the importance of the problem, here is a proposal for solving the long-awaited issue #16850
It's largely inspired from @Solarer work. Sadly I have failed to cleanly rebase his PR, and anyway, there was some changes to do on them.

act_on feature changes :

• This add an option to switch between "act_on hover" and "act_on selection" algorithms
• Apart from bugs, the "hover" mode doesn't change anything in current behavior
• The "selection" mode works like that :

   *              selection| x | ? | ? |
   *          active images|   | x | x |
   *           culling mode|   |   | x |
   *                       |   |   |   |
   *                       | S | A | C |
   *  S = selection ; A = active images ; C = culling selection

except for the image information panel, which works on hovered image like before

culling/preview changes :

• With the "selection" algo, it's now impossible to browse all images from the collection in fullpreview/culling without first deleting the selection of images you've made.
• So we add a new "restrict" toggle button in the lighttable toolbar, to allow to switch between "browse only the selection" and "browse all the images in the collection". Default shortcut : ctrl-R. This is only shown in culling "fixed" and full preview (if you show the bottom toolbar in that mode)
• We also add ways to directly enter "unrestricted" fullpreview/culling : shift-F and shift-X.
• Usual shortcut F and X enter fullpreview and culling in "auto" mode as before.

Default value :

• default is "hover" because :
• to avoid surprising and breaking workflow of "old" dt users. and to avoid unattended dataloss which is a big point of the issue.
• Using dt with selection mode is not so "quick" than with the hover mode (lot of features need more users actions to be achieved)

Code changes :

• This change a lot of code parts, but the act on changes on itself is very contained (in act_on.c)
• Main code changes are for the "culling/preview restriction" feature, which is here for both act_on modes (but required mainly for the "selection" one)

point to check/review :

• regressions in "hover" mode : considering the code changes, I'm quite confident on this point, but anyway...
• part that may have used "direct" images actions, without using the "act_on" routines : they won't adapt to the new algo
• features that are now impossible or too hard to do with the the "selection" algo
• all the "english" wording of new strings as usual ;)
• I wonder if we shouldn't revert to the "hover" algo for the "non-sticky preview" (shortcut W). But I'm reluctant to multiply the "exceptions"...

features now difficult/impossible to achieve :

1. In culling dynamic mode rating a single image with keyboard is impossible : you'll always act on all the images. and you can't change the selection, as dynamic mode is based on this selection... The only solution is to use the rating buttons in the overlays... -> fixed
2. In culling fixed mode, same problem, but you can change the selection via filmstrip for example. -> fixed
3. In mode with filmstrip, we still rely on mouse position, as we act differently if the mouse hover the filmstrip (we act on image like if we were in filemanager) otherwise we act on the main ("active") images. I hesitate to remove this specificity and "force" the user to go back to filemanager if he want to do "bulk" actions. That will complicate some workflow but avoid a little more unexpected result due to random mouse positions (which is one of the user issue with actual algo). What do you think ? -> remove this leftover for consistency : don't rely on mouse position, ever.

Of course everything here is debatable (RFC and Draft), and I'm ready to change lot of things here ;)
It would also be great, as mentioned in the issue, if someone can have a "deep" look at what the code is doing, etc. As we have seen here, it's not good that some features are dependent on just one person ! I'm obviously ready to help ;)

Last point : I've tested the new algo quite a lot the last days for testing, and I'm still not convinced... In my typical usage, I always need more movements/click to do actions (but converting old users to the new algo is not the goal !)

[wpferguson]

Some early testing with hover not active

• image information worked on hover. Using a Lua script to extend it also worked and it updated on hover.
• I could add tags using a shortcut while hovering. Is this an exception, or did it get missed?
• Tried a couple of Lua scripts that rely on hovering and they didn't work (threw errors). The scripts are going to require some work if/when this gets merged.
• During culling (full screen) I could rate and tag while hovering.

When I first started darktable I was using my normal workflow (lots of hovering) to see what worked and what didn't. Not much worked and my first thought was how crippled darktable felt. Doing anything seemed to take way more effort, clicks, time, etc.

Like act on hover or not, it is a feature and disabling it cripples darktable. If the default is to not have hover active, then we are distributing "crippled" software. I think the default should be hover enabled and if the user doesn't like it, then turn it off.

[AlicVB]

Thanks for your quick test.

• For the default value : that could be decided once everything is ok. So I would say : first put everything in place and then change it if needed. (fwiw, I agree with you, but I'm not the one in the best place to decide, as I don't plan to switch to "selection" algo... if it was just for me, I wouldn't have changed anything !)
• I don't have tested anything related to Lua, so it's expected that you get issues. Can you maybe point me to some Lua script that are not working anymore ? And to be sure : the issues appears with the new "selection" mode only or also with the "hover" mode ?
• adding tag and culling ratings are exactly the missing cases we have to track, and fix. In fact they certainly decide directly on which images to act, instead of using the act_on interface. I'll have a look !

[AlicVB]

@wpferguson :

• During culling (full screen) I could rate and tag while hovering.

I'm not able to reproduce this one...
Here's how it works here with the "selection" algo :

1. Select some images (say 6 images)
2. Put the mouse at whatever place and press "x" to enter culling (same happens with "ctrl-x")
3. Once in culling view (set to show multiple images, to see the difference), hover a specific image (you can verify that the blank border appears on the image)
4. Press "r" or whatever rating shortcuts
5. => all the images shown on screen (called "active" images in the first post) are rejected/rated. Not just the hovered one

• I could add tags using a shortcut while hovering. Is this an exception, or did it get missed?

How are you doing this exactly. I've tried "ctrl-t", "alt-t" and specific shortcut assigned to the quicktag Lua script, but they all work on the selection, not the hovered image...

[jenshannoschwalm]

I checked and for me it seems all to be working. Although - i am a pretty bad tester as my personal dt workflow as a user is very conservative / simple, rarely use culling, almost never lua so likely i didn't get cornercases or am not even aware of functionality.

Read through the act_on code and it's readable even for someone not knowing that code :-) First read - in the caching functions you are using id as an int, for reading/debugging code it would be nicer to use imgid and dt_imgid_t ...

[wpferguson]

How are you doing this exactly

I had a collection with nothing selected and I entered full preview layout. I could rate and tag the single image shown.

The Lua errors were about having no image, because we rely on the hovered image being there. I just need to add some checks to make sure there is an image before trying to do something with it.

[AlicVB]

I had a collection with nothing selected and I entered full preview layout. I could rate and tag the single image shown.

Then, in my implementation of the algo, it's fine : "active" image(s) is higher priority. I've done this because I think that in this specific layout (full preview, culling and darkroom) user will expect action to happen on the "main" images, contrary to "browsing" layout...
That doesn't mean it's the right thing to do, just that it's not a "bug" ;)

[AlicVB]

I've added a "features now difficult/impossible to achieve" part in the main post to track "conceptual issues" with new algo that need more thought (read : I'm not sure how to fix them right)

For 1. and 2. currently the "only" solution I've think of is to implement a sort of "culling selection" different from the "normal" one, that won't be saved anywhere. Note that this will require quite a lot of change (but that's possible)...

[AlicVB]

In order to fix 1. and 2. last commit implement a very simple culling-specific selection :
With the "selection" algo, in culling (fixed or dynamic) :

• mouse over doesn't have any visual effect
• image click make the image "selected" (for culling only : this doesn't change the lighttable selection) a blank border appears around the image.
• another click on the same image to deselect.
• a click on another image deselect the first one in favour of the new one.
• act_on image is always the "culling-selected" one.

That should allow a "click and shortcut" workflow...

[TurboGit]

Not tested yet, but yes I think we want to keep the hover mode the default. This mode has been advertised quite a lot and is described in the main lighttable panel when no images are displayed. In the French UI:

[AlicVB]

Not tested yet, but yes I think we want to keep the hover mode the default. This mode has been advertised quite a lot and is described in the main lighttable panel when no images are displayed. In the French UI:

Well, changing the default for one or the other algo is just one line of code, so I would prefer to let the decision for later, once the new mode as been extensively tested and we are sure it's production ready.

But you have pointed a missing change I've completely missed : the "no image" page should reflect the algorithm !

btw : last commit enhance a little the css by swapping the hover and the selected background for selection algo (as the selection becomes more important than hover)

[TurboGit]

btw : last commit enhance a little the css by swapping the hover and the selected background for selection algo (as the selection becomes more important than hover)

Nice and good !

Tested and to me this is working as advertised.

I'll review the code next and then we need to discuss broadly (@ralfbrown @dterrahe @jenshannoschwalm @wpfergusonx) if we really want this complexity added.

In the selection mode I find the culling far harder to use. One need to click on the image first and then add a note or a color label... And then deselect it or select another one. I understand that this is need to avoid rating image not currently visible, but it makes this mode harder to use.

[TurboGit]

BTW, in selection mode, rejecting image using 'r' shortcut multiple time I get:

(darktable:330265): Gtk-CRITICAL **: 10:41:12.575: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed

EDIT: Seems like I can reproduce on master.

[wpferguson]

Tried a couple of Lua scripts that rely on hovering and they didn't work (threw errors). The scripts are going to require some work if/when this gets merged.

I've worked out the modifications needed to the Lua scripts to handle hover/no hover.

[AlicVB]

BTW, in selection mode, rejecting image using 'r' shortcut multiple time I get:

(darktable:330265): Gtk-CRITICAL **: 10:41:12.575: _gtk_widget_captured_event: assertion 'WIDGET_REALIZED_FOR_EVENT (widget, event)' failed

EDIT: Seems like I can reproduce on master.

Yes, I've already reported that in #18464 , as I didn't find yet why it happens...

[AlicVB]

if we really want this complexity added.

tbh, the main complexity in this PR is the added "restriction" feature that will profit to both modes. Other than that, it's mainly duplication (with another algorithm) of the routines in act_on.c.
What scares me a bit is that some users might want a mix of the two modes... ("selection" in filemanager and "hover" in culling for example)

In the selection mode I find the culling far harder to use. One need to click on the image first and then add a note or a color label... And then deselect it or select another one. I understand that this is need to avoid rating image not currently visible, but it makes this mode harder to use.

completely agree, everything feel harder (more "clicks") that's why I'm not ready to switch to that mode !
I suppose the best workflow for culling in this mode is by using the rating icons with the mouse...

[jenshannoschwalm]

Found some time to test this for some editing sessions using keyboard, mouse and a midi device.

For me all works as intended/advertised so far, visualizing to-be-acted-on seems good to me too.

New mode is likely as expected if users want a select-by-click behavior.

For me a go. Default should not change. I wonder if we could change the mouse look depending on the mode. In hover mode indicating that it sort-of-selects ?

[AlicVB]

After one more week of tests :

• hover needs "definitively" less user actions for lot of features.
• I haven't spot any issue...

So I've change the default to "hover", whatever the upgrade type.
After some more thinks, I've also remove the diffrence if the mouse is inside filmstrip or not :

• for consistency : "selection" mode don't rely on mouse position, ever.
• as lot of the issues with "hover" mode are reported by users that are "surprised" that the action doesn't occurs of the right images because they haven't pay attention to the mouse position.

At this point I propose to remove the draft status

[TurboGit]

Works for me, let's move forward. Thanks!

[TurboGit]

@AlicVB : Can you draft a release note entry? TIA.

[AlicVB]

Release note proposal :
Add a new setting to change which images are taken into account for actions: By default, the image under the cursor takes priority. With this parameter enabled, the selected images will take priority, and the image under the cursor will only be taken into account to feed the information modules.

[TurboGit]

Thanks!