darktable-org / darktable

darktable is an open source photography workflow application and raw developer
https://www.darktable.org
GNU General Public License v3.0
9.8k stars 1.14k forks source link

Images to Act On - How Should It Work? #6025

Closed elstoc closed 5 months ago

elstoc commented 4 years ago

Now that a lot of the decision making around image selection in the lighttable and thumbtable is centralised into a single function (dt_view_get_images_to_act_on) I thought it might be time to talk about how it works and come to a consensus on how we can make it a little more user-friendly.

Here's now it works right now:

                mouse over| x | x | x |   |   |
        mouse inside table| x | x |   |   |   |
    mouse inside selection| x |   |   |   |   |
             active images| ? | ? | x |   | x |
                          |   |   |   |   |   |
                          | S | O | O | S | A |
     S = selection ; O = mouseover ; A = active images
     the mouse can be outside thumbtable in case of filmstrip + mouse in center widget

     if only_visible is FALSE, then it will add also not visible images because of grouping

I find myself constantly consulting the above table and still have trouble keeping it in my head. And I don't think I'm the only one who sometimes finds it confusing. Given the number of bug reports and discussions where this comes up it may be better if it were a little more simple/intuitive or perhaps if some of its behaviour could be simplified via preference settings or flagged better in the UI.

For example, probably the most commonly used mode in the lighttable is called 'file manager'. Most users would probably expect it to work (within reason) like a standard file manager and in this mode at least, being able to perform actions just by hovering over images and hitting key combinations is decidedly non-intuitive. And it gets even odder when you have multiple images selected but are inadvertently hovering over an unselected one.

Anyway, I've just opened this for discussion to see whether there might be a better way.

Solarer commented 2 years ago

Hope everybody had peaceful Christmas days!

I found some time to implement a new version of images_to_act on but there is still one problem that I am stuck with and I would like to sort it out before I share the code. I have to check if I am in culling-dynamic mode because the code needs to behave differently there and I do it like this: dt_view_lighttable_get_layout(darktable.view_manager) == DT_LIGHTTABLE_LAYOUT_CULLING_DYNAMIC

What I realised is that this function also returns true when I am in the darkroom if I entered it from culling-dynamic. To me this makes no sense, it should return false in darkroom, map or other layouts. Is this a bug? If it is not a bug, why is it implemented that way and what is the best way for me to check if I am in darkroom or lighttable from within images_to_act_on?

AlicVB commented 2 years ago

well, I don't see this as a bug : as the name of the function indicate, it's the layout in lighttable which is returned... if you want to know the current view, you can use something like :

const dt_view_t *v = dt_view_manager_get_current_view(darktable.view_manager);
if(v->view(v) == DT_VIEW_DARKROOM)
{
  ...
}

Another point to avoid you too many refactoring, have you seen #10649 ? I've refactored that part of code in a separate file, so implementing a parallel algorithm should be simpler and more clear... (I've also fixed and enhanced the caching, but that shouldn't change anything for you)

Solarer commented 2 years ago

Thanks, it worked fine.

No I have not seen #10649. I created a branch ~12days back so we will need to do some merging at some point but first we need to align if people are happy with the new approach. I will share the code tomorrow ;) Should I do a pull request or link to my repo?

AlicVB commented 2 years ago

pull request is better imho, you'll have better test field and it's easier to review/comment

Note that, as I've said you'll need to do some rebasing first as PR #10649 has just been merge into master. Basically, you have to move everything to act_on.c just ask if you need help. Sorry for the extra work... but that should be cleaner to read now !

Solarer commented 2 years ago

Thanks, I think I managed the merge without messing things up :P Here is the PR: #10728

ChristianBirzer commented 2 years ago

As a new user (and hopefully soon developer 😉) to dt I'd like to add a different approach how to handle selections and hovers to the discussion (and sorry for again comparing a little bit with LR... I just can't stop 🤣):

What about separating actions to selected images and the hovered image to different modes? I think about something like LR does with its spray can. Some sort of point-and-shoot mode?

By default, the selection-mode is activated in file manager and filmstrip. So all actions go only to the selected images. If no image is selected, nothing is changed. Hovering is ignored. Like I would expect as a new user. (I'd exclude the 'w' shortcut for zooming into an image because this does not change anything and is very handy)

By activating the point-and-shoot mode (find a better name!) the selection is cleared (no selection possible at all) and all shortcut actions affect only the hovered image. This would allow to very fast reject or rate images. I can also imagine a couple of shortcuts to add some predefined tags to the hovered image ("favorite tags"). In culling mode, it already seems that there is no selection possible (at least in the central widget), so this would always use the point-and-shoot mode.

By separating the problem into two different modes, I think it would be friendlier for new users (they simply work with the selection mode until they discover and explicitely activate the other mode) but still allows to use the quick mode for experienced users without getting confused about selection/hover/hover-in-selection stuff..

I'm sure, I've overlooked a lot of things that argue against it, but from a user's point of view it would simplify some things.

Solarer commented 2 years ago

That is more or less how my code behaves behind the scenes. If some pictures have been selected, hover is disabled (you call this selection-mode) if no pictures are selected, hover works. W works always because it does not change anything. So there is no button to "switch the mode" but you do it by using a selection or not.

phweyland commented 2 years ago

Looking at #11589, the issue can be summarized as: if selected images and occurs hovering, no validation (apply) should happen (because we would apply on the wrong image). Then it would interesting to get from dt_act_on_get_images() the information:

This way the module would be able, at least, not to make a wrong action.

Solarer commented 2 years ago

@phweyland Or (since you plan to ignore the hovering anyways), we just return the selected images ;) Problem is: current consent with @AlicVB was that we do a dual implementation because some people prefer the current images_to_act_on. That means we need another special case handling in the metadata module - creating even more spaghetti code. I am still in favor to fix this implementation for all users in the same way. It is too cumbersome to maintain the old implementation. There will be even more cases like this

phweyland commented 2 years ago

@Solarer, the hovering is still very useful to show information.

Solarer commented 2 years ago

I think I did an overwrite so that showing information works as before using hover.

phweyland commented 2 years ago

In any case I would vote (I think I've already) to use hovering only to show information and not to apply anything. I wouldn't mind to have a preference if we wanted to keep the feature (more spaghetti, though :)).

johnny-bit commented 2 years ago

Hover to show info (like say tooltip or in info modules) and only operate on selected items (and hovering NOT implying selection) is imho best way forward. And less spahetti please :)

Nilvus commented 2 years ago

I totally agree with you @johnny-bit. And good to see you again. Was a long time.

mlaverdiere commented 2 years ago

Late to the game, but to be short, while I love dt, I'm also experiencing problems with lightable hovering/selection process. Each time I do some culling with dt, I end up with a lot of errors when applying tags, stars, colors, rejection, etc.

I suppose that what @johnny-bit and @phweyland are proposing makes sense (hovering for info only), but I would even prefer the behaviour of a regular file manage (like GNOME Files/Nautilus), where hovering does nothing (except some subtle icon highlighting).

RawConvert commented 2 years ago

I also agree with johnny-bit on 22May.

quovadit commented 2 years ago

I agree that it's the best solution not to operate on hovered items by default.

But there are situations, where it's very convenient to quickly apply color-ratings on a number of images without having to click everytime.

That's why I suggested two different modes:

See details and mockups in https://github.com/darktable-org/darktable/pull/10728#issuecomment-1021558133

rekcodocker commented 1 year ago

