From 754385c7d5c570a3db5cdc628c709ca2d19398b7 Mon Sep 17 00:00:00 2001 From: Max Gaukler Date: Thu, 26 Dec 2024 18:51:00 +0100 Subject: [PATCH] Improve code comments on file path encodings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Try to clarify code comments where the string encoding of paths is unclear. I changed mostly comments, except for a few places where the code was obviously wrong and the change could be made without changing other functions. I don't know how to test the correctness of this change. On Windows and Linux, Inkscape uses UTF8 as native file path encoding (and I don't know about MacOS). On Linux, one can set `G_FILENAME_ENCODING=ISO-8859-1` but this will crash even on basic operations like saving to a file named `รค.svg`. I still hope the change makes it easier to migrate to Gio::File one day. --- src/extension/implementation/implementation.h | 22 +++++++++++++++---- src/extension/implementation/script.cpp | 8 +++++++ src/extension/internal/svg.cpp | 5 ++++- src/helper/png-write.cpp | 9 +++++++- src/io/README | 11 ++++++++-- src/io/file-export-cmd.cpp | 3 +++ src/io/fix-broken-links.cpp | 4 ++++ src/io/resource.cpp | 3 +++ src/io/sys.cpp | 7 ++++++ src/ui/clipboard.cpp | 10 ++++++++- src/ui/dialog/print.cpp | 3 ++- src/ui/interface.cpp | 11 ++++++++-- src/xml/repr-io.cpp | 2 +- 13 files changed, 85 insertions(+), 13 deletions(-) diff --git a/src/extension/implementation/implementation.h b/src/extension/implementation/implementation.h index e58e72e75a..4ac52f50ff 100644 --- a/src/extension/implementation/implementation.h +++ b/src/extension/implementation/implementation.h @@ -107,16 +107,30 @@ public: virtual bool match_template_size(Inkscape::Extension::Template *tmod, double width, double height){ return false; } // ----- Input functions ----- + /** + * Open a file. + * @param filename File path. Value is in platform-native encoding (see Glib::filename_to_utf8). + */ virtual std::unique_ptr open(Inkscape::Extension::Input *module, char const *filename, bool is_importing); // ----- Output functions ----- /** Find out information about the file. */ virtual void save(Inkscape::Extension::Output * /*module*/, SPDocument * /*doc*/, gchar const * /*filename*/) {} + /** + * Convert from PNG to raster format. + * + * The function takes a PNG file and converts it into the specific format. + * + * @param png_file Path to input file in PNG format. + * Value is in platform-native encoding (see Glib::filename_to_utf8). + * @param filename Path to output file. + * Value is in platform-native encoding (see Glib::filename_to_utf8). + */ virtual void export_raster( - Inkscape::Extension::Output * /*module*/, - const SPDocument * /*doc*/, - std::string const &/*png_file*/, - gchar const * /*filename*/) {} + Inkscape::Extension::Output * module, + const SPDocument * doc, + std::string const &png_file, + gchar const * filename) {} // ----- Effect functions ----- /** Find out information about the file. */ diff --git a/src/extension/implementation/script.cpp b/src/extension/implementation/script.cpp index e51d2e7e8c..828121a809 100644 --- a/src/extension/implementation/script.cpp +++ b/src/extension/implementation/script.cpp @@ -846,10 +846,18 @@ bool Script::file_listener::read(Glib::IOCondition condition) { return true; } +/** + * @param name File path. + * Value is in UTF8 encoding. + */ bool Script::file_listener::toFile(const Glib::ustring &name) { return toFile(Glib::filename_from_utf8(name)); } +/** + * @param name File path. + * Value is in platform-native encoding (see Glib::filename_to_utf8). + */ bool Script::file_listener::toFile(const std::string &name) { try { Glib::RefPtr stdout_file = Glib::IOChannel::create_from_file(name, "w"); diff --git a/src/extension/internal/svg.cpp b/src/extension/internal/svg.cpp index 5f0d57da4b..0437365d7c 100644 --- a/src/extension/internal/svg.cpp +++ b/src/extension/internal/svg.cpp @@ -125,7 +125,10 @@ Svg::init() \brief This function takes in a filename of a SVG document and turns it into a SPDocument. \param mod Module to use - \param uri The path or URI to the file (UTF-8) + \param uri The path or URI to the file. + FIXME: Path in UTF8 or in platform-native encoding? + It seems like the variable is used as both. + Please change to Gio::File here and in the called functions to avoid the confusion. This function is really simple, it just calls sp_document_new... That's BS, it does all kinds of things for importing documents diff --git a/src/helper/png-write.cpp b/src/helper/png-write.cpp index 2c1a3f30ea..cc1a2ed4c4 100644 --- a/src/helper/png-write.cpp +++ b/src/helper/png-write.cpp @@ -35,6 +35,7 @@ #include "object/sp-root.h" #include "ui/interface.h" +#include /* This is an example of how to use libpng to read and write PNG files. * The file libpng.txt is much more verbose then this. If you have not @@ -120,6 +121,11 @@ void PngTextList::add(gchar const* key, gchar const* text) } } +/** + * Write to PNG. + * + * @arg filename Filename and path. Value is in UTF8 encoding. + */ static bool sp_png_write_rgba_striped(SPDocument *doc, gchar const *filename, unsigned long int width, unsigned long int height, double xdpi, double ydpi, @@ -384,6 +390,7 @@ ExportResult sp_export_png_file(SPDocument *doc, gchar const *filename, * Export an area to a PNG file * * @param area Area in document coordinates + * @param filename Filename and path. Value is UTF8 encoded. */ ExportResult sp_export_png_file(SPDocument *doc, gchar const *filename, Geom::Rect const &area, @@ -399,7 +406,7 @@ ExportResult sp_export_png_file(SPDocument *doc, gchar const *filename, g_return_val_if_fail(height >= 1, EXPORT_ERROR); g_return_val_if_fail(!area.hasZeroArea(), EXPORT_ERROR); - if (!force_overwrite && !sp_ui_overwrite_file(filename)) { + if (!force_overwrite && !sp_ui_overwrite_file(Glib::filename_from_utf8(filename))) { // aborted overwrite return EXPORT_ABORTED; } diff --git a/src/io/README b/src/io/README index c9f9fa30b0..736900c53d 100644 --- a/src/io/README +++ b/src/io/README @@ -11,13 +11,20 @@ types in a utf8 filename into a file dialog. This needs to be converted to a std::string in the locale encoding before using it to open a file. Glib provides functions to handle translating the utf8 encoded strings to/from the locally used encoding (e.g. -Glib::filename_from_utf8()). +Glib::filename_from_utf8()). In the Glib documentation, the local +encoding is often called "platform-native". Gtk4 file dialogs do not use filenames directly. Instead they use Gio::File's. Gio::File's handle string type conversions for you. You can directly obtain the correct string for system use (get_path()) or for GUI use (get_parse_name()). +Currently Inkscape contains some old code parts where it is not clear +which encoding a file path has, or where the encoding is not converted +correctly. Rewriting to use Gio::File would avoid these issues. +Note that Gio::File only supports absolute paths, so it can not +represent a relative path or a part (fragment) of a path. + SVG files generally contain utf8 encoded strings. So constructing a filename for opening a file from an 'id' would require converting the 'id' to the local encoding. @@ -43,7 +50,7 @@ To do: 1. Move all file related code here (other than extensions). 2. Move extension input/output code into subdirectories within src/extensions/internal and share/extensions. 3. Separate out creating a document and creating a document window. The former belongs here, the later in src/ui. -4. Use std::string for all file names and use glibmm file utilities. +4. Use std::string for all file names and use glibmm file utilities. For absolute paths, it is even better to use Gio::File objects. 5. Use Glib::ustring for URI's and use Inkscape's URI utilities (if not available in glibmm). 6. Rewrite file export code to share a common base and to allow easy export of objects. Should be Gio::Action based. Things like cropping, selecting an object, hiding other objects, etc. should be done before the document is passed diff --git a/src/io/file-export-cmd.cpp b/src/io/file-export-cmd.cpp index 3a8c92b5f9..04e6928d2d 100644 --- a/src/io/file-export-cmd.cpp +++ b/src/io/file-export-cmd.cpp @@ -672,6 +672,9 @@ InkFileExportCmd::do_export_png(SPDocument *doc, std::string const &export_filen return 0; } +/** + * @param filename_out Filename and path. Value is UTF8 encoded. + */ void InkFileExportCmd::do_export_png_now(SPDocument *doc, std::string const &filename_out, Geom::Rect area, double dpi_in, const std::vector &items) { diff --git a/src/io/fix-broken-links.cpp b/src/io/fix-broken-links.cpp index 6aba9b062b..bcd96b7abe 100644 --- a/src/io/fix-broken-links.cpp +++ b/src/io/fix-broken-links.cpp @@ -59,6 +59,10 @@ std::vector splitPath( std::string const &path ) * @arg path - The absolute path to convert * @arg base - The base or reference path to be relative to * @arg parents - The number of parents or .. segments to allow + * + * All input strings must have the same encoding, + * either UTF8 or platform-native encoding (see Glib::filename_to_utf8). + * The return value has the same encoding as the input. */ std::string optimizePath(std::string const &path, std::string const &base, unsigned int parents) { diff --git a/src/io/resource.cpp b/src/io/resource.cpp index 5210948c69..1241b88ac7 100644 --- a/src/io/resource.cpp +++ b/src/io/resource.cpp @@ -41,6 +41,9 @@ namespace Inkscape::IO::Resource { #define INKSCAPE_PROFILE_DIR "inkscape" +/** + * @returns Path. Value is in platform-native encoding (see Glib::filename_to_utf8). + */ gchar *_get_path(Domain domain, Type type, char const *filename, char const *extra=nullptr) { if (domain == USER || domain == SHARED) { diff --git a/src/io/sys.cpp b/src/io/sys.cpp index 2af3cb747e..55d5a2de76 100644 --- a/src/io/sys.cpp +++ b/src/io/sys.cpp @@ -62,6 +62,13 @@ void Inkscape::IO::dump_fopen_call( char const *utf8name, char const *id ) #endif } +/** + * Open a file with g_fopen(). + * + * \param utf8name Filename in UTF8 encoding + * \param mode see g_fopen() + * \returns see g_fopen() + */ FILE *Inkscape::IO::fopen_utf8name( char const *utf8name, char const *mode ) { FILE* fp = nullptr; diff --git a/src/ui/clipboard.cpp b/src/ui/clipboard.cpp index a2d0ab0c45..cfb51c5035 100644 --- a/src/ui/clipboard.cpp +++ b/src/ui/clipboard.cpp @@ -99,6 +99,7 @@ #undef NOGDI #include #endif +#include using namespace Inkscape::Util; @@ -178,6 +179,13 @@ void pump_until(F const &f) } // Fixme: Get rid of temporary files hack. +/** Get a temporary file name. + * + * @arg suffix file suffix. May only contain ASCII characters. + * + * @returns Filename with absolute path. + * Value is in platform-native encoding (see Glib::filename_to_utf8). + */ std::string get_tmp_filename(char const *suffix) { return Glib::build_filename(Glib::get_user_cache_dir(), suffix); @@ -1793,7 +1801,7 @@ void ClipboardManagerImpl::_onGet(char const *mime_type, Glib::RefPtr(area.height() + 0.5); // read from namedview - auto const raster_file = get_tmp_filename("inkscape-clipboard-export-raster"); + auto const raster_file = Glib::filename_to_utf8(get_tmp_filename("inkscape-clipboard-export-raster")); sp_export_png_file(_clipboardSPDoc.get(), raster_file.c_str(), area, width, height, dpi, dpi, bgcolor, nullptr, nullptr, true, {}); (*out)->export_raster(_clipboardSPDoc.get(), raster_file.c_str(), filename.c_str(), true); unlink(raster_file.c_str()); diff --git a/src/ui/dialog/print.cpp b/src/ui/dialog/print.cpp index d8322c649f..eb8b7bbeb8 100644 --- a/src/ui/dialog/print.cpp +++ b/src/ui/dialog/print.cpp @@ -172,6 +172,7 @@ void Print::draw_page(const Glib::RefPtr& context, int page_n rect = page->getDesktopRect(); } + /// Path to temporary PNG file. Value is in platform-native encoding (see Glib::filename_to_utf8). std::string tmp_png; std::string tmp_base = "inkscape-print-png-XXXXXX"; @@ -192,7 +193,7 @@ void Print::draw_page(const Glib::RefPtr& context, int page_n bgcolor |= SP_COLOR_F_TO_U(opacity); } - sp_export_png_file(_workaround._doc, tmp_png.c_str(), rect, + sp_export_png_file(_workaround._doc, Glib::filename_to_utf8(tmp_png).c_str(), rect, (unsigned long)(Inkscape::Util::Quantity::convert(rect.width(), "px", "in") * dpi), (unsigned long)(Inkscape::Util::Quantity::convert(rect.height(), "px", "in") * dpi), dpi, dpi, bgcolor, nullptr, nullptr, true, {}); diff --git a/src/ui/interface.cpp b/src/ui/interface.cpp index 32d938179f..380051db21 100644 --- a/src/ui/interface.cpp +++ b/src/ui/interface.cpp @@ -63,10 +63,17 @@ void sp_ui_error_dialog(char const *message) Inkscape::UI::dialog_run(dlg); } +/** + * If necessary, ask the user if a file may be overwritten. + * + * @arg filename path to file. + * Value is in platform-native encoding (see Glib::filename_to_utf8). + * @returns true if it is okay to write to the file. + * This means that the file does not exist yet or the user confirmed that overwriting is okay. + */ bool sp_ui_overwrite_file(std::string const &filename) { - // Fixme: Passing a std::string filename to an argument expecting utf8. - if (!Inkscape::IO::file_test(filename.c_str(), G_FILE_TEST_EXISTS)) { + if (!g_file_test(filename.c_str(), G_FILE_TEST_EXISTS)) { return true; } diff --git a/src/xml/repr-io.cpp b/src/xml/repr-io.cpp index 42077171ce..86394af273 100644 --- a/src/xml/repr-io.cpp +++ b/src/xml/repr-io.cpp @@ -265,7 +265,7 @@ int XmlSource::close() * The default namespace can also be specified, if desired. * XIncude is dangerous to support during use-cases like automated file format conversion, so it is off by default. * - * \param filename The actual file to read from. + * \param filename The actual file to read from. UTF8 encoded. * * \param default_ns Default namespace for the document, can be nullptr. * -- GitLab