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

Change wrong path conversions from string to u8string #5641

Open
Febbe opened this issue Apr 26, 2024 · 1 comment
Open

Change wrong path conversions from string to u8string #5641

Febbe opened this issue Apr 26, 2024 · 1 comment

Comments

@Febbe
Copy link
Collaborator

Febbe commented Apr 26, 2024

There are possible uses of fs::path::string() which may result in a bug.
Changing those would be desired.
Please apply the patch to the release branch release1.2.

One question: There are other instances of fs::path::string() in the code (see a list below). Should some of those also be changed? Some are for error messages, so they should not be converted to utf8 (I think), but some are really path manipulations, and should probably be converted as well.
List of fs::path::string() occurrences

src/util/PathUtil.cpp:
79: dangerous to use, but only one usage where a system encoded path is required ⇒ no bug here.
85: function returns a boolean and works over ASCII only characters. It would only be a problem if windows still uses code pages, which do not extend ASCII.
90: same as 85.
95: dangerous, no known fix, since it's ambiguous, whether const std::string& ext is UTF-8 or ANSI/System encoded. Safe when used with ASCII characters only.
297: g_warning does not know anything about encodings, it's forwarded to the console, which has most likely system encoding ⇒ string is the better choice here.

src/core/control/jobs/AutosaveJob.cpp:45 : the same of g_warning applies for g_message

src/core/control/latex/LatexGenerator.cpp:53 : The variable name already states, that the path must be encoded as OS encoded string ⇒ string is correct here, u8string would be a bug.

src/core/control/settings/MetadataManager.cpp
30: string is better choice
36: same
95: result is used as parameter to strtoll, I bet it only supports ASCII. Chinese numbers will never be accepted, whether it is utf8 or ASCII/ANSY/any other encoding.

src/core/control/settings/Settings.cpp: All are used in a g_xxxxx log function, so string is the better choice.

src/core/control/tools/ImageHandler.cpp:69 : Not part of 1.2.x, but u8string would be better here. Has only visual effects

src/core/control/xojfile/SaveHandler.cpp: Can't change those without breaking old files, u8string is better here!. But it has no negative impact on computers with the same system encoding

src/core/control/xojfile/LoadHandler.cpp:
All uses of error(...) are prone to bugs and may not work as expected.
This should be refactored. Can't tell, whether error is used only in g_xxx log functions or if it is also used in an error message.
No negative impact for the correctness of xournalpp only look and feel.
Changing string to u8string for the rest will also break old files.

src/core/control/CrashHandler.cpp: Safe
src/core/control/LatexController.cpp: Safe

src/core/control/XournalMain.cpp:
124: Safe
125: Safe
167: Safe, since ASCII only characters are used
191: u8string should be used for better look and feel, no breaking bug
446: u8string should be used for better look and feel, no breaking bug
504: Safe

src/core/control/Control.cpp:
238: u8string should be used for better look and feel, no breaking bug
1424: Safe
1608: u8string is the better choice for LAF
1609 neither string nor u8string are good here, string is required for correctness, u8string for LAF, best would be to keep filename as path.
1616: ???
1617: u8string LAF
1620: u8string LAF
1621: Same as 1609
1639: u8string LAF

src/core/gui/GladeSearchpath.cpp:37: Safe

src/core/plugin/luapi_application.h: I think lua works on the system path, but @rolandlo is the better one to ask.
src/core/plugin/Plugin.cpp:
src/core/plugin/PluginController.cpp: I think lua works on the system path, but @rolandlo is the better one to ask.

src/exe/osx/setup-env.cpp:
Completely secure, since osx and unix work on utf-8 anyway, there is no difference between u8string and string.

Originally posted by @Febbe in #5638 (comment)

@Febbe
Copy link
Collaborator Author

Febbe commented Apr 26, 2024

@xournalpp/core I created a new issue, since this is out of scope for #5638.
I'll mark this as first good issue.

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

No branches or pull requests

1 participant