Ah, so that is where this behaviour comes from. It's not a bug, it's a feature. Or actually (and that's the problem) many features in one.

I notice in the discussion that some people want to have as many ways of selecting/acting as possible. However, even though you can explain it, it is not intuitive. The other approach is to work from what a user might expect - and trust me, on a filemanager-layout she expects filemanager behaviour.

This principle would work for me:

elstoc commented 1 year ago

Essentially act-on-hover is the thing that needs removing. We could probably cherry-pick some commits from R&dt, which iirc already has that removed.

rekcodocker commented 1 year ago

Essentially act-on-hover is the thing that needs removing. We could probably cherry-pick some commits from R&dt, which iirc already has that removed.

To be clear: You mean you could no longer apply any function when you hover over a picture?
To do away entirely with the notion of an 'active image' ? Hovering means nothing?

(That I find very scary. That is extremely useful behaviour)

elstoc commented 1 year ago

You mean you could no longer apply any function when you hover over a picture?

Yes, it's just far too easy to accidentally do stuff IMO. I would be fine with a tick box to enable it, but it should default to off as it is too much of a surprise to new users.

quovadit commented 1 year ago

I would be fine with a tick box to enable it, but it should default to off as it is too much of a surprise to new users.

That's what I meant with the two modes (see comment above)

Solarer commented 1 year ago

Most of the stuff has already been done by me and is sitting idle in a branch waiting for clean up and merge

Problem is that this requires a change to the culling view mode and my first draft was not approved by @AlicVB . Since then I have been very busy with life and it will stay that way for the unforseeable future. So I cannot complete the requested changes.

If someone is willing to put in the time to complete this I would be happy to assist and explain the new code...

airflow2010 commented 1 year ago

I read this discussion with great interest. While I cannot really contribute something very meaningful to it, I just want to raise my hand signalling that I think the implementation of image selection in darktable leaves to be desired, and frustrates me a lot. I recently tried to describe the problems I have with it in this thread: https://discuss.pixls.us/t/frustration-with-culling-mode/33795

What I think is interesting about the discussion here is that I notice that different users have different problems with consistent behaviour of image selection, leading to mistakes and unexpected results. 1) Some people are accidently manipulating the wrong image because they hovered over an image, making the existing selection invalid - they would want the "hover"-selection to be disabled. 2) Some people (like me) are using the "hover"-selection method in the culling mode (where it is mandatory, because there you cannot select pictures by clicking on them compared to lighttable-mode), and are frustrated because sometimes the hover-mechanism doesn't work reliably, and then more images are manipulated then intended - this is described in detail here or in the discussion thread i linked in the first paragraph.

elstoc commented 1 year ago

The worst offender, though, is when you purposely select multiple images (with shift+click and/or ctrl+click) and then accidentally apply changes to an unselected image just because you happened not to be hovering your mouse over any of your chosen images at the time you pressed the shortcut. That behaviour is just plain dumb

airflow2010 commented 1 year ago

Most of the stuff has already been done by me and is sitting idle in a branch waiting for clean up and merge

  • Fix order of acting so that selection has hingest priority
  • Allow to enable/disable mouse over for people who want it
  • keep old order of acting as a fall back so that people can switch back if they want (I would prefer to just delete that code)
  • all of this can be tuned via a selector in the settings screen

Problem is that this requires a change to the culling view mode and my first draft was not approved by @AlicVB . Since then I have been very busy with life and it will stay that way for the unforseeable future. So I cannot complete the requested changes.

If someone is willing to put in the time to complete this I would be happy to assist and explain the new code...

@Solarer, thanks for your work on this one. Here in the discussion, and also your coding efforts. Your code was not accepted - may I ask what were the problems with your PR according to @AlicVB? Perhaps we can discuss and find possible solutions that make everybody happy, perhaps even find somebody who can implement it. :-) What would interest me in your suggested solution is, how is your suggested proposal fixing the problem some people have with culling mode (where "mouse-over"-action is a must and where we have many reports of people accidentally rating all pictures on the screen although they hovered over an image).

Solarer commented 1 year ago

where we have many reports of people accidentally rating all pictures on the screen although they hovered over an image

