mirror of
https://github.com/penpot/penpot.git
synced 2026-04-30 21:59:10 +00:00
🐛 Preserve Inkscape labels when pasting SVGs (#9252)
Steps to reproduce: paste an SVG authored in Inkscape (or any editor that follows the inkscape:label convention) into a penpot file. The group/element names visible in the source editor are dropped — penpot shows generic auto-ids like 'g1234' or 'path5678' instead. Root cause: parse-svg-element in common/src/app/common/files/shapes_ builder.cljc derived the shape name from (or (:id attrs) (tag->name tag)). Inkscape stores user-given element labels in the inkscape:label and sodipodi:label namespaced attributes while id holds an auto- generated technical id, so the operator's chosen name was always overridden by the technical id when present. tubax/xml->clj (the SVG parser the import pipeline already uses for upload, paste, and library import) keeps namespaced attributes as :prefix:name keywords — the same shape this file already reads :xlink:href from on line 134, and that app.common.svg uses for the xlink: namespace at lines 300-307. Fix: extract the name-resolution logic into a public resolve-element- name helper that prefers :inkscape:label, then :sodipodi:label, then :id, then (tag->name tag). Existing SVGs that don't carry either label namespace fall through the same chain as before, so the behaviour for non-Inkscape-authored SVGs is unchanged. This restores the behaviour dfelinto's penpot-icon-generator-plugin relies on (linked from the issue body) — that plugin reads element names from the imported SVG to map Blender icons to penpot components. Tests: 6 deftest blocks in common/test/common_tests/files/shapes_ builder_test.cljc covering the priority order (inkscape > sodipodi > id > tag), each fallback in isolation, and the empty-attrs case. Registered in common-tests.runner. Closes #7869 Co-authored-by: Andrey Antukh <niwi@niwi.nz>
This commit is contained in:
parent
13414e7bed
commit
d09985edee
@ -677,6 +677,22 @@
|
||||
(remove is-style-fragment?) ;; Filter style fragments and hex colors
|
||||
(filter #(contains? defs %))))) ;; Only existing defs
|
||||
|
||||
(defn resolve-element-name
|
||||
"Pick the most user-meaningful name for an SVG element.
|
||||
|
||||
Inkscape (and editors following the same convention) write the
|
||||
operator-given label to ``inkscape:label``/``sodipodi:label`` while
|
||||
``id`` holds an auto-generated technical id like ``path1234``.
|
||||
Preferring the namespaced label keeps the layer/group/element names
|
||||
the operator sees in their source editor across a paste/import
|
||||
(#7869); the existing ``id`` and ``(tag->name tag)`` fallbacks keep
|
||||
legacy SVGs that don't carry a label working unchanged."
|
||||
[tag attrs]
|
||||
(or (:inkscape:label attrs)
|
||||
(:sodipodi:label attrs)
|
||||
(:id attrs)
|
||||
(tag->name tag)))
|
||||
|
||||
(defn parse-svg-element
|
||||
[frame-id svg-data {:keys [tag attrs hidden] :as element} unames]
|
||||
|
||||
@ -684,7 +700,7 @@
|
||||
;; think we should handle this case early and avoid some code
|
||||
;; execution
|
||||
|
||||
(let [name (or (:id attrs) (tag->name tag))
|
||||
(let [name (resolve-element-name tag attrs)
|
||||
att-refs (csvg/find-attr-references attrs)
|
||||
defs (get svg-data :defs)
|
||||
valid-refs (filter-valid-def-references att-refs defs)
|
||||
|
||||
52
common/test/common_tests/files/shapes_builder_test.cljc
Normal file
52
common/test/common_tests/files/shapes_builder_test.cljc
Normal file
@ -0,0 +1,52 @@
|
||||
;; This Source Code Form is subject to the terms of the Mozilla Public
|
||||
;; License, v. 2.0. If a copy of the MPL was not distributed with this
|
||||
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
|
||||
;;
|
||||
;; Copyright (c) KALEIDOS INC
|
||||
|
||||
(ns common-tests.files.shapes-builder-test
|
||||
(:require
|
||||
[app.common.files.shapes-builder :as sb]
|
||||
[clojure.test :as t]))
|
||||
|
||||
;; Regression for https://github.com/penpot/penpot/issues/7869.
|
||||
;; ``parse-svg-element`` used to derive the shape name from
|
||||
;; ``(or (:id attrs) (tag->name tag))`` which dropped Inkscape-authored
|
||||
;; labels. ``tubax/xml->clj`` (the SVG parser the rest of the import
|
||||
;; pipeline already feeds these maps to) keeps namespaced attributes as
|
||||
;; ``:prefix:name`` keywords — same shape the codebase already reads
|
||||
;; ``:xlink:href`` from in this file (line 134) and in
|
||||
;; ``app.common.svg``.
|
||||
|
||||
(t/deftest resolve-element-name-prefers-inkscape-label
|
||||
(t/is (= "Layer 1"
|
||||
(sb/resolve-element-name :g {:inkscape:label "Layer 1"
|
||||
:id "g1234"}))))
|
||||
|
||||
(t/deftest resolve-element-name-prefers-sodipodi-label-when-no-inkscape-label
|
||||
(t/is (= "phone-icon"
|
||||
(sb/resolve-element-name :path {:sodipodi:label "phone-icon"
|
||||
:id "path5678"}))))
|
||||
|
||||
(t/deftest resolve-element-name-falls-back-to-id-when-no-label-namespace
|
||||
(t/is (= "manual-id"
|
||||
(sb/resolve-element-name :rect {:id "manual-id"}))))
|
||||
|
||||
(t/deftest resolve-element-name-falls-back-to-tag-name-when-no-id-and-no-label
|
||||
;; The tag->name mapping returns generic names for known SVG element
|
||||
;; tags. Asserting on the call result here (rather than a hardcoded
|
||||
;; string) keeps the test stable if the tag->name mapping is updated.
|
||||
(t/is (some? (sb/resolve-element-name :rect {})))
|
||||
(t/is (string? (sb/resolve-element-name :rect {}))))
|
||||
|
||||
(t/deftest resolve-element-name-inkscape-label-wins-over-sodipodi-and-id
|
||||
;; Both label conventions and an id present together; the priority is
|
||||
;; inkscape > sodipodi > id > tag, matching the order operators expect
|
||||
;; (Inkscape's own UI shows ``inkscape:label`` as the canonical name).
|
||||
(t/is (= "user-name"
|
||||
(sb/resolve-element-name :g {:inkscape:label "user-name"
|
||||
:sodipodi:label "stale-label"
|
||||
:id "g1"}))))
|
||||
|
||||
(t/deftest resolve-element-name-empty-attrs-uses-tag-fallback
|
||||
(t/is (some? (sb/resolve-element-name :path {}))))
|
||||
@ -15,6 +15,7 @@
|
||||
[common-tests.files-builder-test]
|
||||
[common-tests.files-changes-test]
|
||||
[common-tests.files-migrations-test]
|
||||
[common-tests.files.shapes-builder-test]
|
||||
[common-tests.geom-align-test]
|
||||
[common-tests.geom-bounds-map-test]
|
||||
[common-tests.geom-flex-layout-test]
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user