Filmroll path incorrect when importing from camera
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:
Note that $(MONTH) and $(DAY) work as expected.
#3 Updated by Gareth Williams almost 2 years ago
- File 0001-Use-instead-of-G_DIR_SEPARATORS_S-for-Windows-build-.patch 0001-Use-instead-of-G_DIR_SEPARATORS_S-for-Windows-build-.patch added
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.
#5 Updated by Peter Budai almost 2 years 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 almost 2 years ago
Just some thoughts...
'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.
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 almost 2 years ago
The escape character is not going to change.
\ is the standard for that, known since decades to users, so we are using that.
/ 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 almost 2 years ago
I have an alternative proposal describe here, could you check and let me know what you think: