Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use / path separator on Windows in export dialog #17941

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gerritsangel
Copy link

I noticed that the export path on Windows starts with a forward slash, like

$(FILE_FOLDER)/darktable_exported/$(FILE_NAME)

Interestingly, after a directory is selected with the file picker, the folder name changes to double backslashes.

I think this is probably really weird on Windows, at least I never ever see this anywhere, and it's also quite hard to type. Instead, I therefore propose to just use the unix slash for the time being. At least I guess it would be an easy fix (probably might on the other hand have some side effects on Unix which i'm not sure about? Maybe need to have some OS specific logic here).

Imho best way would be to properly support backslashes like a normal Windows file path, but a / is nowadays also sometimes used, and Windows supports it without problems, so I guess it would be better than a double backslash.

@parafin
Copy link
Member

parafin commented Dec 3, 2024

As per comment just above your changes:

on other platforms it can be part of a regular folder name

So your change breaks an (improbable, but still possible) case on non-Windows systems with \ in the file/directory name. My suggestion is to guard these changes with #ifdef _WIN32.

But in general it would be nice to be consistent about used on Windows path separator - either use / as all normal operating systems, or stick with more native \ and just accept all the escaping nonsense it comes with.

@gerritsangel
Copy link
Author

So your change breaks an (improbable, but still possible) case on non-Windows systems with \ in the file/directory name. My suggestion is to guard these changes with #ifdef _WIN32.

Okay, will do.

But in general it would be nice to be consistent about used on Windows path separator - either use / as all normal operating systems, or stick with more native \ and just accept all the escaping nonsense it comes with.

Hmm yeah, of course I think having native file names is better. But \\ is not native at all. It does not even work in the explorer. If I type C:\\Users, the explorer is complaining that the file cannot be found. C:/Users work without problems though.

Probably I'm missing something, but why even need to escape \? The variable substitution is $(), so how does it clash with the \ ?

So, as said, what I was now thinking about is to have it at least consistent with the startup screen, when there is no file picker used yet. Because then also on Windows it shows forward slashes. I don't think it's perfect solution, but at least (for now) better than \\.

I guess it's probably a different discussion :D but I guess to have the application really feel like a nicely integrated application on Windows, it should not expose any "normal operating system" design decisions, which Windows users.

@wpferguson
Copy link
Member

Probably I'm missing something, but why even need to escape \

\ is the escape character, so to prevent it from escaping the next character you have to escape it. In Lua the escape character is % so if I want % I have to use %%.

@gerritsangel
Copy link
Author

Ah okay, I think now I get it.

Linux:

  • /home/theuser/pictures -> no problem
  • /home/theuser/$(EXIF_YEAR) -> no problem if I want to have it rendered as /home/theuser/2024.
  • But if I want to have it exported to the folder /home/theuser/$(EXIF_YEAR), then I would have to write there the export folder /home/theuser/\$(EXIF_YEAR), to not use variable substitution. And if I now replace \ by /, then it becomes /home/theuser//$(EXIF_YEAR), which is not the same anymore. So all the backslashes get escaped by \\. And probably \ is not used so often on Unix, it's not a real issue that it appears with \.

On Windows on the other hand:

  • I cannot have \ nor / as a folder name, so it would be fine to replace all the \ in the file with /.
  • But, the user can still use variables substituion in the input box, so the \ in the whole manual written box does not work.

So I guess for Windows user this export box is highly problematic:

  • If you write a path there manually or copy paste it from Explorer, the path would look like C:\Users\theuser\Pictures. But Darktable then treats \ as an escape character, even if there is nothing to escape there.
  • If you use the file picker, it converts it automatically to \, which works in Darktable, but is not a proper Windows path.

I guess the original problem here is that Darktable seems to store the path as some kind of templating string/expression language, which is also directly used for display, which causes the issue. If the underlying storage is separated from how to display it, it might be easier.

But not to rewrite everything, I then suggest to - only on windows - replace all backslashes in the result from the GTK file picker with forward slashes, and after that append any variables templates to it.

  • Then there is no change on Linux
  • On Windows, there is no problem because there cannot be a \ in a file path which would be ambigously converted to /, and the result from the file picker is always a path, and never contains any variables inside.
  • The user sees that Darktable in this box works with forward slashes. Still not perfect, but probably a bit "better" than the double backslashes. At least, it would be a working path in the explorer.

@gerritsangel
Copy link
Author

I have now updated the pull request, it should keep the original behaviour on Unix and only adjust the filename on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants