Project

General

Profile

Bug #9712

Moving images in lighttable mode to a different directory moved the wrong images.

Added by Josef Wells over 5 years ago. Updated over 4 years ago.

Status:
Fixed
Priority:
High
Assignee:
-
Category:
Lighttable
Start date:
12/07/2013
Due date:
% Done:

100%

Affected Version:
git development version
System:
other GNU/Linux
bitness:
64-bit
hardware architecture:
amd64/x86

Description

Tried moving some images from one filmstrip to another in lighttable mode. The incorrect images were moved, and subsequent moves also moved non-selected images.

Restored my database and tried again, same result (same images moved, resulted in the same incorrect images moving)

Debian Experimental darktable 1.4~rc1-1

foo.png (296 KB) Richard Levitte, 12/31/2013 12:27 PM

cntl_job_selection_fix.diff Magnifier (959 Bytes) Sergey Pavlov, 10/09/2014 12:32 AM

Associated revisions

Revision a7918591
Added by Johannes Hanika over 4 years ago

jobs: get list of images first, then ask ui stuff. fixes #9712

History

#1 Updated by Pascal Obry over 5 years ago

  • Status changed from New to Incomplete
  • % Done changed from 0 to 20

We will need more information as I cannot reproduce on my side.

#2 Updated by Josef Wells over 5 years ago

  • Status changed from Incomplete to Closed: invalid
  • % Done changed from 20 to 0

I Tried it on another computer and could not get it to happen again.

I then went back, restored my backup database, and tried the same thing.. could not reproduce.

Freaking gremlins on the wing, I'm telling you.

#3 Updated by Adrián Goig over 5 years ago

Hi everyone,

First of all, thank you for Darktable and thank you for your support.

I have the same problem. When I have selected an image in lightable mode and I use
the "move" button to move the image from the original folder to another, it moves the
wrong file and it doesn't appear to perform it following any pattern. It seems to me that
Darktable move a random image to the target folder.

I'm using Darktable 1.4 on Ubuntu 12.04 64 bits.

#4 Updated by Pascal Obry over 5 years ago

Again will need far more information. I'm using the move feature extensively since almost a year now and I have never had a single problem. This ticket is closed, if you have a way to reproduce please send it otherwise this issue won't be fixed!

#5 Updated by Richard Levitte over 5 years ago

I've had a similar experience previously. I'll see if I still get that and get back to you. Or should I create a new issue for it?

#6 Updated by Richard Levitte over 5 years ago

Ok, I've been able to reproduce this bug consistently.

First of all, there are three prerequisite:

1. that the option ask_before_move is TRUE
2. for some of the tests, have a film roll that's big enough to fill the light table.
3. darktable is maximized (covers the whole screen).

I've done three distinct tests, which are all about where the selection is on screen:

1. Select a few images (one line of them) at the top of the film roll, then try to have them moved.
2. Select all visible images in the film roll, the try to have them moved.
3. Go to the end of the film roll, far enough that not all the visible light table is filled.

Results:

1. FAIL. The image that was under the "Open" button of the directory selector is moved instead of those I had selected. I noticed that this image "blinked" (was selected as if the mouse hovered there) just after I pressed "Yes" in the confirmation dialog. None of the images that I had selected were moved.
2. WORKED. Although I saw a behavior similar to the above (the image that was directly under the "Open" button "blinked"), all the images I selected I moved. I can only assume this is because that image was part of my selection anyway.
3. WORKED. I can only assume that this was because there was no image directly under the "Open" button.

I've attached an image that shows test 1 directly after I've pressed the "move" button in dt. You'll notice that there are a number of selected images. None of them will be moved, but the one down to the right (not selected) will.

This bug seems to be a race condition similar to the one fixed for remove/delete in pull #403

Please re-open this issue. Also, maybe targetting the next MAJOR release is taking it a bit far. This is quite clearly a bug that should be fixed soon.