I mainly use culling in single image mode so I never had that issue. I might be mistaking but I think there is always exactly one image active in culling, no matter what you do with the mouse (if not, it's a bug). The only case in which you could edit multiple images by accident is if you hover over one of your selected images in the filmstrip. That will prioritize the full selection over any other image and is a very unintuitive "feature" which is only required because otherwise selection would always be ignored in favor of mouse over. It can be removed if the code is changed so that selection gets a higher priority than mouse over.

If this is indeed the problem that you mean, my code fixes that by making that "feature" redundant and removing it.

where "mouse-over"-action is a must

You mean you MUST use the mouse in that mode? We could improve keyboard support by making it so that the arrow keys move the white selection border and we only display new images if the first image is selected and we hit the left key or if we select the last visible image and hit the right key. This is not addressed in my code.

may I ask what were the problems with your PR

My Images to act on code was not the problem. It was a change to culling (which is a prerequisite so that selection can continue to properly work in culling) that was not accepted. I made some button layout changes that were more than the bare minimum and @AlicVB wanted to keep the change as small as possible to not confuse people. We found a compromise but I ran out of time to implement it. And also, ran a bit out of motivation because I think my change was a quite elegant solution that would now need to be redone completely. The culling change is here: https://github.com/darktable-org/darktable/pull/11112

If someone has time to make culling ready for the change to images to act on, we can merge the new images to act on code with little additional work I think.

airflow2010 commented 1 year ago

The more I read and think about certain design decisions around darktable, the more it becomes clear that it can be difficult sometimes, as methods of thinking and working vary greatly between persons. This problem becomes worse the more complex the program is. And darktable is quite complex. I think it's important to 1) respect other's peoples usecases and viewpoints and 2) be very cautious about adding more complexity.

I mainly use culling in single image mode so I never had that issue.

This statement alone surprises me. Because in my understanding, culling mode shows its strength when comparing multiple images side by side. In my understanding, that's what it was built for, and how I use it. If I would just want to inspect a single image and rate it then, I would do it from lighttable-view.

