Solarer/split culling mode #13278
Solarer/split culling mode #13278
Conversation
|
@AlicVB it has been some time but I found some time to code recently. Can you have a look at this? |
|
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. |
|
@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) imho, we have 1 main issue here :
I fear that I still fail to understand the reason behind this change... (what is blocking exactly for your new act-on algo ?) |
|
To go further with my difficulty to understand, I've read again PR #11112 where you say :
esp. the last sentence : what exactly would not work anymore ? I can only see one :
I really think I miss something... |
That was unintended, my bad. I will fix that.
That is the point and your second bullet point is how I want to solve it:
For culling restricted I will ignore selection in |
|
great ! Thanks for the details... |
|
This PR should probably be rebased, the diff is, uh, |
|
Oh, I didn't mean to push to this repo. So I did not check it. |
Solarer
force-pushed
the
solarer/split_culling_mode
branch
from
6e9b5fd to
de7deb2
Compare
|
Solarer
force-pushed
the
solarer/split_culling_mode
branch
from
de7deb2 to
b072c1f
Compare
|
... forgot to squash my commits |
|
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. |
|
still active and waiting for review from @AlicVB |
|
I've just done another try... and I think there's still some issues :
I've not looked in the code to find where the issues come from, but I can if you need... |
|
Thanks for finding the time.
|
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) |
|
I've done a quick test. Just to be sure, I've spotted 2 changes :
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........ |
Yes, that was present starting with my first PR. All other view modes have a key binding and since culling starts with
Err... honestly, I never noticed that before and I worked on this topic for 2 years... Can we just keep it the way it is now? It is just more straight forward. If someone selects images and then wants to enter unrestricted culling, they can now press
Not planned. Act-on will continue to work only on the 1 full screen image. Since the filmstrip is hidden by default, nobody should be surprised that you interact with the one image the screen is showing you. |
As soon as it's still not the case for full preview, I'm not sure it's really a critical issue. Somme more remarks :
|
I implemented "c" to act like a switch to activate or deactivate the restricted movement. Since it is not a real view mode (at least it is not listed with the other ones in the UI) it did not feel right when it could be used to return to filemanager layout. I tested both and found this to be more natural.
We lost that some 2 years ago but nobody noticed 😅. It was not my doing. What is selected or not does not matter for this PR here - but it will matter for the act on code later. I suggest to revisit the synchronisation when we work on that.
If a mouse-over is available, the code tries to start culling there. Since "x" forces the restriction to selected images it snaps to the next selected image. If we are past the final image in the selection it snaps back. |
|
Pushed a refined version. Also removed a |
From an user POV, I think that this can be really confusing, as "X", "F", etc don't work like that. But that's not really critical. Let's see what other think...
Oops... you're right. That's an indication of an unneeded feature, I guess ;) Maybe we can clean up last bit of related code then...
iirc, the code you mention is manly used to move to the nearest image when browsing in a "restricted" mode. That point should remain. But if possible, I agree with you that for the "starting" case, it would be better to go to the beginning... |
|
Ok, I believe that is all for now. So I will wait for your approval and merge now that 4.8 is out |
|
Well, my 3 comments above are still actual I think... Esp, it would be great to have some opinions on :
And to fix :
@wpferguson (as you have volunteered to review), @TurboGit, etc |
|
@AlicVB I already implemented that fix in my last commit. It should now start with the first image. About the
|
Oh great ! no idea why I have missed that, I may have done the test before your last commit... Sorry ! So there's just the |
|
Testing... These are some observations from just trying to use it. Did someone request the restricted/unrestricted mode? Toggling in and out of it can be rather disorienting. For instance:
My thought process about restricted mode is if I make a selection then go into culling mode, culling should be restricted to the selection. Why would I go to the effort of making a selection before culling if I didn't want culling restricted to the selection? |
There was a problem hiding this comment.
After some more test in "real work" with a lot of images, I came to the conclusion that we lost here a quite important feature when using shortcuts :
Actually, when you have a selection set (let's say 10 selected images of 50), if you hover an image from the selection, you enter culling in restricted mode. If you hover an image outside this selection you enter culling in unrestricted mode, starting with the image hovered.
That quite handy imho, and this is consistent with the rest of darktable : fullpreview, darkroom, etc...
The proposed changes allow to keep this feature intact for current algo, and is future proof for a new algo (here "selection driven act_on")
| int sel_count = 0; | ||
| sqlite3_stmt *stmt; | ||
| // clang-format off | ||
| DT_DEBUG_SQLITE3_PREPARE_V2(dt_database_get(darktable.db), | ||
| "SELECT count(*) " | ||
| "FROM memory.collected_images AS col, main.selected_images as sel " | ||
| "WHERE col.imgid=sel.imgid", | ||
| -1, &stmt, NULL); | ||
| // clang-format on | ||
| if(sqlite3_step(stmt) == SQLITE_ROW) sel_count = sqlite3_column_int(stmt, 0); | ||
| sqlite3_finalize(stmt); |
There was a problem hiding this comment.
if we replace all that by : int sel_count = dt_act_on_get_images_nb(TRUE, FALSE); that will :
- simplify the code
- avoid current behavior change with current algo
- be future proof once the new algo is done as it will propose another way to compute
dt_act_on_get_images_nb. esp in the "selection driven algo", it will return the nb of selected images
| int sel_count = 0; | ||
| sqlite3_stmt *stmt; | ||
| // clang-format off | ||
| DT_DEBUG_SQLITE3_PREPARE_V2(dt_database_get(darktable.db), | ||
| "SELECT count(*) " | ||
| "FROM memory.collected_images AS col, main.selected_images as sel " | ||
| "WHERE col.imgid=sel.imgid", | ||
| -1, &stmt, NULL); | ||
| // clang-format on | ||
| if(sqlite3_step(stmt) == SQLITE_ROW) sel_count = sqlite3_column_int(stmt, 0); | ||
| sqlite3_finalize(stmt); |
iiuc, this is needed, because in the context of a "selection driven act_on" algo, as soon as some selection is set, there would be no way to enter culling in unrestricted mode. @Solarer : I'm right ?
I tend to agree with you here... Maybe we can find a middle way by making the "rectricted" icon just an indicator and not a switch button ? I'm not sure the switch between restricted and unrestricted mode is useful in any workflow (apart to correct an initial mistake of the user, but in this case exit/reentering culling is not so hard...) |
|
@wpferguson and @AlicVB: I am on vacation for 2 weeks without a computer so I cannot test your findings right now. I will come back to it once I am back at home.
The main reason for the split is that a selection driven act_on will always prioritize selected images if they exist. In the culling workflow you always have some selected images to restrict your movement but you still want to be able to apply star ratings to INDIVIDUAL images from that selection - if selection is prioritized all would receive the same rating. So I need to make an exception in the algorithm in case selection in culling is used for movement restriction. The split allows me to check for that and act accordingly. @AlicVB was referring to the people who use the culling view mode like file manager layout. They enter without selection, move freely around the collection and select images for export and other actions. When they enter another view mode and return, they are stuck in the selection. The button/shortcut gives them a way back to unrestricted movement. |
|
The shortcut is not necessary for the act_on code but that is just 2 more lines of code to fix the dead lock I just described. And I am rewriting that code anyways. Whether it's a toggle or works the same as I can confirm findings 1 and 3 of @wpferguson and know how to address them. |
|
Back from vacation
fixed. (Regarding the disorientation you mentioned I would like to point out that there is no need to press
fixed
That will not work. We are trying to decide whether we want to enter restricted or unrestricted mode and that decision is not about the number of images that will be acted on but rather whether a selection is present or not. If 1 image is selected, we want to enter culling restricted. If 0 images are selected but 1 image is hovered over by the mouse we want to enter unrestricted. Yet, it both cases |
|
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. |
When implementing culling (and reworking full preview) there has been a design decision to not consider selection of 1 image as a "real" selection but more as a starting point, so to enter unrestricted mode in this case. Is there any imperative reason to change that in the context of "selection driven" algo ? |
|
Ok, then it should work in case of hovering 1 image. |
|
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. |
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:
xenters culling. If images were selected, movement is restricted to selected images. If no images were selected, movement is not restricted.Changes:
lockbutton 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.ctoggles the movement restriction on and off. If you are not in culling mode already, enter culling.Issues:
(a first draft was attempted in PR #11112. I will leave that PR open for reference until this one has been merged.)