darktable icon indicating copy to clipboard operation
darktable copied to clipboard

RFC: Fix "overwrite if modified"

Open Sturmflut opened this issue 2 years ago • 8 comments

Turns out the code that made it into https://github.com/darktable-org/darktable/pull/15033 doesn't actually check if the output file exists or not. For some unclear reason this was not caught during testing.

While we're at it, rename the option to "overwrite if modified". "changed" could otherwise be misunderstood as the exported file having changed on disk.

Sturmflut avatar Sep 20 '23 22:09 Sturmflut

For some unclear reason this was not caught during testing.

Probably because if the file doesn't exists it has not been exported and then export_timestamp is not > change_timestamp and so it gets exported. Adding the new check for file existence seems ok.

One question, are you actually trying to fix an issue you've experienced? If some can you explain way to reproduce. TIA.

TurboGit avatar Sep 21 '23 09:09 TurboGit

Yes, this fixes an actual error. Steps to reproduce:

  1. Export an image to disk
  2. Switch the "on conflict" setting to "overwrite if changed"
  3. Delete the exported image file on disk
  4. Export the image again (without modifying the image inside darktable between step 1 and 4)

What should happen:

  • The image is exported to disk a second time. The "on conflict" setting only affects the behaviour when the file already exists on disk, but in this case it doesn't. If there is no conflict, the file always has to be exported.

What happens instead:

  • The image is not exported even though there is no conflict.

Sturmflut avatar Sep 29 '23 00:09 Sturmflut

What happens instead:

* The image is not exported even though there is no conflict.

Which can be a valid scenario when using "overwrite if changed". You export in a directory and upload the files to some Web server and then delete them. Then you want to export only those modified, the new files gets created and you upload only those.

Maybe we want another option?

TurboGit avatar Sep 29 '23 07:09 TurboGit

What happens instead:

* The image is not exported even though there is no conflict.

Which can be a valid scenario when using "overwrite if modified". You export in a directory and upload the files to some Web server and then delete them. Then you want to export only those modified, the new files gets created and you upload only those.

Maybe we want another option?

I think we are crossing into the territory of the lightroom here. It makes sense to keep image selection in the lightroom and the "on conflict" option in the disk module, since the former is also useful in many other aspects than just choosing which images should be exported, but the latter is specific to a technicality only present in the disk exporter (the possibility of there being export conflicts based on filenames).

I can come up with at least the following filters for selecting which images should be part of an export:

  • all images in the database
  • all images currently visible in the lightroom (has its own filtering system)
  • all images part of the same film roll
  • a manually selected list of random images (possibly from multiple film rolls)
  • all "untouched" images (no modifications since first imported into the database)
  • all "touched" images (modified since first imported into the database)
  • all "modified since last export" images
  • all "not modified since last export" images

The "all images currently visible in the lightroom", "all images part of the same film roll", "a manually selected list" and "all untouched images" options already exist as dedicated buttons in the "select" dialogue. Plus there is an "invert selection" button, so the "all touched images" option also already exists indirectly. Basically all that is missing is a "select modified" button in that dialogue and all the options I've mentioned above exist.

I've given a shot at implementing a "select modified since export" option in the branch https://github.com/Sturmflut/darktable/commit/88efc077dba11694ad709f7f8f1942c5a60a655e . I think it should be treated separately from this bugfix merge proposal here.

P.S. I think we've realised a couple of months ago on some developer chat that the "select untouched" option does not work as expected. I might look into that at some other time.

Sturmflut avatar Sep 29 '23 18:09 Sturmflut

I've been thinking about this and really cannot come up with a proper solution... What do others think about this? We need more feedback... TIA.

TurboGit avatar Oct 12 '23 13:10 TurboGit

We need more feedback...

To me, overwrite if modified makes things unnecessary complicated. I am happy with the 3 remaining

  • create unique filename
  • overwrite
  • skip

If the file already exists and overwrite is chosen, then it simply gets exported again, even if there are no modifications.

I have a bunch of images selected for export and now I expect to find them at the specified location.

zisoft avatar Oct 12 '23 14:10 zisoft

If the file already exists and overwrite is chosen, then it simply gets exported again, even if there are no modifications.

That's correct, but this is also unnecessarily wasteful if you have a lot of images and only a few have been changed. Even with a fast OpenCL GPU 45.7 MP Nikon RAWs can take several seconds in full resolution.

I am in favor of dropping "overwrite if modified" if the "select modified since export" button gets merge instead. So basically:

  • revert 39ca4b26096818d1b90da8f324149c59942f10d0
  • keep b149cefd854e8eeb5026bedf75acab78a36751f6
  • merge https://github.com/Sturmflut/darktable/commit/88efc077dba11694ad709f7f8f1942c5a60a655e

Sturmflut avatar Oct 21 '23 16:10 Sturmflut

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.

github-actions[bot] avatar Dec 21 '23 00:12 github-actions[bot]