-
Notifications
You must be signed in to change notification settings - Fork 669
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
Windows: Use the application icon for the sidebar #5955
Conversation
One thing to think about: do we allow the default ignore list to be edited in order to allow syncing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
desktop.ini
is in the default sync exclude list and cannot be removed. (altought maybe we want to hardcode the rule to be sure)
src/libsync/utility_win.cpp
Outdated
qCInfo(lcUtility) << "Creating" << desktopIni.fileName() << "to set a folder icon in Explorer."; | ||
desktopIni.open(QFile::WriteOnly); | ||
desktopIni.write("[.ShellClassInfo]\r\nIconResource="); | ||
desktopIni.write(qApp->applicationFilePath().toUtf8().replace('/', '\\')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should any other character be escaped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not escaped (besides the C++ literal), it converts slashes to backslashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right.
Then maybe QDir::toNativeSeparators would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure UTF-8 is the right encoding for desktop.ini?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, forgot about toNativeSeparators again, I'll fix it.
I thought I saw a note on some documentation that this should be UTF-8, but I can't find it anymore. If there are non-latin1 characters in the path, it won't work if I use toLatin1 either, so I'd keep it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
By setting the icon in Desktop.ini of the root folder, this adds the icon both when browsing the folder directly and to the sidebar shortcut. To avoid overwriting any user setting that could exist in Desktop.ini, only do this if the file doesn't exist. Editing .ini files on Windows isn't trivial and isn't worth it given that this file won't exist most of the time. Fixes #2446
793a7f6
to
b5a1270
Compare
I also added b5a1270 to hardcode the |
@jturcotte Could you harmonize Desktop.ini vs desktop.ini to whatever Windows normally does? (btw, does csync_fnmatch watch case?) |
b5a1270
to
37a8f6d
Compare
This prevents it from being removed from the exclude list, which would be an issue since the client itself creates this file in a way that wouldn't match on machines with different installation paths.
37a8f6d
to
84240f2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(btw you need to ping people if you push more, there is no notification)
Thanks, I was waiting for the CI to pass, the previous build didn't pass. |
By setting the icon in Desktop.ini of the root folder, this adds the icon
both when browsing the folder directly and to the sidebar shortcut.
To avoid overwriting any user setting that could exist in Desktop.ini,
only do this if the file doesn't exist. Editing .ini files on Windows
isn't trivial and isn't worth it given that this file won't exist most
of the time.
Fixes #2446