diff --git a/.serena/memories/workflow/updating-node.md b/.serena/memories/workflow/updating-node.md index 4449bb5655..a1359bb93f 100644 --- a/.serena/memories/workflow/updating-node.md +++ b/.serena/memories/workflow/updating-node.md @@ -17,6 +17,7 @@ No single source of truth: nothing reads `.nvmrc` at build time, and the version - DHI tags are exact-version and lag the upstream release, and dhi.io answers 401 to anonymous requests, so a tag that does not exist yet can only be caught by a real build. `build-docker.yml` and `build-docker-devenv.yml` log into dhi.io, so the failure surfaces in CI, not locally. - Node 24.x is the Krypton LTS line: take the newest patch of that major unless a major bump is the point. - Renovate also opens these bumps, on `renovate-*` branches and with its config living outside this repo; a manual bump and Renovate's must not diverge on any of the six sites. +- A backport merge can deliver a newer Node into `docker/` and the plugins-deploy workflows while main's `.nvmrc` stays behind, because main receives partial backports of `.github/` and `docker/` from develop. Merging main into staging therefore conflicts on the pin lines: take main's set and finish the bump (`.nvmrc` plus the `packageManager` fields via `mem:workflow/updating-pnpm`), or the six sites drift. - Both workflows must stay valid YAML. The `python3` in the devenv has no `yaml` module, so validate with `uv run --with pyyaml python3 -c ...` instead of a bare import. ## Verification diff --git a/.serena/memories/workflow/updating-pnpm.md b/.serena/memories/workflow/updating-pnpm.md index 3baf21b358..c8455ff680 100644 --- a/.serena/memories/workflow/updating-pnpm.md +++ b/.serena/memories/workflow/updating-pnpm.md @@ -88,13 +88,14 @@ file (never pipe tool output through filters). so a nested one silently forks install and lockfile behavior. Do not reintroduce it. - `media-processor/target/package.json` is a generated, gitignored copy that - the script also stamps, so stamping and `--check` report 36 files while - only 35 are tracked. A stale copy there makes `--check` fail with nothing + the script also stamps, so stamping and `--check` report 35 files while + only 34 are tracked. A stale copy there makes `--check` fail with nothing wrong in the repo: delete `media-processor/target` and re-run. - Expect metadata-only lockfile diffs when only the pnpm version moves: the pnpm self-reference entries, plus a new `packageManagerDependencies` section in lockfiles last written by older pnpm. Large diffs mean re-resolution; inspect them before accepting. +- A backport merge of main into staging can bring a newer `PNPM_VERSION` in `docker/` while every `packageManager` field stays behind, since main receives partial `.github/`+`docker/` backports from develop. Resolve those conflicts with main's version and run `scripts/sync-pnpm-version ` in the same merge. ## Verification diff --git a/backend/resources/app/templates/debug.tmpl b/backend/resources/app/templates/debug.tmpl index 64ddd9d150..1498edaf87 100644 --- a/backend/resources/app/templates/debug.tmpl +++ b/backend/resources/app/templates/debug.tmpl @@ -326,6 +326,7 @@ Debug Main Page PROGRESS CMD BACKEND + REPLICA NAME CREATED ENDED @@ -339,12 +340,13 @@ Debug Main Page {{job.done}} / {{job.total}} {{job.cmd}} {{job.backend}} + {{job.replica}} {{job.name}} {{job.created-at}} {{job.ended-at}} {% empty %} - No export jobs. + No export jobs. {% endfor %} diff --git a/backend/src/app/rpc/commands/files.clj b/backend/src/app/rpc/commands/files.clj index 288e3fb190..d538563ad6 100644 --- a/backend/src/app/rpc/commands/files.clj +++ b/backend/src/app/rpc/commands/files.clj @@ -608,11 +608,21 @@ (let [process-row (fn [{:keys [id library-file-ids]}] - (let [file (get-file-with-summary cfg id)] - (assoc file :library-file-ids (db/decode-pgarray library-file-ids #{})))) + ;; One shared file that fails to load (for example, a migration + ;; error on old data) must not break the whole list; skip it + ;; and report it with its id. + (try + (let [file (get-file-with-summary cfg id)] + (assoc file :library-file-ids (db/decode-pgarray library-file-ids #{}))) + (catch Exception cause + (l/error :hint "unable to load shared file" + :file-id (str id) + :team-id (str team-id) + :cause cause) + nil))) xform - (map process-row)] + (keep process-row)] (->> (db/plan conn [sql:team-shared-files team-id] {:fetch-size 1}) (transduce xform conj #{})))) diff --git a/backend/test/backend_tests/rpc_file_test.clj b/backend/test/backend_tests/rpc_file_test.clj index a6fc2f0a0b..2d222f7add 100644 --- a/backend/test/backend_tests/rpc_file_test.clj +++ b/backend/test/backend_tests/rpc_file_test.clj @@ -3119,3 +3119,23 @@ (get-in (meta (:result out)) [:app.loggers.audit/props :id]))) (t/is (= (:id team) (get-in (meta (:result out)) [:app.loggers.audit/props :team-id]))))) + +(t/deftest get-team-shared-files-skips-files-that-fail-to-load + (let [profile (th/create-profile* 1) + file1 (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared true}) + file2 (th/create-file* 2 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared true}) + get-file (deref #'files/get-file-with-summary) + broken (fn [cfg id] + (if (= id (:id file1)) + (throw (ex-info "broken file" {:type :assertion})) + (get-file cfg id))) + out (with-redefs-fn {#'files/get-file-with-summary broken} + #(th/command! {::th/type :get-team-shared-files + ::rpc/profile-id (:id profile) + :team-id (:default-team-id profile)}))] + (t/is (th/success? out)) + (t/is (= #{(:id file2)} (into #{} (map :id) (:result out)))))) diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index d0312f5a56..de82f16f9e 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -230,7 +230,10 @@ ;; its geometry from the content as for line-like paths. (let [{:keys [width height] :as rect} (grc/points->rect (:points shape))] (if (or (nil? rect) (mth/almost-zero? width) (mth/almost-zero? height)) - (let [selrect (path.segment/content->selrect (:content shape)) + ;; Content whose points are not numbers gives no rect + ;; either; fall back to the empty rect. + (let [selrect (or (path.segment/content->selrect (:content shape)) + (grc/make-rect)) points (grc/rect->points selrect) transform (gmt/matrix) transform-inv (gmt/matrix)] @@ -1339,6 +1342,16 @@ (let [decode-segments (sm/decoder path/schema:segments sm/json-transformer) + ;; Old files may have segments with missing coordinates, which + ;; the binary path data can not store; drop them. + valid-segment? + (fn [{:keys [command params]}] + (case command + :close-path true + (:move-to :line-to) (and (number? (:x params)) (number? (:y params))) + :curve-to (every? #(number? (get params %)) [:x :y :c1x :c1y :c2x :c2y]) + false)) + update-object (fn [object] (if (or (cfh/bool-shape? object) @@ -1352,9 +1365,10 @@ (path/content []) :else - (-> content - (decode-segments) - (path/content)))] + (->> content + (decode-segments) + (filterv valid-segment?) + (path/content)))] (assoc object :content content)) object)) diff --git a/common/src/app/common/types/file.cljc b/common/src/app/common/types/file.cljc index 3a52dd79ca..df4733c25a 100644 --- a/common/src/app/common/types/file.cljc +++ b/common/src/app/common/types/file.cljc @@ -1259,23 +1259,22 @@ (defn get-ref-chain-until-target-ref "Returns a vector with the shape ref chain until target-ref, including itself" [container libraries shape target-ref] - (loop [chain [shape] - current shape] - (if (= current target-ref) + (loop [chain [shape] + current shape + container container] + (if (= (:id current) target-ref) chain (if-let [ref (find-ref-shape nil container libraries current :with-context? true)] - (recur (conj chain ref) ref) + (recur (conj chain ref) ref (:container (meta ref))) chain)))) (defn get-touched-from-ref-chain-until-target-ref "Returns a set with the :touched of all the items on the shape ref chain until target-ref, including itself" - [container libraries shape target-ref] - (let [chain (get-ref-chain-until-target-ref container libraries shape target-ref) - more-touched (->> chain - (map :touched) - (remove nil?) - (apply set/union) - (remove ctk/swap-slot?) - set)] - (set/union (or (:touched shape) #{}) more-touched))) + [container libraries shape target-ref & {:keys [remove-slots?] :or {remove-slots? true}}] + (let [chain (get-ref-chain-until-target-ref container libraries shape target-ref) + more-touched (cond->> chain + :always (map :touched) + :always (reduce set/union #{}) + remove-slots? (remove ctk/swap-slot?))] + (set/union (or (:touched shape) #{}) (set more-touched)))) \ No newline at end of file diff --git a/common/test/common_tests/files_migrations_test.cljc b/common/test/common_tests/files_migrations_test.cljc index 61ba0e7486..cb27492b6a 100644 --- a/common/test/common_tests/files_migrations_test.cljc +++ b/common/test/common_tests/files_migrations_test.cljc @@ -9,6 +9,7 @@ [app.common.data :as d] [app.common.files.migrations :as cfm] [app.common.files.tokens :as cfo] + [app.common.geom.rect :as grc] [app.common.test-helpers.components :as thk] [app.common.test-helpers.compositions :as tho] [app.common.test-helpers.files :as thf] @@ -16,6 +17,7 @@ [app.common.test-helpers.shapes :as ths] [app.common.test-helpers.variants :as thv] [app.common.types.file :as ctf] + [app.common.types.path :as path] [app.common.types.shape :as cts] [app.common.types.token :as ctt] [app.common.uuid :as uuid] @@ -637,6 +639,62 @@ (t/is (= (:selrect shape) (:selrect shape')) "selrect rebuilt from content"))))) +(def ^:private contents-without-rect + "Path contents from old files whose points give no rect" + {"segments without params" [{:command :move-to} {:command :line-to}] + "only a close path" [{:command :close-path :params {}}] + "not finite coordinates" [{:command :move-to :params {:x ##NaN :y ##Inf}} + {:command :line-to :params {:x 10 :y 10}}]}) + +(t/deftest migration-legacy-6-repairs-paths-whose-content-has-no-rect + (let [migration-id "legacy-6" + page-id (uuid/next) + shape-id (uuid/next)] + + (doseq [[label content] contents-without-rect] + (let [shape (-> (make-path-shape shape-id) + (assoc :points [] :content content)) + data {:pages-index {page-id {:objects {shape-id shape}}}} + data' (cfm/migrate-data data migration-id) + shape' (get-in data' [:pages-index page-id :objects shape-id])] + + (t/is (grc/rect? (:selrect shape')) (str "selrect is a rect: " label)) + (t/is (= 4 (count (:points shape'))) (str "points rebuilt: " label)))))) + +(t/deftest migration-0003-convert-path-content-drops-segments-without-coordinates + (let [migration-id "0003-convert-path-content-v2" + page-id (uuid/next) + shape-id (uuid/next) + content [{:command :move-to :params {:x 10 :y 20}} + {:command :line-to} + {:command :curve-to :params {:x 50 :y 50}} + {:command :line-to :params {:x 110 :y 70}} + {:command :close-path :params {}}] + shape (assoc (make-path-shape shape-id) :content content) + data {:pages-index {page-id {:objects {shape-id shape}}}} + data' (cfm/migrate-data data migration-id) + content' (get-in data' [:pages-index page-id :objects shape-id :content])] + + (t/is (path/content? content')) + (t/is (= [:move-to :line-to :close-path] (mapv :command (vec content')))))) + +(t/deftest migration-legacy-6-chain-with-content-without-rect + ;; The file must load: every migration from legacy-6 on and the final + ;; data validation must succeed. + (doseq [[label content] contents-without-rect] + (let [shape-id (uuid/next) + file (ctf/make-file {:name "Legacy path without rect"}) + page-id (first (get-in file [:data :pages])) + shape (-> (make-path-shape shape-id) + (assoc :points [] :content content)) + file (-> file + (dissoc :migrations) + (assoc :version 5) + (assoc-in [:data :pages-index page-id :objects shape-id] shape)) + file' (cfm/migrate-file file (delay {}))] + + (t/is (contains? (:migrations file') "legacy-6") (str "migrated: " label))))) + (t/deftest migration-legacy-6-runs-through-file-migration ;; A file saved before version 6 runs every migration from legacy-6 on, ;; and the whole chain must succeed for the file to load. diff --git a/docs/technical-guide/configuration.md b/docs/technical-guide/configuration.md index aa4593fa61..91c6ab740d 100644 --- a/docs/technical-guide/configuration.md +++ b/docs/technical-guide/configuration.md @@ -923,6 +923,15 @@ PENPOT_INTERNAL_URI: http://penpot-frontend:8080 `http://penpot-frontend:8080` used in the docker-compose is a good default and it is recommended to keep it unchanged. +**Beta, new render engine (WebGL):** when you run more than one exporter, give +each one its own name and keep it across restarts and upgrades. An exporter +uses it to cancel only its own unfinished exports when it restarts: + +```bash +# Exporter +PENPOT_EXPORTER_REPLICA_ID: exporter-1 +``` + ### MCP The MCP server lets AI agents read and edit Penpot files. It runs as a separate diff --git a/exporter/README.md b/exporter/README.md index df4d4412db..5e3986b90c 100644 --- a/exporter/README.md +++ b/exporter/README.md @@ -88,6 +88,7 @@ section, with a `?job-id=` filter. | `PENPOT_EXPORTER_MAX_CONCURRENT_JOBS` | `4` | Admission limit | | `PENPOT_EXPORTER_MAX_JOBS_PER_PROFILE`| `2` | Per-profile admission limit | | `PENPOT_EXPORTER_QUEUE_MAX` | `64` | Queue cap; over it, `429 :queue-full` | +| `PENPOT_EXPORTER_REPLICA_ID` | hostname | Names this replica; boot cleanup only cancels its own jobs | | `PENPOT_WASM_WORKER_POOL_MAX` | `2` | Headless render worker threads; min 1 | | `PENPOT_WASM_WORKER_POOL_MIN` | `1` | Workers kept warm; clamped to the max | | `PENPOT_WASM_WORKER_IDLE_TIMEOUT` | `300` | Silence before a worker is terminated, in seconds | diff --git a/exporter/src/app/config.cljs b/exporter/src/app/config.cljs index 0fcc5a052d..4f94887ce0 100644 --- a/exporter/src/app/config.cljs +++ b/exporter/src/app/config.cljs @@ -9,6 +9,7 @@ (:require ["node:buffer" :as buffer] ["node:crypto" :as crypto] + ["node:os" :as os] ["node:process" :as process] [app.common.data :as d] [app.common.flags :as flags] @@ -55,6 +56,7 @@ [:exporter-max-jobs-per-profile {:optional true} ::sm/int] [:exporter-queue-max {:optional true} ::sm/int] [:exporter-job-ttl {:optional true} ::sm/int] + [:exporter-replica-id {:optional true} :string] [:wasm-worker-pool-max {:optional true} ::sm/int] [:wasm-worker-pool-min {:optional true} ::sm/int] [:wasm-worker-idle-timeout {:optional true} ::sm/int] @@ -124,6 +126,12 @@ (or (c/get config :internal-uri) (c/get config :public-uri))) +(def replica-id + "Names this exporter among its siblings. It must survive a restart, since + the boot sweep uses it to find the jobs the previous run left behind." + (or (c/get config :exporter-replica-id) + (os/hostname))) + (def management-key (let [key (or (c/get config :exporter-shared-key) (let [secret-key (c/get config :secret-key) diff --git a/exporter/src/app/core.cljs b/exporter/src/app/core.cljs index a1ddab7efa..7dcf3a5b71 100644 --- a/exporter/src/app/core.cljs +++ b/exporter/src/app/core.cljs @@ -33,6 +33,7 @@ (l/info :msg "initializing" :public-uri (str (cf/get :public-uri)) :internal-uri (str (cf/get-internal-uri)) + :replica cf/replica-id :version (:full cf/version)) (l/info :msg "headless wasm export available" :wasm-dir wasm/artifact-dir @@ -86,8 +87,19 @@ (fn [cause] (js/console.error cause))) +(defonce ^:private exiting? (atom false)) + +(defn- exit! + "Cancels this process's jobs before exiting, so their records and clients + do not keep waiting on work that dies with it. A second signal exits at once." + [] + (if (compare-and-set! exiting? false true) + (->> (shutdown-step "cancel-jobs" jobs/cancel-all!) + (p/fnly (fn [_ _] (proc/exit 0)))) + (proc/exit 0))) + ;; Signals are only delivered to the main thread, and `exit` in a worker would ;; take down that worker rather than the process. (when ^boolean wt/isMainThread - (.on proc/default "SIGTERM" (fn [] (proc/exit 0))) - (.on proc/default "SIGINT" (fn [] (proc/exit 0)))) + (.on proc/default "SIGTERM" exit!) + (.on proc/default "SIGINT" exit!)) diff --git a/exporter/src/app/jobs.cljs b/exporter/src/app/jobs.cljs index e3497b2861..d555269a52 100644 --- a/exporter/src/app/jobs.cljs +++ b/exporter/src/app/jobs.cljs @@ -20,6 +20,7 @@ [app.common.logging :as l] [app.common.time :as ct] [app.common.uuid :as uuid] + [app.config :as cf] [app.jobs.store :as store] [app.redis :as redis] [promesa.core :as p])) @@ -81,6 +82,7 @@ [{:keys [profile-id cmd backend total name resource-id]} run-fn] (let [job {:id (uuid/next) :profile-id profile-id + :replica cf/replica-id :cmd cmd :backend backend :state "queued" @@ -165,7 +167,10 @@ overwrite either." [job data] (if-let [job (live job)] - (persist-and-publish! (merge job data)) + (let [job (merge job data)] + (l/info :hint "export job" :job-id (str (:id job)) :state (:state job) + :replica (:replica job)) + (persist-and-publish! job)) (p/resolved job))) (defn start! @@ -208,7 +213,7 @@ :error (ex-message cause)})) (defn- cancel-local! - [job-id] + [job-id & {:as data}] (let [k (str job-id) rt (get @registry k) job (:job rt)] @@ -222,7 +227,7 @@ ;; (`scheduler/drop-queued!`) releases the job, and `transition!` on a ;; released job is a no-op, so a queued job would keep claiming to be ;; queued in the store and never publish its `cancelled` update. - (let [result (transition! job {:state "cancelled" :ended-at (now-ms)})] + (let [result (transition! job (merge {:state "cancelled" :ended-at (now-ms)} data))] (doseq [f (:cancel-fns rt)] (try (f) @@ -245,22 +250,40 @@ (store/request-cancel! (:id job)) job))))))) +(defn cancel-all! + "Cancels every job this process holds, on shutdown. Nothing in flight survives + the process, and clients learn it now rather than when this replica boots + again, if it ever does under the same id." + [] + (let [job-ids (keys @registry)] + (when (seq job-ids) + (l/info :hint "cancelling jobs on shutdown" :count (count job-ids))) + (->> job-ids + (map #(cancel-local! % :interrupted true)) + (p/all)))) + +(defn- abandoned? + "A job this replica's previous run left mid-flight. A sibling's jobs are its + own business: they may well be running." + [job] + (and (= cf/replica-id (:replica job)) + (not (terminal? job)))) + (defn- clean-abandoned! - "Marks every job left mid-flight by a previous process as cancelled. + "Marks every job left mid-flight by a previous run of this replica as + cancelled. A queue and its running jobs live in the memory of the process that owns them, so nothing in flight when it died can be resumed; without this the - record would keep claiming to be running until its TTL expires. - - NOTE: the store cannot tell whose jobs are whose, so with more than one - exporter behind a load balancer this would also cancel a sibling's running - jobs. Single-instance deployments only." + record would keep claiming to be running until its TTL expires. A replica + that never comes back under the same id leaves its records to the TTL." [] (->> (store/fetch-all) (p/mcat (fn [jobs] - (let [abandoned (remove terminal? jobs)] + (let [abandoned (filter abandoned? jobs)] (when (seq abandoned) (l/warn :hint "cancelling jobs abandoned by a previous process" + :replica cf/replica-id :count (count abandoned))) (->> abandoned (map (fn [job] diff --git a/exporter/src/app/jobs/scheduler.cljs b/exporter/src/app/jobs/scheduler.cljs index 320689d90d..a986019a57 100644 --- a/exporter/src/app/jobs/scheduler.cljs +++ b/exporter/src/app/jobs/scheduler.cljs @@ -8,6 +8,7 @@ "Admission control for export jobs. Limits concurrent jobs and rejects work rather than allowing an unbounded backlog. + Every limit is per replica: the queue lives in this process only. Queue order is FIFO, except jobs whose profile is already at its cap are skipped, as are headless jobs once every render worker is busy." (:require diff --git a/exporter/test/exporter_tests/jobs_test.cljs b/exporter/test/exporter_tests/jobs_test.cljs index c8baa9b9fd..a92c1c1790 100644 --- a/exporter/test/exporter_tests/jobs_test.cljs +++ b/exporter/test/exporter_tests/jobs_test.cljs @@ -9,6 +9,7 @@ reported and swallowed, so only the in-process record is exercised." (:require [app.common.uuid :as uuid] + [app.config :as cf] [app.jobs :as jobs] [cljs.test :as t :include-macros true] [promesa.core :as p])) @@ -84,3 +85,23 @@ (t/is (= "ended" (:state ended))) (t/is (nil? (jobs/lookup (:id job)))) (done))))) + +(t/deftest boot-sweep-leaves-sibling-jobs-alone + (let [abandoned? #'jobs/abandoned?] + (t/is (abandoned? {:replica cf/replica-id :state "running"})) + (t/is (abandoned? {:replica cf/replica-id :state "queued"})) + (t/is (not (abandoned? {:replica cf/replica-id :state "ended"}))) + (t/is (not (abandoned? {:replica (str cf/replica-id "-sibling") :state "running"}))))) + +(t/deftest shutdown-cancels-all-its-jobs + (t/async done + (p/let [running (create!) + queued (create!) + _ (jobs/start! running) + _ (jobs/cancel-all!)] + (doseq [job [running queued]] + (let [current (jobs/lookup (:id job))] + (t/is (= "cancelled" (:state current))) + (t/is (true? (:interrupted current)))) + (jobs/release! (:id job))) + (done)))) diff --git a/frontend/src/app/main/data/shortcuts.cljs b/frontend/src/app/main/data/shortcuts.cljs index 7181f5ebb3..9a045d4995 100644 --- a/frontend/src/app/main/data/shortcuts.cljs +++ b/frontend/src/app/main/data/shortcuts.cljs @@ -187,8 +187,11 @@ shortcuts (reduce-kv (fn [acc sc-key new-command] + ;; A non-customizable shortcut ignores overrides stored before it + ;; became so: the settings page no longer offers to reset them. (if (and (contains? acc sc-key) - (not (:disabled (get acc sc-key)))) + (not (:disabled (get acc sc-key))) + (not (false? (:customizable (get acc sc-key))))) (-> acc (assoc-in [sc-key :original-command] (get-in acc [sc-key :command])) (assoc-in [sc-key :command] new-command) diff --git a/frontend/src/app/main/data/workspace/clipboard.cljs b/frontend/src/app/main/data/workspace/clipboard.cljs index 6b560ccd6f..37f7ef80b4 100644 --- a/frontend/src/app/main/data/workspace/clipboard.cljs +++ b/frontend/src/app/main/data/workspace/clipboard.cljs @@ -69,6 +69,43 @@ [potok.v2.core :as ptk] [promesa.core :as p])) +(defn- advance-shape + [file libraries page level-delta head-id objects shape] + (let [new-shape-ref (ctf/advance-shape-ref file page libraries shape level-delta {:include-deleted? true}) + container (ctn/make-container page :page) + ;; The head becomes a root copy, which cannot carry a swap slot, so + ;; strip inherited slots only for it. Nested sub-heads keep them so + ;; they still pair with the children of the new near main. + head? (= (:id shape) head-id) + new-touched (ctf/get-touched-from-ref-chain-until-target-ref + container libraries shape new-shape-ref :remove-slots? head?)] + (cond-> objects + (and (some? new-shape-ref) (not= new-shape-ref (:shape-ref shape))) + (update (:id shape) assoc :shape-ref new-shape-ref :touched new-touched)))) + +(defn- advance-copy + [file libraries page objects shape] + (if (and (ctk/instance-head? shape) (not (ctk/main-instance? shape))) + (let [level-delta (if (nil? (ctk/get-swap-slot shape)) + (ctn/get-nesting-level-delta (:objects page) shape uuid/zero) + 0)] + (if (pos? level-delta) + (reduce (partial advance-shape file libraries page level-delta (:id shape)) + objects + (cfh/get-children-with-self objects (:id shape))) + objects)) + objects)) + +(defn advance-copies + "When copying an instance that is nested inside another one, we need to + advance the shape refs to one or more levels of remote mains." + [file libraries page selected data] + (let [heads (mapcat #(ctn/get-child-heads (:objects data) %) selected)] + (update data :objects + #(reduce (partial advance-copy file libraries page) + % + heads)))) + (defn copy-selected [] (letfn [(sort-selected [state data] @@ -150,39 +187,13 @@ (let [frame (get objects parent-frame-id)] (gsh/translate-to-frame shape frame)))) - ;; When copying an instance that is nested inside another one, we need to - ;; advance the shape refs to one or more levels of remote mains. - (advance-copies [state selected data] - (let [file (dsh/lookup-file state) - libraries (:files state) - ;; FIXME - page (dsh/lookup-page state) - heads (mapcat #(ctn/get-child-heads (:objects data) %) selected)] - (update data :objects - #(reduce (partial advance-copy file libraries page) - % - heads)))) - - (advance-copy [file libraries page objects shape] - (if (and (ctk/instance-head? shape) (not (ctk/main-instance? shape))) - (let [level-delta (if (nil? (ctk/get-swap-slot shape)) - (ctn/get-nesting-level-delta (:objects page) shape uuid/zero) - 0)] - (if (pos? level-delta) - (reduce (partial advance-shape file libraries page level-delta) - objects - (cfh/get-children-with-self objects (:id shape))) - objects)) - objects)) - - (advance-shape [file libraries page level-delta objects shape] - (let [new-shape-ref (ctf/advance-shape-ref file page libraries shape level-delta {:include-deleted? true}) - container (ctn/make-container page :page) - new-touched (ctf/get-touched-from-ref-chain-until-target-ref container libraries shape new-shape-ref)] - (cond-> objects - (and (some? new-shape-ref) (not= new-shape-ref (:shape-ref shape))) - (-> (assoc-in [(:id shape) :shape-ref] new-shape-ref) - (assoc-in [(:id shape) :touched] new-touched))))) + (advance-copies-from-state [state selected data] + (advance-copies (dsh/lookup-file state) + (:files state) + ;; FIXME + (dsh/lookup-page state) + selected + data)) (on-copy-error [error] (js/console.error "clipboard blocked:" error) @@ -239,7 +250,7 @@ (rx/merge-map (partial prepare-object objects frame-id)) (rx/reduce (partial collect-data state) initial) (rx/map (partial sort-selected state)) - (rx/map (partial advance-copies state selected)) + (rx/map (partial advance-copies-from-state state selected)) (rx/map #(t/encode-str % {:type :json-verbose})) (rx/map #(wapi/create-blob % "text/plain")) (rx/subs! resolve reject))))] @@ -254,7 +265,7 @@ (rx/merge-map (partial prepare-object objects frame-id)) (rx/reduce (partial collect-data state) initial) (rx/map (partial sort-selected state)) - (rx/map (partial advance-copies state selected)) + (rx/map (partial advance-copies-from-state state selected)) (rx/map #(t/encode-str % {:type :json-verbose})) (rx/map clipboard/to-clipboard) (rx/catch on-copy-error) diff --git a/frontend/src/app/main/data/workspace/path/shortcuts.cljs b/frontend/src/app/main/data/workspace/path/shortcuts.cljs index bd924df5fe..21f1c87960 100644 --- a/frontend/src/app/main/data/workspace/path/shortcuts.cljs +++ b/frontend/src/app/main/data/workspace/path/shortcuts.cljs @@ -127,6 +127,7 @@ :label (fn [] (tr "shortcuts.copy")) :command (ds/c-mod "c") :subsections [:path-editor] + :customizable false :fn #(st/emit! (drp/copy-selected-nodes))} :cut {:tooltip (ds/meta "X") @@ -139,6 +140,7 @@ :label (fn [] (tr "shortcuts.paste")) :command (ds/c-mod "v") :subsections [:path-editor] + :customizable false :fn #(st/emit! (drp/paste-nodes))} :duplicate {:tooltip (ds/meta "D") @@ -176,6 +178,7 @@ :label (fn [] (tr "shortcuts.escape")) :command ["escape" "v"] :section [:workspace] + :customizable false :fn #(st/emit! (esc-pressed))} ;; Reuses the `:start-editing` key (instead of adding "enter" to diff --git a/frontend/src/app/main/data/workspace/shortcuts.cljs b/frontend/src/app/main/data/workspace/shortcuts.cljs index bdbd3cedaf..17246e5889 100644 --- a/frontend/src/app/main/data/workspace/shortcuts.cljs +++ b/frontend/src/app/main/data/workspace/shortcuts.cljs @@ -94,6 +94,7 @@ :command (ds/c-mod "c") :subsections [:edit] :section [:workspace] + :customizable false :fn #(st/emit! (dw/copy-selected))} :copy-link {:tooltip (ds/shift (ds/alt "C")) @@ -178,6 +179,7 @@ :command "escape" :subsections [:edit] :section [:workspace] + :customizable false :fn #(st/emit! :interrupt (dwdc/clear-drawing) (dw/deselect-all true))} :find {:tooltip (ds/meta "F") :command (ds/c-mod "f") :subsections [:edit] diff --git a/frontend/src/app/main/ui/dashboard/organization_team_switch.cljs b/frontend/src/app/main/ui/dashboard/organization_team_switch.cljs index 9d99ecb3c0..62144bfbf7 100644 --- a/frontend/src/app/main/ui/dashboard/organization_team_switch.cljs +++ b/frontend/src/app/main/ui/dashboard/organization_team_switch.cljs @@ -69,6 +69,18 @@ (nil? (dm/get-in team [:organization :id])) (contains? #{"unlimited" "enterprise"} (get-subscription-type (:subscription team))))) +(def ^:private combining-marks-re (js/RegExp. "\\p{M}" "gu")) + +(defn- name-sort-key + "Key that sorts `label` alphabetically regardless of case and + accents: NFD splits an accented letter into its base letter plus a + combining mark, and dropping the marks makes \"Ábaco\" sort like + \"abaco\" instead of after \"z\"." + [label] + (-> (.normalize label "NFD") + (str/replace combining-marks-re "") + (str/lower))) + (defn sort-organization-teams "Orders the teams of a single organization for the dropdown's second column: alphabetical, with the organization's default team @@ -76,7 +88,7 @@ `:is-default` and an explicit `false` into one comparator value." [teams] (sort-by (juxt (fn [team] (boolean (:is-default team))) - (fn [team] (str/lower (:name team "")))) + (fn [team] (name-sort-key (:name team "")))) teams)) (defn sort-all-teams @@ -89,7 +101,7 @@ (sort-all-teams teams team-display-name)) ([teams display-name] (sort-by (juxt (fn [team] (boolean (:is-default team))) - (comp str/lower display-name)) + (comp name-sort-key display-name)) teams))) (defn teams-for-organization @@ -113,7 +125,7 @@ stable." [organizations] (sort-by (juxt (fn [organization] (if (nil? (:id organization)) 1 0)) - (fn [organization] (str/lower (:name organization ""))) + (fn [organization] (name-sort-key (:name organization ""))) :id) organizations)) diff --git a/frontend/src/app/main/ui/settings/import_shortcuts_diff_modal.cljs b/frontend/src/app/main/ui/settings/import_shortcuts_diff_modal.cljs index a7462d9380..43478a33f1 100644 --- a/frontend/src/app/main/ui/settings/import_shortcuts_diff_modal.cljs +++ b/frontend/src/app/main/ui/settings/import_shortcuts_diff_modal.cljs @@ -65,7 +65,7 @@ (mf/defc import-shortcuts-diff-modal {::mf/register modal/components ::mf/register-as :import-shortcuts-diff-modal} - [{:keys [imported-shortcuts custom-shortcuts all-shortcuts-raw]}] + [{:keys [imported-shortcuts skipped-entries custom-shortcuts all-shortcuts-raw]}] (let [diff-entries (mf/with-memo [imported-shortcuts custom-shortcuts] (->> (compute-diff imported-shortcuts custom-shortcuts) @@ -145,7 +145,17 @@ :is-customized true :has-conflict? false}])]]])))]] [:div {:class (stl/css :no-changes)} - (tr "import-shortcuts.no-changes")])] + (tr "import-shortcuts.no-changes")]) + + (when (seq skipped-entries) + [:div {:class (stl/css :skipped-entries)} + [:div {:class (stl/css :modal-content-text)} + (tr "import-shortcuts.skipped-entries")] + [:ul {:class (stl/css :skipped-entries-list)} + (for [entry skipped-entries] + [:li {:key entry + :class (stl/css :skipped-entry)} + entry])]])] [:div {:class (stl/css :modal-footer)} [:div {:class (stl/css :action-buttons)} diff --git a/frontend/src/app/main/ui/settings/import_shortcuts_diff_modal.scss b/frontend/src/app/main/ui/settings/import_shortcuts_diff_modal.scss index e810578008..c28d5de804 100644 --- a/frontend/src/app/main/ui/settings/import_shortcuts_diff_modal.scss +++ b/frontend/src/app/main/ui/settings/import_shortcuts_diff_modal.scss @@ -165,3 +165,22 @@ text-align: center; padding-block: var(--sp-xl); } + +.skipped-entries { + display: flex; + flex-direction: column; + gap: var(--sp-s); +} + +.skipped-entries-list { + display: flex; + flex-direction: column; + gap: var(--sp-xs); +} + +.skipped-entry { + @include t.use-typography("code-font"); + + color: var(--color-foreground-secondary); + overflow-wrap: anywhere; +} diff --git a/frontend/src/app/main/ui/settings/shortcuts.cljs b/frontend/src/app/main/ui/settings/shortcuts.cljs index 2b246339b2..841841ac5b 100644 --- a/frontend/src/app/main/ui/settings/shortcuts.cljs +++ b/frontend/src/app/main/ui/settings/shortcuts.cljs @@ -121,6 +121,53 @@ [:dashboard {:optional true} (context-schema :dashboard)] [:viewer {:optional true} (context-schema :viewer)]])) +(defn- entry-path + [& segments] + (str/join "/" (map d/name segments))) + +(defn- sanitize-context + "Keeps the entries of one imported context that map to a known shortcut key + with a valid command; every other entry goes to :skipped." + [result ctx entries known-keys] + (let [viable (reduce-kv + (fn [viable k command] + (if (and (contains? known-keys k) + (ss/valid-command? command) + (not (ss/reserved-command? command))) + (assoc viable k command) + viable)) + {} + entries) + skipped (->> (keys entries) + (remove #(contains? viable %)) + (map #(entry-path ctx %)))] + (cond-> (update result :skipped into skipped) + ;; A context left empty by skipped entries is dropped: an empty context + ;; clears the user's shortcuts on import, an explicit {} still does. + (or (seq viable) (empty? entries)) + (assoc-in [:shortcuts ctx] viable)))) + +(defn sanitize-imported-shortcuts + "Splits an imported custom-shortcuts payload into the entries Penpot can + apply and the ones it must skip: unknown contexts, unknown or + non-customizable shortcut keys, commands that are not a key combination + (see `app.main.ui.shortcuts/valid-command?`) and reserved combinations + (see `app.main.ui.shortcuts/reserved-command?`). + + Returns {:shortcuts :skipped [\"context/key\" ...]}, or + nil when the payload is not a map." + [data] + (when (map? data) + (-> (reduce-kv + (fn [result ctx entries] + (let [known-keys (get known-shortcut-keys ctx)] + (if (and (some? known-keys) (map? entries)) + (sanitize-context result ctx entries known-keys) + (update result :skipped conj (entry-path ctx))))) + {:shortcuts {} :skipped []} + data) + (update :skipped (comp vec sort))))) + (defn- flatten-humanize "Turns Malli's humanized explain output into a flat list of {:path :message} maps." ([human] (flatten-humanize [] human)) @@ -450,12 +497,16 @@ (rx/subs! (fn [content] (try - (let [shortcuts (js->clj (.parse js/JSON content) - :keywordize-keys true) - validation (validate-imported-shortcuts shortcuts)] - (if (:valid? validation) + (let [data (js->clj (.parse js/JSON content) + :keywordize-keys true) + {:keys [shortcuts skipped]} (sanitize-imported-shortcuts data)] + ;; Rejects the file only when nothing in it can be applied. + (if (and (some? shortcuts) + (or (seq shortcuts) (empty? skipped)) + (:valid? (validate-imported-shortcuts shortcuts))) (st/emit! (modal/show {:type :import-shortcuts-diff-modal :imported-shortcuts shortcuts + :skipped-entries skipped :custom-shortcuts custom-shortcuts :all-shortcuts-raw all-shortcuts-raw})) (st/emit! (ntf/error (tr "import-shortcuts.invalid-file"))))) diff --git a/frontend/src/app/main/ui/shortcuts.cljs b/frontend/src/app/main/ui/shortcuts.cljs index 1520f3258b..30aed17a2c 100644 --- a/frontend/src/app/main/ui/shortcuts.cljs +++ b/frontend/src/app/main/ui/shortcuts.cljs @@ -68,6 +68,122 @@ "Meta" "meta" " " "space"}) +;; Mirrors how mousetrap reads a bind string (`_keysFromString` and +;; `_getKeyInfo` in @penpot/mousetrap), so a command is valid exactly when +;; mousetrap can match it. + +(def ^:private command-aliases + {"ctrl" "ctrl" + "control" "ctrl" + "alt" "alt" + "option" "alt" + "shift" "shift" + "meta" "meta" + "command" "meta" + "return" "enter" + "enter" "enter" + "esc" "escape" + "escape" "escape" + "del" "del" + "delete" "del" + "ins" "ins" + "insert" "ins" + "space" "space" + "plus" "+"}) + +(def ^:private command-modifiers + #{"ctrl" "alt" "shift" "meta"}) + +(def ^:private command-named-keys + (into #{} (remove command-modifiers) (vals key-name-map))) + +(defn- canonical-command-key + [token] + (cond + ;; `mod` follows the platform, like `ds/c-mod`. + (= "mod" token) + (if (cf/check-platform? :macos) "meta" "ctrl") + + (contains? command-aliases token) + (get command-aliases token) + + (= 1 (count token)) + (str/lower token) + + :else + (str/lower (get key-name-map token token)))) + +(defn- command-step-keys + "Splits one step of a command (\"ctrl+shift+z\") into canonical keys. + A lone \"+\" is the plus key, and \"++\" ends a step with it." + [step] + (if (= "+" step) + ["+"] + (->> (.split (str/replace step #"\+{2}" "+plus") "+") + (mapv canonical-command-key)))) + +(defn- final-command-key? + [k] + (or (= 1 (count k)) + (contains? command-named-keys k) + ;; Two-digit alternatives first: cljs `re-matches` does not + ;; backtrack into a longer alternative. + (some? (re-matches #"f(?:1[0-9]|2[0-4]|[1-9])" k)) + (some? (re-matches #"num[0-9]" k)))) + +(defn- valid-command-step? + "A lone modifier is a valid step: `:show-measure` binds a bare `alt`." + [step-keys] + (let [final-key (peek step-keys)] + (if (contains? command-modifiers final-key) + (= 1 (count step-keys)) + (and (every? command-modifiers (pop step-keys)) + (final-command-key? final-key))))) + +(defn valid-command? + "True when `command` is a key combination mousetrap can match: steps + separated by single spaces, each a lone modifier or modifiers joined + with \"+\" ending with a single character, a named key (`escape`, `up`, + ...), a function key (`f1`..`f24`) or a numpad digit (`num0`..`num9`). + The empty string is valid: it disables the shortcut." + [command] + (and (string? command) + (or (= "" command) + (every? (comp valid-command-step? command-step-keys) + (.split command " "))))) + +(defn- canonical-command-step + "One step in the form mousetrap matches on: canonical keys with the + modifiers sorted, so `Shift+Ctrl+Z` and `ctrl+shift+z` compare equal." + [step] + (let [step-keys (command-step-keys step) + final-key (peek step-keys)] + (str/join "+" (conj (vec (sort (pop step-keys))) final-key)))) + +(defn- reserved-command-steps + "Key combinations of the non-customizable workspace shortcuts (cancel, + copy, paste): assigning them to another action would bind one key to two + actions. The path editor's cancel also binds `v`, the move tool key, + so path shortcuts are left out." + [] + (into #{} + (comp (filter #(false? (:customizable (val %)))) + (mapcat #(let [command (:command (val %))] + (if (vector? command) command [command]))) + (mapcat #(.split % " ")) + (map canonical-command-step)) + wsc/shortcuts)) + +(defn reserved-command? + "True when any step of `command` is a combination Penpot keeps for a + non-customizable shortcut." + [command] + (and (string? command) + (not= "" command) + (let [reserved (reserved-command-steps)] + (some? (some #(contains? reserved (canonical-command-step %)) + (.split command " ")))))) + (defn- event->native "Unwraps a goog.events.BrowserEvent to the native event, since the wrapper copies `key` but not `code`." @@ -601,12 +717,16 @@ effective-section-key)) (clean-editing-state))) + reserved? (and (some? recorded-command) + (not reset-pending) + (reserved-command? recorded-command)) + save-fn (mf/use-fn - (mf/deps recorded-command conflict command effective-section-key) + (mf/deps recorded-command reserved? conflict command effective-section-key) (fn [event] (dom/prevent-default event) - (when recorded-command + (when (and recorded-command (not reserved?)) (if (deref reset-pending*) (st/emit! (customize/reset-custom-shortcut command recorded-command effective-section-key)) (st/emit! (customize/set-custom-shortcut @@ -710,9 +830,16 @@ :has-conflict? false}]]) (when (and recorded-command (not reset-pending)) - (if conflict + (cond + reserved? + [:> context-notification* {:level :warning :class (stl/css :modal-error-msg)} + (tr "shortcuts.edit-modal.reserved")] + + conflict [:> context-notification* {:level :warning :class (stl/css :modal-error-msg)} (tr "shortcuts.edit-modal.conflict" (:name conflict))] + + :else [:> context-notification* {:level :success :class (stl/css :modal-success-msg)} (tr "shortcuts.edit-modal.success")])) [:div {:class (stl/css :edit-buttons)} @@ -725,6 +852,7 @@ [:> icon-button* {:variant "primary" :aria-label (tr "labels.save") :on-click save-fn + :disabled reserved? :icon i/tick :icon-size "m"}]] [:div {:class (stl/css :confirmation-buttons)} diff --git a/frontend/src/app/main/worker.cljs b/frontend/src/app/main/worker.cljs index bbfec651c6..4daaeebc36 100644 --- a/frontend/src/app/main/worker.cljs +++ b/frontend/src/app/main/worker.cljs @@ -22,7 +22,8 @@ (uw/ask! worker {:cmd :configure :config {:public-uri cf/public-uri :build-data cf/build-date - :version cf/version}}) + :version cf/version + :version-tag cf/version-tag}}) (set! instance worker))) (defn ask! diff --git a/frontend/src/app/worker/impl.cljs b/frontend/src/app/worker/impl.cljs index 5461756ff5..c7f65dc1b7 100644 --- a/frontend/src/app/worker/impl.cljs +++ b/frontend/src/app/worker/impl.cljs @@ -36,6 +36,9 @@ (when-let [version (get config :version)] (set! cf/version version)) + (when-let [version-tag (get config :version-tag)] + (set! cf/version-tag version-tag)) + (when-let [build-date (get config :build-data)] (set! cf/build-date build-date)) diff --git a/frontend/src/app/worker/thumbnails.cljs b/frontend/src/app/worker/thumbnails.cljs index 074b23c400..bed81a00e9 100644 --- a/frontend/src/app/worker/thumbnails.cljs +++ b/frontend/src/app/worker/thumbnails.cljs @@ -180,7 +180,7 @@ :file-id file-id :revn revn})) (rx/end! subs))))))) - (rx/end! subs)) + (rx/error! subs (ex-info "unable to init wasm canvas" {:file-id file-id}))) nil))))))) (defmethod impl/handler :thumbnails/generate-for-file-wasm diff --git a/frontend/test/frontend_tests/helpers/pages.cljs b/frontend/test/frontend_tests/helpers/pages.cljs index 6ac59e6259..4a4b7784a0 100644 --- a/frontend/test/frontend_tests/helpers/pages.cljs +++ b/frontend/test/frontend_tests/helpers/pages.cljs @@ -14,13 +14,11 @@ [app.common.geom.point :as gpt] [app.common.geom.shapes :as gsh] [app.common.logic.libraries :as cll] - [app.common.types.component :as ctk] - [app.common.types.container :as ctn] - [app.common.types.file :as ctf] [app.common.types.shape :as cts] [app.common.types.shape-tree :as ctst] [app.common.uuid :as uuid] [app.main.data.helpers :as dsh] + [app.main.data.workspace.clipboard :as dwc] [app.main.data.workspace.groups :as dwg] [app.main.data.workspace.layout :as layout])) @@ -225,34 +223,7 @@ (if (= parent-frame-id uuid/zero) shape (let [frame (get objects parent-frame-id)] - (gsh/translate-to-frame shape frame)))) - - ;; When copying an instance that is nested inside another one, we need to - ;; advance the shape refs to one or more levels of remote mains. - (advance-copies [data] - (let [heads (mapcat #(ctn/get-child-heads (:objects data) %) selected)] - (update data :objects - #(reduce (partial advance-copy file libraries page) - % - heads)))) - - (advance-copy [file libraries page objects shape] - (if (and (ctk/instance-head? shape) (not (ctk/main-instance? shape))) - (let [level-delta (if (nil? (ctk/get-swap-slot shape)) - (ctn/get-nesting-level-delta (:objects page) shape uuid/zero) - 0)] - (if (pos? level-delta) - (reduce (partial advance-shape file libraries page level-delta) - objects - (cfh/get-children-with-self objects (:id shape))) - objects)) - objects)) - - (advance-shape [file libraries page level-delta objects shape] - (let [new-shape-ref (ctf/advance-shape-ref file page libraries shape level-delta {:include-deleted? true})] - (cond-> objects - (and (some? new-shape-ref) (not= new-shape-ref (:shape-ref shape))) - (assoc-in [(:id shape) :shape-ref] new-shape-ref))))] + (gsh/translate-to-frame shape frame))))] (let [file-id (:id file) frame-id (cfh/common-parent-frame objects selected) @@ -273,4 +244,4 @@ (map (partial prepare-object objects frame-id)) (reduce collect-data initial) sort-selected - advance-copies)))) + (dwc/advance-copies file libraries page selected))))) diff --git a/frontend/test/frontend_tests/logic/comp_remove_swap_slots_test.cljs b/frontend/test/frontend_tests/logic/comp_remove_swap_slots_test.cljs index 476d549c5d..f93bd40d3b 100644 --- a/frontend/test/frontend_tests/logic/comp_remove_swap_slots_test.cljs +++ b/frontend/test/frontend_tests/logic/comp_remove_swap_slots_test.cljs @@ -5,6 +5,7 @@ ;; Copyright (c) KALEIDOS SUBSIDIARY SL (ns frontend-tests.logic.comp-remove-swap-slots-test (:require + [app.common.files.validate :as cfv] [app.common.test-helpers.components :as cthc] [app.common.test-helpers.compositions :as ctho] [app.common.test-helpers.files :as cthf] @@ -870,3 +871,149 @@ (t/is (some? copied-blue1')) (t/is (nil? (ctk/get-swap-slot copied-blue1'))))))))) + +(t/deftest test-keep-inherited-swap-slot-copy-paste-nested-copy-to-root + (t/async + done + (let [;; ==== Setup + ;; {:frame-red} [:name frame-red] # [Component :red] + ;; {:frame-blue} [:name frame-blue] # [Component :blue] + ;; {:frame-icon} [:name frame-icon] # [Component :icon] + ;; :red-copy-icon [:name frame-red] @--> frame-red + ;; {:frame-title} [:name frame-title] # [Component :title] + ;; :icon-copy-title [:name frame-icon] @--> frame-icon + ;; :blue-copy-title [:name frame-blue, :swap-slot-label :red-copy-icon] @--> frame-blue + ;; :title-copy [:name frame-title] #--> frame-title + ;; :icon-copy-in-title-copy [:name frame-icon] @--> :icon-copy-title + ;; :blue-copy-in-title-copy [:name frame-blue] @--> :blue-copy-title + file (-> (cthf/sample-file :file1) + (ctho/add-frame :frame-red :name "frame-red") + (cthc/make-component :red :frame-red) + (ctho/add-frame :frame-blue :name "frame-blue") + (cthc/make-component :blue :frame-blue) + (ctho/add-frame :frame-icon :name "frame-icon") + (cthc/make-component :icon :frame-icon) + (cthc/instantiate-component :red :red-copy-icon :parent-label :frame-icon) + (ctho/add-frame :frame-title :name "frame-title") + (cthc/make-component :title :frame-title) + (cthc/instantiate-component :icon :icon-copy-title :parent-label :frame-title + :children-labels [:red-copy-in-icon-copy-title]) + (cthc/component-swap :red-copy-in-icon-copy-title :blue :blue-copy-title) + (cthc/instantiate-component :title :title-copy + :children-labels [:icon-copy-in-title-copy + :blue-copy-in-title-copy])) + store (ths/setup-store file) + + ;; ==== Action + page (cthf/current-page file) + icon (cths/get-shape file :icon-copy-in-title-copy) + features #{"components/v2"} + version 47 + + pdata (thp/simulate-copy-shape #{(:id icon)} (:objects page) {(:id file) file} page file features version) + + events + [(dws/select-shape uuid/zero) + (dw/paste-shapes pdata)]] + + (ths/run-store + store done events + (fn [new-state] + (let [;; ==== Get + file' (ths/get-file-from-state new-state) + page' (cthf/current-page file') + icon' (cths/get-shape file' :icon-copy-in-title-copy) + blue' (cths/get-shape file' :blue-copy-in-title-copy) + copied-icon' (find-copied-shape icon' page' uuid/zero) + copied-blue' (find-copied-shape blue' page' (:id copied-icon')) + errors (try + (cfv/validate-file! file' {(:id file') file'}) + nil + (catch :default cause + (ex-data cause)))] + + ;; ==== Check + ;; the copied icon now refers to the icon main, where blue + ;; does not exist, so the copied blue must keep the swap slot + ;; inherited from :blue-copy-title + (t/is (some? copied-icon')) + (t/is (= (:shape-ref copied-icon') (cthi/id :frame-icon))) + (t/is (some? copied-blue')) + (t/is (= (ctk/get-swap-slot copied-blue') (cthi/id :red-copy-icon))) + + ;; the file must be valid after the paste + (t/is (nil? errors)))))))) + +(t/deftest test-keep-touched-copy-paste-nested-copy-to-root + (t/async + done + (let [;; ==== Setup + ;; Add a nested copy inside a copy of :green, and override its fill + ;; so that the copy has a touched attribute. + file (-> (setup-file) + (cthc/instantiate-component :green :green-copy-2 + :parent-label :frame-b2 + :children-labels [:red-copy-in-green-copy-2]) + (cths/update-shape :red-copy-in-green-copy-2 + :fills (cths/sample-fills-color :fill-color "#ff0000"))) + store (ths/setup-store file) + + ;; ==== Action + page (cthf/current-page file) + red (cths/get-shape file :red-copy-in-green-copy-2) + features #{"components/v2"} + version 47 + + pdata (thp/simulate-copy-shape #{(:id red)} (:objects page) {(:id file) file} page file features version) + + events + [(dws/select-shape uuid/zero) + (dw/paste-shapes pdata)]] + + (ths/run-store + store done events + (fn [new-state] + (let [;; ==== Get + file' (ths/get-file-from-state new-state) + page' (cthf/current-page file') + red' (cths/get-shape file' :red-copy-in-green-copy-2) + copied-red' (find-copied-shape red' page' uuid/zero)] + + ;; ==== Check + ;; the touched fill override must be preserved in the pasted copy + (t/is (some? copied-red')) + (t/is (contains? (:touched copied-red') :fill-group)) + (t/is (= (:touched red') (:touched copied-red'))))))))) + +(t/deftest test-remove-swap-slot-copy-paste-nested-swapped-copy-to-root + (t/async + done + (let [;; ==== Setup (sample file as is) + file (setup-file) + store (ths/setup-store file) + + ;; ==== Action + page (cthf/current-page file) + blue (cths/get-shape file :blue-copy-in-green-copy) + features #{"components/v2"} + version 47 + + pdata (thp/simulate-copy-shape #{(:id blue)} (:objects page) {(:id file) file} page file features version) + + events + [(dws/select-shape uuid/zero) + (dw/paste-shapes pdata)]] + + (ths/run-store + store done events + (fn [new-state] + (let [;; ==== Get + file' (ths/get-file-from-state new-state) + page' (cthf/current-page file') + blue' (cths/get-shape file' :blue-copy-in-green-copy) + copied-blue' (find-copied-shape blue' page' uuid/zero)] + + ;; ==== Check + ;; the copied blue has no swap-id + (t/is (some? copied-blue')) + (t/is (nil? (ctk/get-swap-slot copied-blue'))))))))) diff --git a/frontend/test/frontend_tests/ui/organization_team_switch_test.cljs b/frontend/test/frontend_tests/ui/organization_team_switch_test.cljs index 6cf00fa228..6d10a260f4 100644 --- a/frontend/test/frontend_tests/ui/organization_team_switch_test.cljs +++ b/frontend/test/frontend_tests/ui/organization_team_switch_test.cljs @@ -46,6 +46,26 @@ (t/is (= ["org-a" "org-b" "org-c" nil] (map :id (dts/sort-organizations organizations)))))) +(t/deftest names-starting-with-an-accented-letter-sort-as-the-plain-letter + ;; A code-point comparison would push "Ábaco", "Órbita" and "Ámbar" + ;; after "z"; accented letters must sort like their base letter. + (let [organizations [{:id "org-1" :name "OrgA"} + nil + {:id "org-2" :name "Órbita"} + {:id "org-3" :name "beta"} + {:id "org-4" :name "Ábaco"}] + teams [{:id "t1" :name "zeta"} + {:id "t2" :name "Personal projects" :is-default true} + {:id "t3" :name "apricot"} + {:id "t4" :name "Ámbar"}] + display-name (fn [team] (if (:is-default team) "Personal projects" (:name team)))] + (t/is (= ["Ábaco" "beta" "Órbita" "OrgA" nil] + (map :name (dts/sort-organizations organizations)))) + (t/is (= ["Ámbar" "apricot" "zeta" "Personal projects"] + (map :name (dts/sort-organization-teams teams)))) + (t/is (= ["Ámbar" "apricot" "zeta" "Personal projects"] + (map :name (dts/sort-all-teams teams display-name)))))) + (t/deftest closed-control-line-2-matrix (t/testing "profile with organizations, destination has an organization" (t/is (= "Org A" diff --git a/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs b/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs index fd82bfe17d..3ed7e035c9 100644 --- a/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs +++ b/frontend/test/frontend_tests/ui/settings_shortcuts_test.cljs @@ -1,8 +1,12 @@ (ns frontend-tests.ui.settings-shortcuts-test (:require [app.config :as cf] + [app.main.data.dashboard.shortcuts :as dsc] [app.main.data.profile :as du] [app.main.data.shortcuts :as ds] + [app.main.data.viewer.shortcuts :as vsc] + [app.main.data.workspace.path.shortcuts :as psc] + [app.main.data.workspace.shortcuts :as wsc] [app.main.ui.settings.import-shortcuts-diff-modal :as diff-modal] [app.main.ui.settings.restore-shortcuts-modal :as restore-modal] [app.main.ui.settings.shortcuts :as sut] @@ -14,21 +18,21 @@ (t/deftest validate-imported-shortcuts-accepts-valid-workspace (t/is (= {:valid? true} (sut/validate-imported-shortcuts - {:workspace {:escape "escape" + {:workspace {:draw-rect "shift+r" :increase-zoom "+" :zoom-lense-decrease "alt+z"}})))) (t/deftest validate-imported-shortcuts-accepts-optional-contexts (t/is (= {:valid? true} (sut/validate-imported-shortcuts - {:workspace {:escape "escape"} + {:workspace {:draw-rect "shift+r"} :dashboard {:toggle-theme "alt+m"} :viewer {:next-frame "right"}})))) (t/deftest validate-imported-shortcuts-accepts-empty-string-command (t/is (= {:valid? true} (sut/validate-imported-shortcuts - {:workspace {:escape ""}})))) + {:workspace {:draw-rect ""}})))) (t/deftest validate-imported-shortcuts-rejects-nil (let [result (sut/validate-imported-shortcuts nil)] @@ -52,19 +56,19 @@ (t/is (some #(= "workspace" (:path %)) (:errors result))))) (t/deftest validate-imported-shortcuts-rejects-non-string-command - (let [result (sut/validate-imported-shortcuts {:workspace {:escape 123}})] + (let [result (sut/validate-imported-shortcuts {:workspace {:draw-rect 123}})] (t/is (= false (:valid? result))) (t/is (seq (:errors result))) - (t/is (some #(= "workspace/escape" (:path %)) (:errors result))))) + (t/is (some #(= "workspace/draw-rect" (:path %)) (:errors result))))) (t/deftest validate-imported-shortcuts-rejects-vector-command - (let [result (sut/validate-imported-shortcuts {:workspace {:escape ["ctrl+e"]}})] + (let [result (sut/validate-imported-shortcuts {:workspace {:draw-rect ["ctrl+e"]}})] (t/is (= false (:valid? result))) (t/is (seq (:errors result))) - (t/is (some #(= "workspace/escape" (:path %)) (:errors result))))) + (t/is (some #(= "workspace/draw-rect" (:path %)) (:errors result))))) (t/deftest validate-imported-shortcuts-rejects-unknown-top-level-key - (let [result (sut/validate-imported-shortcuts {:workspace {:escape "escape"} + (let [result (sut/validate-imported-shortcuts {:workspace {:draw-rect "shift+r"} :unknown-context {}})] (t/is (= false (:valid? result))) (t/is (seq (:errors result))) @@ -87,6 +91,195 @@ (t/is (= false (:valid? (sut/validate-imported-shortcuts #{:workspace})))) (t/is (= false (:valid? (sut/validate-imported-shortcuts (fn [])))))) +(t/deftest sanitize-imported-shortcuts-skips-unknown-action-keys + (t/is (= {:shortcuts {:workspace {:draw-rect "shift+r"}} + :skipped ["workspace/not-a-real-shortcut"]} + (sut/sanitize-imported-shortcuts + {:workspace {:draw-rect "shift+r" + :not-a-real-shortcut "x"}})))) + +(t/deftest sanitize-imported-shortcuts-skips-unknown-contexts + (t/is (= {:shortcuts {:workspace {:draw-rect "shift+r"}} + :skipped ["unknown-context"]} + (sut/sanitize-imported-shortcuts + {:workspace {:draw-rect "shift+r"} + :unknown-context {:draw-rect "shift+r"}})))) + +(t/deftest sanitize-imported-shortcuts-skips-non-string-commands + (t/is (= {:shortcuts {:workspace {:zoom-lense-decrease "alt+z"}} + :skipped ["workspace/draw-rect" "workspace/increase-zoom"]} + (sut/sanitize-imported-shortcuts + {:workspace {:draw-rect 123 + :increase-zoom ["+"] + :zoom-lense-decrease "alt+z"}})))) + +(t/deftest sanitize-imported-shortcuts-skips-invalid-commands + (t/is (= {:shortcuts {:workspace {:zoom-lense-decrease "alt+z"}} + :skipped ["workspace/draw-rect" "workspace/increase-zoom"]} + (sut/sanitize-imported-shortcuts + {:workspace {:draw-rect "foo+bar" + :increase-zoom "ctrl+" + :zoom-lense-decrease "alt+z"}})))) + +(t/deftest cancel-copy-and-paste-are-not-customizable + (t/testing "in the workspace and in the path editor, which share the + shortcut keys of the workspace context" + (doseq [shortcuts [wsc/shortcuts psc/shortcuts] + k [:escape :copy :paste]] + (t/is (false? (get-in shortcuts [k :customizable])) + (str k))))) + +(t/deftest sanitize-imported-shortcuts-skips-non-customizable-keys + (t/testing "cancel, copy and paste cannot be imported, even with a free + key combination" + (t/is (= {:shortcuts {} + :skipped ["workspace/copy" "workspace/escape" "workspace/paste"]} + (sut/sanitize-imported-shortcuts + {:workspace {:escape "shift+escape" + :copy "ctrl+shift+1" + :paste "ctrl+shift+2"}}))))) + +(t/deftest apply-custom-overrides-skips-non-customizable-shortcuts + (t/testing "an override stored before a shortcut became non-customizable + is ignored, since the settings page can no longer reset it" + (let [shortcuts {:escape {:command "escape" :customizable false} + :undo {:command "ctrl+z"}} + result (ds/apply-custom-overrides shortcuts + {:workspace {:escape "shift+escape" + :undo "ctrl+u"}} + :workspace)] + (t/is (= "escape" (get-in result [:escape :command]))) + (t/is (= "ctrl+u" (get-in result [:undo :command])))))) + +(t/deftest sanitize-imported-shortcuts-skips-reserved-commands + (t/is (= {:shortcuts {:workspace {:zoom-lense-decrease "alt+z"}} + :skipped ["workspace/draw-rect" "workspace/increase-zoom"]} + (sut/sanitize-imported-shortcuts + {:workspace {:draw-rect "escape" + :increase-zoom "ctrl+v" + :zoom-lense-decrease "alt+z"}})))) + +(t/deftest sanitize-imported-shortcuts-skips-copy-combination + (t/is (= {:shortcuts {:workspace {:draw-rect "ctrl+alt+c"}} + :skipped ["workspace/draw-ellipse"]} + (sut/sanitize-imported-shortcuts + {:workspace {:draw-ellipse "ctrl+c" + :draw-rect "ctrl+alt+c"}})))) + +(t/deftest sanitize-imported-shortcuts-skips-non-map-context + (t/is (= {:shortcuts {:viewer {:next-frame "right"}} + :skipped ["workspace"]} + (sut/sanitize-imported-shortcuts + {:workspace "not-a-map" + :viewer {:next-frame "right"}})))) + +(t/deftest sanitize-imported-shortcuts-drops-context-without-viable-entries + (t/testing "a context whose entries are all skipped is left out, so the + import does not clear the user's shortcuts in that context" + (t/is (= {:shortcuts {} + :skipped ["viewer/not-a-real-shortcut"]} + (sut/sanitize-imported-shortcuts + {:viewer {:not-a-real-shortcut "x"}}))))) + +(t/deftest sanitize-imported-shortcuts-keeps-empty-context + (t/is (= {:shortcuts {:workspace {}} + :skipped []} + (sut/sanitize-imported-shortcuts {:workspace {}})))) + +(t/deftest sanitize-imported-shortcuts-returns-nil-for-non-maps + (t/is (nil? (sut/sanitize-imported-shortcuts nil))) + (t/is (nil? (sut/sanitize-imported-shortcuts "shortcuts"))) + (t/is (nil? (sut/sanitize-imported-shortcuts 42))) + (t/is (nil? (sut/sanitize-imported-shortcuts [])))) + +(t/deftest sanitize-imported-shortcuts-output-is-valid + (let [{:keys [shortcuts]} (sut/sanitize-imported-shortcuts + {:workspace {:draw-rect "shift+r" + :not-a-real-shortcut "x" + :increase-zoom 1} + :dashboard "not-a-map" + :viewer {:next-frame "right"} + :unknown-context {}})] + (t/is (= {:valid? true} + (sut/validate-imported-shortcuts shortcuts))))) + +(t/deftest valid-command-accepts-key-combinations + (t/are [command] (true? (ui-shortcuts/valid-command? command)) + "" + "a" + "ctrl+shift+k" + "Ctrl+s" + "command+alt+ñ" + "+" + "shift++" + "ctrl+plus" + "shift+\"" + "g v" + "escape" + "Escape" + "f1" + "f12" + "f24" + "num5" + "alt")) + +(t/deftest valid-command-rejects-other-strings + (t/are [command] (false? (ui-shortcuts/valid-command? command)) + "foo" + "foo+bar" + "a+b" + "ctrl+foo" + "ctrl+" + "ctrl+shift" + "++" + "f25" + " " + " a" + "a " + "a b" + "🙂" + nil + 123)) + +(t/deftest reserved-command-matches-non-customizable-combinations + (t/testing "cancel, copy and paste keep their keys; spelling and modifier + order do not matter, and a reserved step taints a sequence" + (with-redefs [cf/check-platform? (constantly false)] + (t/are [command] (true? (ui-shortcuts/reserved-command? command)) + "escape" + "Escape" + "esc" + "ctrl+c" + "Control+C" + "mod+c" + "ctrl+v" + "Control+V" + "mod+v" + "g escape" + "ctrl+c g")))) + +(t/deftest reserved-command-ignores-other-combinations + (with-redefs [cf/check-platform? (constantly false)] + (t/are [command] (false? (ui-shortcuts/reserved-command? command)) + "" + "v" + "c" + "shift+escape" + "ctrl+shift+v" + "ctrl+shift+c" + "ctrl+alt+c" + "shift+alt+c" + nil))) + +(t/deftest valid-command-accepts-every-default-command + (t/testing "a profile exported by Penpot always holds valid commands" + (doseq [shortcuts [psc/shortcuts wsc/shortcuts dsc/shortcuts vsc/shortcuts] + [shortcut-key {:keys [command]}] shortcuts + command (if (vector? command) command [command]) + :when (some? command)] + (t/is (ui-shortcuts/valid-command? command) + (str shortcut-key " -> " (pr-str command)))))) + (t/deftest import-custom-shortcuts-updates-multiple-contexts (let [imported {:workspace {:add-comment "alt+c"} :viewer {:select-all "ctrl+alt+a"}} @@ -173,17 +366,17 @@ (t/is (= "ctrl+shift+a" (:imported select-all-entry)))))) (t/deftest compute-diff-shows-empty-string-as-disabled-shortcut - (let [imported {:workspace {:escape ""}} + (let [imported {:workspace {:draw-rect ""}} current {} diff (diff-modal/compute-diff imported current)] (t/is (pos? (count diff))) - (let [entry (first (filter #(= :escape (:key %)) diff))] + (let [entry (first (filter #(= :draw-rect (:key %)) diff))] (t/is (= :workspace (:context entry))) (t/is (= "" (:imported entry)))))) (t/deftest compute-diff-excludes-unchanged-shortcuts - (let [imported {:workspace {:escape ""}} - current {:workspace {:escape ""}} + (let [imported {:workspace {:draw-rect ""}} + current {:workspace {:draw-rect ""}} diff (diff-modal/compute-diff imported current)] (t/is (empty? diff)))) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 5c22390bbd..fe90880cc5 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -2169,6 +2169,10 @@ msgstr "This file is not a valid shortcuts export." msgid "import-shortcuts.no-changes" msgstr "No differences found between the current and imported shortcuts." +#: src/app/main/ui/settings/import_shortcuts_diff_modal.cljs:153 +msgid "import-shortcuts.skipped-entries" +msgstr "These entries can't be imported: they don't match any Penpot shortcut or use a reserved key combination." + #: src/app/main/ui/components/color_input.cljs:31 msgid "inspect.attributes.color" msgstr "Color" @@ -4973,6 +4977,10 @@ msgstr "Edit %s" msgid "shortcuts.edit-modal.conflict" msgstr "Already assigned to \"%s\". Saving will move it to this action." +#: src/app/main/ui/shortcuts.cljs:836 +msgid "shortcuts.edit-modal.reserved" +msgstr "This combination is reserved by Penpot and can't be assigned." + #: src/app/main/ui/shortcuts.cljs:636 msgid "shortcuts.edit-modal.reset-pending" msgstr "Saving will reset this shortcut to default:" diff --git a/frontend/translations/es.po b/frontend/translations/es.po index 4900d82796..2d2b8109bd 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -2205,6 +2205,10 @@ msgstr "Este archivo no es una exportación de atajos válida." msgid "import-shortcuts.no-changes" msgstr "No se encontraron diferencias entre los atajos actuales y los importados." +#: src/app/main/ui/settings/import_shortcuts_diff_modal.cljs:153 +msgid "import-shortcuts.skipped-entries" +msgstr "Estas entradas no se pueden importar: no corresponden a ningún atajo de Penpot o usan una combinación reservada." + #: src/app/main/ui/components/color_input.cljs:31 msgid "inspect.attributes.color" msgstr "Color" @@ -5037,6 +5041,10 @@ msgstr "Editar %s" msgid "shortcuts.edit-modal.conflict" msgstr "Ya asignado a \"%s\". Al guardar, pasará a esta acción." +#: src/app/main/ui/shortcuts.cljs:836 +msgid "shortcuts.edit-modal.reserved" +msgstr "Penpot reserva esta combinación y no se puede asignar." + #: src/app/main/ui/shortcuts.cljs:636 msgid "shortcuts.edit-modal.reset-pending" msgstr "Al guardar, este atajo volverá a su valor por defecto:" diff --git a/render-wasm/src/wasm/fills/image.rs b/render-wasm/src/wasm/fills/image.rs index 86c9a12abe..301e5cfd1a 100644 --- a/render-wasm/src/wasm/fills/image.rs +++ b/render-wasm/src/wasm/fills/image.rs @@ -1,30 +1,67 @@ use crate::error::{Error, Result}; use crate::get_resources; use crate::mem; -use crate::shapes::Fill; +use crate::shapes::{Fill, Shape, TextContent, Type}; use crate::state::State; use crate::uuid::Uuid; use crate::with_state; use crate::{shapes::ImageFill, utils::uuid_from_u32_quartet}; use macros::wasm_error; +fn text_content_uses_image(content: &TextContent, image_id: Uuid) -> bool { + content.paragraphs().iter().any(|paragraph| { + paragraph.children().iter().any(|span| { + span.fills + .iter() + .any(|f| matches!(f, Fill::Image(i) if i.id() == image_id)) + }) + }) +} + +fn shape_uses_image(shape: &Shape, image_id: Uuid) -> bool { + if shape + .fills() + .any(|f| matches!(f, Fill::Image(i) if i.id() == image_id)) + || shape + .strokes + .iter() + .any(|s| matches!(&s.fill, Fill::Image(i) if i.id() == image_id)) + { + return true; + } + match &shape.shape_type { + Type::Text(content) => text_content_uses_image(content, image_id), + _ => false, + } +} + +fn rebuild_text_layout_for_image(shape: &mut Shape) { + let Type::Text(text_content) = &mut shape.shape_type else { + return; + }; + // Image shaders are baked into Skia paragraphs at layout time. When the + // media arrives after the first layout, force a rebuild so text fills + // pick up `get_image` (rect fills look up the image every paint). + text_content.force_next_layout_update(); + text_content.update_layout(shape.selrect); + shape.apply_deferred_batch_paint(); + shape.invalidate_extrect(); +} + fn touch_shapes_with_image(state: &mut State, image_id: Uuid) { - let ids: Vec = state + let ids: Vec<(Uuid, bool)> = state .shapes .iter() - .filter(|shape| { - shape - .fills() - .any(|f| matches!(f, Fill::Image(i) if i.id() == image_id)) - || shape - .strokes - .iter() - .any(|s| matches!(&s.fill, Fill::Image(i) if i.id() == image_id)) - }) - .map(|shape| shape.id) + .filter(|shape| shape_uses_image(shape, image_id)) + .map(|shape| (shape.id, matches!(shape.shape_type, Type::Text(_)))) .collect(); - for id in ids { + for (id, is_text) in ids { + if is_text { + if let Some(shape) = state.shapes.get_mut(&id) { + rebuild_text_layout_for_image(shape); + } + } state.touch_shape(id); } }