#7 Updated by Adrián Goig over 5 years ago

Thank you Richard!

I tried your prerequisites and I found you are right. Altough is not THE
solution, just minimizing the window or setting in configuration the option
"ask before move" off, it is possible to use the move button with good results.
It is not THE solution but is A solution.

#8 Updated by Richard Levitte over 5 years ago

Yeah, that's not a bug correction, just a workaround ;-)

#9 Updated by Pascal Obry over 5 years ago

  • % Done changed from 0 to 20
  • Status changed from Closed: invalid to Triaged

#10 Updated by Pascal Obry over 5 years ago

I tried hard to reproduce, even adding to sleep() in the code in the hope to trigger the race condition... No luck! I just can't reproduce. What's your OS? And more importantly what is your Window Manager? And looking at the code I do not see the same kind of race condition I have fixed for the remove/delete actions.

#11 Updated by Richard Levitte over 5 years ago

My OS: Debian GNU/Linux [unstable]
My wm: CTWM, latest possible version, and setup with focus-on-mouse (I imagine that might be important, although not sure how)

#12 Updated by Pascal Obry over 5 years ago

My wm: CTWM, latest possible version, and setup with focus-on-mouse (I imagine that might be important, although not sure how)

Oh yes, looks like this is it. Can you check without this option?

I'm also on Debian GNU/Linux (unstable) but using GNOME Shell and I don't see a way to set the focus-on-mouse option.

#13 Updated by Richard Levitte over 5 years ago

I added the ClickToFocus option, restarted ctwm, same result.

I'll have another look in the source, see if I can pinpoint where it goes wrong...

#14 Updated by Josef Wells over 5 years ago

I'm also debian unstable, but using Gnome "flashback" with SloppyFocus. I'll see if I can reproduce the bug using Richard's instructions. It is very likely that when I attempted to reproduce I did not use fullscreen mode, but that when I originally encountered the issue I did.

#15 Updated by Bernd Hausmann over 5 years ago

Hello,

my observations, below.

Ubuntu 12.04, DT 1.4, Gnome-Shell 3.4.1
Window Focus mode = Click

Conditions:
- darktable in maximized window
- setting 'ask before move file' = true
- folder with 100+ files

Observations:
1. There are four pictures per row. If I select the first OR the second OR the third OR the First AND the second, always the last (fourth) picture of the row gets moved.

2. Whole row selected -> the whole row gets moved.

3. full row and the following picture of the second row selected -> all selected pictures get moved.

4. the first two pictures of the first row and the first picture of the second row are selected -> The last picture of the first row gets moved only.

5. when a picture in any other row of the collection is selected -> the last one of the first row gets moved.

Changing window size and/or setting 'ask before move file' does not affect above results.

Please let me know if you need further specific tests to be performed or additional information regarding my system setup.

Bernd

#16 Updated by Romano Giannetti over 5 years ago

Pascal Obry wrote:

I'm also on Debian GNU/Linux (unstable) but using GNOME Shell and I don't see a way to set the focus-on-mouse option.

To set focus-follow-mouse on Gnome, run gnome-tweak-tool, and in the category "Windows" select "Sloppy" focus method. (Why they decided that two configuration tools are better than one is beyond me).

#17 Updated by Christian Rosencreuz about 5 years ago

I'm having same problem on Ubuntu 13.10 with unity and (default installation). Darktable 1.4.1

Workaround from #6 works for me.

#18 Updated by Bernd Schuhmacher almost 5 years ago

I found the same problem with DT 1.4 on Kununtu 14.10.
Simple work around is to move the "ask bevor move file" box on to a space where no picture is under the "ok" Button.

#19 Updated by Thomas Guillet over 4 years ago

I just experienced this issue with image copy on the latest git (f0ee8af right now).

Looking a bit into the code, looks like the problem could come from an interaction between mouse pointer movement during/between GTK dialogs, and dt_view_get_image_to_act_on().

I'd guess what happens goes as follows:
  1. Copying/moving an image in the GUI triggers dt_control_copy_images()
  2. dt_control_copy_images() first opens a directory selector dialog, then asks for confirmation for the copy. Then, dt_control_generic_images_job_create() is called to setup the copy job.
  3. dt_control_generic_images_job_create() calls dt_control_image_enumerator_job_selected_init() to setup the list of images.
  4. dt_control_image_enumerator_job_selected_init() calls dt_view_get_image_to_act_on().
  5. Now, according to comments in dt_view_get_image_to_act_on():
    • if the mouse cursor happens to be over one of the images from the selection, the whole selection will be returned and we get the expected behavior,
    • if the mouse cursor sits over an image that's NOT in the selection, this image gets selected instead, and everything behaves as if the user had selected that particular image only.

However, at this point, the mouse cursor location can probably be anywhere on the screen around one of the GTK dialogs buttons. Depending on how fast the GTK dialogs pop up, whether the user is moving the mouse, etc..., the mouse movement may have already updated darktable.control->mouse_over_id to point to an image outside of the initial selection.

Maybe ignoring the mouse pointer altogether (whenever dialogs need to be opened between image selection and processing) would fix these issues?

#20 Updated by Sergey Pavlov over 4 years ago

It seems like there is an logic insonsistency in dt_control_image_enumerator_job_selected_init().
Since all group operations in conrtol_job.c initially check only explicitly selected image by
calling dt_collection_get_selected_count() there is simply no reason to use dt_view_get_image_to_act_on()
in dt_control_image_enumerator_job_selected_init(). Operations in conrtol_job.c simply don't intende to
work with "hovered" images.

So there is a simple patch in attachment.

#21 Updated by Thomas Guillet over 4 years ago

Hi Serguey,

Thanks a lot for that patch, I've done a few quick tests on my side, and it seems to fix the "moving mouse messes with selection" issue.

Now, unfortunately, I think a race condition still remains: the (global) darktable selection may still change between the moment one issues the copy/move operation, and the moment the job is actually executed. For example, try this:
  1. In light table mode, select, say, 5 images
  2. Do "selected images > copy"; folder selection dialog opens.
  3. Navigate to some folder. Before validating and closing the dialog by clicking "Open", change the selection in darktable in the background, e.g. by clicking now on a single image. This is possible since the open dialog is not modal.
  4. Confirmation box asks: "do you really want to physically copy the 5 selected images to /tmp?". Click "Yes"
  5. darktable will now only copy the single image you selected during step 3, instead of the 5 images of step 1.

So, ideally, the selection should be captured at the moment the job is created (i.e. in the first stages of dt_control_copy_images()), and a copy of this selection passed to the job, instead of pointing to the global darktable selection object. This seems like the cleanest way to avoid this kind of race condition.

A dirty workaround would be to make the GTK directory selection a modal dialog, but that doesn't really address the underlying race condition, which could then be triggered by other code paths which modify the selection.

Thomas

#22 Updated by Sergey Pavlov over 4 years ago

Hi Thomas,

thanks for your previous investigation. The issue quite annoys me too so you help me to find a possible fix. However I am not sure that it will be approved by core team.

Now regarding the additional issue which you have described. In fact "directory selector window" must be modal and in normal conditions it is modal. But you probably has faced with the bug introduced by Ubuntu "overlay scroll bar" (see https://bugs.launchpad.net/ubuntu/+source/overlay-scrollbar/+bug/903302). It can be solved:
  1. by setting environment variable before DT start:
    export LIBOVERLAY_SCROLLBAR=0
  2. by disabling the "overlay bar" for all application using "gsettings set com.canonical.desktop.interface scrollbar-mode normal" command (to enable gsettings set com.canonical.desktop.interface scrollbar-mode overlay-auto)

#23 Updated by Thomas Guillet over 4 years ago

Thanks Sergey, you were right: my GTK window bug (non-modal instead of modal) was related to that particular overlay-scrollbar bug with Ubuntu. Since the problem affects critical RAW file management functions in darktable, I've opted temporarily for an ugly (but hopefully safe) solution, which is to add:
if(setenv("LIBOVERLAY_SCROLLBAR", "0", 1)) exit(1);
in the main.c of my darktable sources. This indeed makes the directory selection dialog modal, and hopefully prevents changing the selection from the GUI before the job runs.

I think it would still be nice to capture the selection right after the user clicks the "copy" or "move" buttons, to avoid unexpected pitfalls and race conditions of that kind. However, this would mean changing a number of things in control_jobs.c in a way that's not obvious to me :)

In the meantime, looks like at least Ubuntu users are exposed to dt file management issues because of the overlay-scrollbar bug.

Regards,
Thomas

#24 Updated by Johannes Hanika over 4 years ago

Sergey Pavlov wrote:

So there is a simple patch in attachment.

yes, i think this is not the right way to fix it.

i think the way to do it would be to collect the list of images to act on just before initing the gtk_message_dialog_new to ask the user for random unnecessary things. this will avoid the selection getting confused by mouse movements and hovering over images when clicking a button.

currently the image list is not collected until inside
dt_control_generic_images_job_create(). the easy fix would be to create the job using this very function before constructing the user interface stuff, and destroying the job in case the user declines the dialog message (this cleanup() function will have to be written and needs to free the GList constructed in the process to avoid a memory leak).

#25 Updated by Johannes Hanika over 4 years ago

  • Status changed from Triaged to Fixed
  • % Done changed from 20 to 100

marking as fixed (a791859), let me know if it's not.

#26 Updated by Sergey Pavlov over 4 years ago

Sorry, but with the latest fix in it is simply improssible to move files. After confirmation of "move" operation it fails with message "failed to create film roll for destination directory, aborting move...".

Regarding the fix by itself. I agree that collecting images before modal dialog init is good approach. But usage for such purpose dt_view_get_image_to_act_on() looks strange for me. If something is intended to work only with explicitly select images then why we are trying to use for this purpose dt_view_get_image_to_act_on()? This function is kind of "smart chooser" between explicit selection and hovered selection but it contradicts to the part of code which checks explicitly selected image. For example in dt_control_delete_images() there is still peace of code which work exactly with explicit selection

     int number = dt_collection_get_selected_count(darktable.collection);
     // Do not show the dialog if no image is selected:
     if(number == 0) return;

As I understand the main idea of dt_view_get_image_to_act_on() is to provide some kind of "prioritized" selection for operations triggered by hotkey. But for such destructive operation like "move" and "delete" usage of such approach is disputable. For example even right now attempt to delete hovered image with one explicitly select image using hot key ("Delete" key) produces quite ambiguous modal dialog. It is simply impossible to understand what image would be removed after confirmation because hovered selection is deactivated at this time.

#27 Updated by Richard Levitte over 4 years ago

Just tested, I get the same result as Sergey Pavlov above.

The main issue for moving images is, btw, in dt_control_move_images(), where dir is NULL when the job is created at the beginning. Somehow, dir needs to be passed to the job after it has been assigned the name of the directory to move the images to.

This issue needs to be reopened.

#28 Updated by Simon Spannagel over 4 years ago

  • Status changed from Fixed to Triaged
  • % Done changed from 100 to 20

#29 Updated by Richard Levitte over 4 years ago

I created a fix for the functions that have the "failed to create film roll" issue (moving and copying images are afflicted). See pull request #768.

#30 Updated by Richard Levitte over 4 years ago

  • Status changed from Triaged to Fixed
  • % Done changed from 20 to 100

Someone else was quicker than me, see commit 8dce7d6732724b2e1a87b9190ca511dab93d1838. That seems to fix this remaining issue.

Also available in: Atom PDF