Skip to content

Remove the onimport preference from extensions

Martin Owens requested to merge doctormo/inkscape:fix-4839-remove-onimport into 1.4.x

During importing of data, we were setting a temporary preference called /options/onimport, this is very dangerous as becomes apparent in issue 4839 where simply trying to open an svg icon would crash if the temporary pref was set permanently by accident.

This is a comprehensive fix which removes the temp preference entirely.

  1. Remove use from Drag and Drop XML data, it was copypasta and unused.
  2. Move onimport setters from drag and drop uri, command palette and window action and into the file_import function where it's always true anyway.
  3. Add a new is_importing boolean to the extension implementation interface which handles the state for svg and points future developers towards fixing import mechanisms for other extensions when those issues appear. This bool replaces onimport and now can't be true when just opening a file.
  4. Move the "open as new document" out of Svg::open and back into file_import where it is way more appropriate than deep in the extension stack.

Fixes #4839 Fixes #3835

Detailed Research Notes

Because this change could scare release maintainers, I'm detailing the process of carefully researching each of the used code paths and why I believe the change is appropriate for 1.4.x

There are 4 main codepaths which change /options/onimport:

  1. Drag and drop SVG XML data
    • sp_repr_read_mem No connection to onimport, suspected copypasta
  2. Drag and drop SVG URI
    • sp_ui_import_files / ui/interface.cpp
    • file_import(doc, filename, nullptr)
  3. Command Palette operate_recent_file
    • file_import(doc, uri, nullptr)
  4. document_import in actions-file-window.cpp
    • sp_file_import(window) / file.cpp
    • file_import(doc, filename, extension)

These are calling three code paths which make use of /options/onimport:

  1. file_import in file.cpp
    • Remembers if onimport was set
    • Calls 2. extensions system open command
    • If onimport was flipped from true to false, uses it as a signal which means "it was opened as a new document"
  2. Extension system.cpp open(...)
    • Uses onimport to enable the gui selectively for svg and pixbuf extensions only
    • Then this calls the import extension's open implementation
  3. SVG Internal Extension, Svg::open(...)
    • Uses onimport to decide how the svg will be constructed (image tag href, bunch of svg, pages, etc)
    • Sets onimport if the "open as a new document" is set.

Using the above research the final changes were decided to be the minimal amount needed to fix the issues properly. It does not refactor any other extension type, as that needs to be something for the future in master.

Edited by Martin Owens

Merge request reports