I might be mistaking but I think there is always exactly one image active in culling, no matter what you do with the mouse (if not, it's a bug).

Yes that would be great, but it's not the case. Perhaps you assume that because you use culling mode with single image only, where that would be correct? Otherwise, me and other people wouldn't complain about problems with accidentally rating a bunch of pictures in culling mode instead of only the one they wanted to act on. Of course, if you use culling mode with multiple displayed images, there has to be a method to define which image you want to act on. I use the mouse for that.

The only case in which you could edit multiple images by accident is if you hover over one of your selected images in the filmstrip. That will prioritize the full selection over any other image and is a very unintuitive "feature" which is only required because otherwise selection would always be ignored in favor of mouse over. It can be removed if the code is changed so that selection gets a higher priority than mouse over.

If this is indeed the problem that you mean, my code fixes that by making that "feature" redundant and removing it.

That's not the problem I mean. The problem I mean is when the mouse is clearly and definitely over a specific image in culling mode (when multiple images are shown besides each other) and want to act on it. In this case, it often (not always) happens that all displayed images are affected, instead of just the one where the mouse was over.

My Images to act on code was not the problem. It was a change to culling (which is a prerequisite so that selection can continue to properly work in culling) that was not accepted. I made some button layout changes that were more than the bare minimum and @AlicVB wanted to keep the change as small as possible to not confuse people. We found a compromise but I ran out of time to implement it. And also, ran a bit out of motivation because I think my change was a quite elegant solution that would now need to be redone completely. The culling change is here: #11112

If someone has time to make culling ready for the change to images to act on, we can merge the new images to act on code with little additional work I think.

Thanks for sharing. I read through the discussion, but I'm afraid I miss some of the knowledge about different usecases of culling mode and cannot contribute. I will look into it more, perhaps I can make a suggestion at some point.

Solarer commented 1 year ago

The problem I mean is when the mouse is clearly and definitely over a specific image in culling mode (when multiple images are shown besides each other) and want to act on it. In this case, it often (not always) happens that all displayed images are affected, instead of just the one where the mouse was over.

I have never observed that. What does happen is: When you enter culling, there is no image active initially until you hover the mouse or use the arrow keys. So in this case you would act on all visible images - but only until you move around! Once you moved the mouse the white border appears and then you should only act on that one image. This issue was described earlier (e.g. here: #7254) and I believe I also fixed that somewhere in this PR (not sure though). If that is still not it, you need to click around a little bit until you figure out the exact circumstance that causes this behavior.

airflow2010 commented 1 year ago

The problem I mean is when the mouse is clearly and definitely over a specific image in culling mode (when multiple images are shown besides each other) and want to act on it. In this case, it often (not always) happens that all displayed images are affected, instead of just the one where the mouse was over.

I have never observed that. What does happen is: When you enter culling, there is no image active initially until you hover the mouse or use the arrow keys. So in this case you would act on all visible images - but only until you move around! Once you moved the mouse the white border appears and then you should only act on that one image.

Your description about how the selection process works in this scenario meets exactly my understanding and expectation how it should work. Sadly, even if the white border is displayed, sometimes all images on the screen are affected. As an engineer myself (different area), I understand how important and helpful reproducibility of a given problem is, to narrow it down and finally fix it. But even as I run into this problem constantly I couldn't yet find a way to reproduce it on purpose every time. My guess at the moment is that it's a timing issue and that it happens if you work a bit faster.

I can just encourage one to try to work for just 5 minutes in the way I described, and I am very confident that you'll run into the problem. Alternatively, I will gladly record a screencapture-movie of my workflow and cut it down to a moment where you'll nicely and clearly observe the problem.

Another approach to the problem - actually all of the problems mentioned in this thread - what do you think: How about making the process of selecting the images about which to act on configurable? The OP made a nice ASCII-drawing at the top of this thread, describing the multiple ways about how the program makes this decision. What if each of the lines describing one method ("mouse over", "mouse inside table", etc) could be dis/enabled to fit everyone's needs? People who dislike mouse-over selection could disable it, and me and others could fix their problems as well? And the old argument "we cannot change it, because other people rely on it" would be obsolete, because if you don't mess around with this setting, it's just the old way some are used to.

AlicVB commented 1 year ago

I can just encourage one to try to work for just 5 minutes in the way I described, and I am very confident that you'll run into the problem. Alternatively, I will gladly record a screencapture-movie of my workflow and cut it down to a moment where you'll nicely and clearly observe the problem.

that would be great, because what you describe is clearly a bug. Can you open a new issue for that, thought, as it's not related to the discussion here and should be fixed independently of the act-on algorithm... Thanks !

airflow2010 commented 1 year ago

that would be great, because what you describe is clearly a bug. Can you open a new issue for that, thought, as it's not related to the discussion here and should be fixed independently of the act-on algorithm... Thanks !

Sure! Here you have two examples of the same problem. In the first example, I use the zoom-feature. In the second example, I don't. But the principle is the same. I used an additional software ("Screenkey") to display the keys I pressed during recording, so you can see exactly what I'm doing. In the comments of the video you see a more detailed explanation with the info of exact moment of problem.

I will open a new issue for that as requested, please just let me briefly know that you still agree that's actually a bug and not some kind of misunderstanding from my side.

AlicVB commented 1 year ago

yep, seems like a bug... When you fill the new issue, can you provide a step by step as detailed as possible, so we try to reproduce (I can't actually) Thanks !

rekcodocker commented 1 year ago

The worst offender, though, is when you purposely select multiple images (with shift+click and/or ctrl+click) and then accidentally apply changes to an unselected image just because you happened not to be hovering your mouse over any of your chosen images at the time you pressed the shortcut. That behaviour is just plain dumb

True, that makes no sense ever. Best (most logical) behaviour is:

I.e. clicking is a firm positive confirmation. Moving your mouse over something is also a meaningful action but should never override clicking.

ptilopteri commented 1 year ago

since inception this selection method has been the default in dt and I have become accustomed. but as an alternative, why not consider the image below the mouse as "selected" unless selected images exist? maybe that can be the best of both worlds?

github-actions[bot] commented 1 year ago

This issue did not get any activity in the past 60 days and will be closed in 365 days if no update occurs. Please check if the master branch has fixed it and report again or close the issue.

Nilvus commented 1 year ago

Just a thing, another issue related to that one: #14462. Maybe this should change and I'm with @elstoc here to change that. This is really needed to improve UI and user-friendly app. Just be like others apps here. I hope this could change for 4.6. @Solarer: you have worked on that (and culling mode). Possibility to see updates on your work?

Solarer commented 1 year ago

@Nilvus I created a PR in January for @AlicVB to review but I got no feedback so far. It is not perfect yet but I wanted to discuss the overall idea before spending more time on it. The PR is a prerequisite for this discussion and does not solve the act-on problems yet.

You can find it here: #13278 I will rebase the code after I received an 'ok' to proceed or feedback what to change.

rekcodocker commented 1 year ago

This is really needed to improve UI and user-friendly app. Just be like others apps here.

Exactly this. There is quality in consistency. Sometimes it is more userfriendly to not be special but be like other apps.

oijn commented 1 year ago

If I forget to "park" my mouse pointer off screen somewhere - when I select images in lighttable with arrow keys and spacebar - when the displayed image table is shifted as the page scrolls - the highlight jumps to the image under the mouse pointer and not the logical next image. This just add hundreds or on a large library thousands of extra arrow key presses just to go over the images you've already arrowed over and can add ten plus minutes to a simple selection task.

For the last many months - I just thought that this was a specific darktable bug - well at least I know now that this is a unique darktable feature.

elstoc commented 1 year ago

For the last many months - I just thought that this was a specific darktable bug - well at least I know now that this is a unique darktable feature.

Honestly for this one, I'd be tempted to use the term "intentional bug". I guess I'm used to it now after many years but it's still wrong.

Solarer commented 1 year ago

@elstoc and @Nilvus, I am still waiting for a review by @AlicVB on my PR but he does not seem to be very active on github since the beginning of the year. Do you know if he will be back soon or can someone else give feedback to the code?

AlicVB commented 1 year ago

I'm sorry to be so unavailable these days : my "real" job and some graduations are taking up all my free time... Especially in this case, as this review requires lot of time and thinking.

I still hope to do it this month (but I've already said that last month...)

github-actions[bot] commented 1 year ago

This issue 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 check if the master branch has fixed it and report again or close the issue.

londeril commented 11 months ago

Hello all,

Introduction

Longtime Lightroom Classic user here - I've only very recently looked into Darktable and I'm currently in the process of switching over to it because I love the image pipeline that much going back to Lightroom to try things out feels "stale" and quite "limiting" - so first of all THANKS to everyone who is or has written code for this amazing project!

The issue

I've been searching around the interwebs and this github because I found the "Act-on-hover" thing DT does quite disorienting. I understand the logic behind it and I can see the benefits of it (I'm a "focus follows mouse" guy since forever) so I can get my head around the fact that the mouse hover selects an image (though I'd still prefer it to be an option to turn this off and have "normal" click to select functionality). BUT - the "Act-on-hover" gets very weird and I think even "workflow impeding" when using the arrow keys to navigate the Lighttable view. The way I like to use the Lighttable is to have rather big thumbnails for a quick overview in my culling process and use the arrow keys to select the next image or go back to the previous one. this works fine as long as the Lighttable view stays "on the same page" as soon as the images get "pushed up" (or down) to make room for the next row - and the mouse hovers over an image (like in the middle of the screen) that image gets selected and pressing the arrow keys will select the next image starting from there... this is very confusing... and now that I write it down I realize that it's also quite hard to show in text... so I made a screen capture off it (with sample images to for privacy concerns, naturally): https://streamable.com/ze5n3g

This can be mitigated by simply moving the mouse out of the Lighttable (over a module, or over the window decoration) but I feel like this should not be necessary.

Yeah... well... in the end I just wanted to add this to the conversation - forgive me if that has been discusses before - this issue has been open for a long time, I see ;)

Solarer commented 11 months ago

@londeril thanks for sharing your finding. The issue you describe is not actually caused by the act-on code, so it can be fixed independently :wink:

Please open a new ticket and add your video there

londeril commented 11 months ago

@Solarer will do :)

github-actions[bot] commented 9 months ago

This issue 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 check if the master branch has fixed it and report again or close the issue.

ralfbrown commented 5 months ago

Please post any further discussion in #16850.