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

[RFC] act on selection #18484

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Conversation

AlicVB
Copy link
Contributor

@AlicVB AlicVB commented Feb 27, 2025

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 | ? | ? |
   *     mouse inside table| ? | x |   |
   *          active images|   | x | x |
   *                       |   |   |   |
   *                       | S | S | A |
   *  S = selection ; O = mouseover ; A = active images

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 :

  • For new dt installation (no darktablerc) : default is "selection"
  • For the other cases, default is "hover"
  • The reasoning behind this is to avoid surprising and breaking workflow of "old" dt users. and to avoid unattended dataloss which is a big point of the issue.

Code changes :

  • This change a lot of code parts, but the act on changes on itself is very contained (in act_on.c and prefs)
  • 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 ?

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 !)

@AlicVB AlicVB added feature: enhancement current features to improve wip pull request in making, tests and feedback needed feature: new new features to add difficulty: hard big changes across different parts of the code base scope: UI user interface and interactions documentation-pending a documentation work is required labels Feb 27, 2025
@wpferguson
Copy link
Member

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
Copy link
Contributor Author

AlicVB commented Feb 28, 2025

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
Copy link
Contributor Author

AlicVB commented Feb 28, 2025

@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
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Contributor Author

AlicVB commented Feb 28, 2025

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
Copy link
Contributor Author

AlicVB commented Feb 28, 2025

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
Copy link
Contributor Author

AlicVB commented Feb 28, 2025

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
Copy link
Member

TurboGit commented Mar 1, 2025

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:

image

@AlicVB
Copy link
Contributor Author

AlicVB commented Mar 1, 2025

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:

image

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: hard big changes across different parts of the code base documentation-pending a documentation work is required feature: enhancement current features to improve feature: new new features to add scope: UI user interface and interactions wip pull request in making, tests and feedback needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants