Project

General

Profile

Bug #11850

Filmroll path incorrect when importing from camera

Added by Gareth Williams 11 months ago. Updated 10 months ago.

Status:
New
Priority:
Low
Assignee:
-
Category:
Lighttable
Target version:
Start date:
12/10/2017
Due date:
% Done:

0%

Affected Version:
2.4.0rc0
System:
Windows
bitness:
64-bit
hardware architecture:
amd64/x86

Description

After a fresh install of Darktable 2.4.0rc0 on a fresh VM, with no configuration options changed; I attemtpted to import from my camera.

Unfortunately, the filmroll path fails to translate the $(YEAR) variable into the year (2017) resulting in a path of:

C:\Users\<user>\Pictures/Darktable$(YEAR)1210_noname

instead of:

C:\Users\<user>\Pictures\Darktable\20171210_noname

Note that $(MONTH) and $(DAY) work as expected.

ImportPathError.PNG - Filmroll Path (3.27 KB) Gareth Williams, 12/10/2017 06:15 PM

Filesystem.PNG - Filesystem (3.06 KB) Gareth Williams, 12/10/2017 06:17 PM

0001-Use-instead-of-G_DIR_SEPARATORS_S-for-Windows-build-.patch Magnifier - Patch (978 Bytes) Gareth Williams, 01/04/2018 01:47 PM

History

#1 Updated by Roman Lebedev 10 months ago

  • Target version changed from 2.4.0 to 2.6.0

#2 Updated by Peter Budai 10 months ago

Confirmed, this function indeed are not functioning on Windows as expected

#3 Updated by Gareth Williams 10 months ago

It seems that this issue is due to the fact that DT uses the backslash character ("\") to escape any variable expansion, which conflicts with the use of the same character as a directory separator in Windows.

Line 593 of common\variables.c checks for a "\" and skips over the next character if it finds it. If the next character is the start of a variable, then this effectively skips the expansion of that variable.

That is, the default location is $(PICTURES_FOLDER)/Darktable which is concatenated with "\" and $(YEAR)$(MONTH)$(DAY)_$(SEQUENCE).$(FILE_EXTENSION) to generate the file path for an import. The "\" escapes the succeeding "$" and consequently $(YEAR) isn't expanded.

As it is impossible to ascertain whether the "\" is a directory separator or an escape character (in the case that someone really does want a "$(" in their filename) then the only logical option is to use the "/" character as the directory separator when concatenating the paths on Windows instead of G_DIR_SEPARATOR_S.

Small patch attached.

#4 Updated by Tobias Ellinghaus 10 months ago

After thinking about this for a while I too think that just fixing the separator is better than messing with the whole path afterwards. However, instead of using / on Windows I would use \\, i.e., escape it properly.
Peter, what do you think?

#5 Updated by Peter Budai 10 months ago

The problem is that the variable escape character is the same character as dir separator on Windows :) We need to change one of those.

In that sense one option is to instruct users to use double '\' for path separator. But I feel that this would be a bit error prone, and also as the default is still $(PICTURES_FOLDER)/Darktable, so we need to have proper instructions in place for each platform to avoid bug reports complaining that that it is not working.

Another option what Gareth is suggesting: use always '/' for manual concatenating of base dir and subdir, don't change the user instructions. But that means that after doing the the variable expansion the path should be modified on Windows and replace remaining '/' characters with '\\'

Third option would be to change the variable escape character, but that would be too much change IMHO.

Currently I'd prefer the second (Gareth suggestion). Maybe you have good arguments for the first option (double '\')?

#6 Updated by Gareth Williams 10 months ago

Just some thoughts...

For '/':

'dartablerc' already uses a forward slash as a directory separator at line 469. That one cannot change as it would break compatibility with Linux builds. If you're going to have one forward slash in the path, you may as well have many? It seems that forward slashes are converted automatically to back slashes by Glib (or whichever library would do this) on Windows.

For '\\':

Unfortunately, using a '/' breaks the names of film rolls (in recently used collections, for example) as the code for this breaks paths down on the G_DIR_SEPARATOR_S. When a '/' is used instead, we end up with long film rolls names, such as 'Pictures/Darktable/20180104_noname' instead of simply '20180104_noname'. My previous patch does this I'm afraid. We'd need to chase those down and change G_DIR_SEPARATOR_S to '/' to fix it. Using `\` as the separator allows correct display of film rolls.

For a new escape character:

While a revised version of the patch above (using double back slashes) works, it probably only works for the camera import. I'm sure there are other functions/areas which uses variable expansion within DT and we'd need to fix all/most of those to use double back slashes instead of G_DIR_SEPARATOR_S too. This could bring up other issues.

Although not Peter's favourite option, a new Windows-only escape character would work. Chances are that nobody is currently using variable expansion and escape codes within Windows as it doesn't work properly. If it was changed quickly, this could possibly be done before it affects anyone? The only downside is that users would need to be told of this change. We could use any of the many Windows forbidden characters: <>:"|?* (ignoring /\ as they're already causing trouble!). I've just tested using a pipe character '|' for the escape character and it worked.

#7 Updated by Tobias Ellinghaus 10 months ago

The escape character is not going to change. \ is the standard for that, known since decades to users, so we are using that.

Using / has the problem that is seems to somehow end up in the database, breaking things down the line.

My hope is that using \\ just magically works.

Ideally the string shouldn't end up in the database but be normalized. I am not familiar with that part of the code and would have to check how it's being used at all.

#8 Updated by Peter Budai 10 months ago

I have an alternative proposal describe here, could you check and let me know what you think:
https://github.com/darktable-org/darktable/pull/1601#issuecomment-355734373

Also available in: Atom PDF