Chore: Add deprecation for :editor/command-trigger config (#8720)

* Add deprecation for :editor/command-trigger config * enhance: warning dialog * enhance: notification styles * enhance: notification icons * Fix tests after config warning improvements --------- Co-authored-by: Konstantinos Kaloutas <konstantinos@logseq.com>
logseq · Mar 4, 2023 · 91e89ef · 91e89ef
1 parent 5d0117b
commit 91e89ef
Show file tree

Hide file tree

Showing 5 changed files with 95 additions and 32 deletions.
diff --git a/resources/css/common.css b/resources/css/common.css
@@ -95,9 +95,9 @@ html[data-theme='dark'] {
   --ls-highlight-color-blue: var(--color-blue-900);
   --ls-highlight-color-purple: var(--color-purple-900);
   --ls-highlight-color-pink: var(--color-pink-900);
-  --ls-error-text-color: var(--color-red-100);
+  --ls-error-text-color: var(--color-red-400);
   --ls-error-background-color: var(--color-red-900);
-  --ls-warning-text-color: var(--color-yellow-100);
+  --ls-warning-text-color: var(--color-yellow-400);
   --ls-warning-background-color: var(--color-yellow-900);
   --ls-success-text-color: var(--color-green-100);
   --ls-success-background-color: var(--color-green-900);
@@ -173,9 +173,9 @@ html[data-theme='light'] {
   --ls-highlight-color-blue: var(--color-blue-100);
   --ls-highlight-color-purple: var(--color-purple-100);
   --ls-highlight-color-pink: var(--color-pink-100);
-  --ls-error-text-color: var(--color-red-800);
+  --ls-error-text-color: var(--color-red-600);
   --ls-error-background-color: var(--color-red-100);
-  --ls-warning-text-color: var(--color-yellow-800);
+  --ls-warning-text-color: var(--color-yellow-700);
   --ls-warning-background-color: var(--color-yellow-100);
   --ls-success-text-color: var(--color-green-800);
   --ls-success-background-color: var(--color-green-100);

diff --git a/src/main/frontend/handler/common/config_edn.cljs b/src/main/frontend/handler/common/config_edn.cljs
@@ -1,11 +1,13 @@
 (ns frontend.handler.common.config-edn
   "Common fns related to config.edn - global and repo"
-  (:require [clojure.edn :as edn]
-            [clojure.string :as string :refer [includes?]]
+  (:require [malli.error :as me]
+            [malli.core :as m]
+            [clojure.string :as string]
+            [clojure.edn :as edn]
+            [lambdaisland.glogi :as log]
             [frontend.handler.notification :as notification]
             [goog.string :as gstring]
-            [malli.core :as m]
-            [malli.error :as me]))
+            [reitit.frontend.easy :as rfe]))
 
 (defn- humanize-more
   "Make error maps from me/humanize more readable for users. Doesn't try to handle
@@ -19,18 +21,34 @@ nested keys or positional errors e.g. tuples"
        [k (->> v flatten (remove nil?) first)]))
    errors))
 
+(defn- file-link
+  [path]
+  [:a {:href (rfe/href :file {:path path})} path])
+
+(defn- error-list
+  [errors class]
+  (map (fn [[k v]]
+         [:dl.my-2.mb-0
+          [:dt.m-0 [:strong (str k)]]
+          [:dd {:class class} v]]) errors))
+
+(defn config-notification-show!
+  ([title body]
+   (config-notification-show! title body :error))
+  ([title body status]
+   (config-notification-show! title body status true))
+  ([title body status clear?]
+   (notification/show!
+    [:.mb-2
+     [:.text-lg.mb-2 title]
+     body] status clear?)))
+
 (defn- validate-config-map
   [m schema path]
   (if-let [errors (->> m (m/explain schema) me/humanize)]
     (do
-      (notification/show! (gstring/format "The file '%s' has the following errors:\n%s"
-                                          path
-                                          (->> errors
-                                               humanize-more
-                                               (map (fn [[k v]]
-                                                      (str k " - " v)))
-                                               (string/join "\n")))
-                          :error)
+      (config-notification-show! [:<> "The file " (file-link path) " has the following errors:"]
+                                 (error-list (humanize-more errors) "text-error"))
       false)
     true))
 
@@ -47,7 +65,7 @@ nested keys or positional errors e.g. tuples"
     (cond
       (nil? parsed-body)
       true
-      (and failed? (includes? (second parsed-body) "duplicate key"))
+      (and failed? (string/includes? (second parsed-body) "duplicate key"))
       (do
         (notification/show! (gstring/format "The file '%s' has duplicate keys. The key '%s' is assigned multiple times."
                                             path, (subs (second parsed-body) 36))
@@ -56,18 +74,35 @@ nested keys or positional errors e.g. tuples"
 
       failed?
       (do
-
-        (notification/show! (gstring/format "Failed to read file '%s'. Make sure your config is wrapped
-in {}. Also make sure that the characters '( { [' have their corresponding closing character ') } ]'."
-                                            path)
-                            :error)
-        false)
+        (config-notification-show! [:<> "Failed to read file " (file-link path)]
+                                   "Make sure your config is wrapped in {}. Also make sure that the characters '( { [' have their corresponding closing character ') } ]'.")
+                false)
       ;; Custom error message is better than malli's "invalid type" error
       (not (map? parsed-body))
       (do
-        (notification/show! (gstring/format "The file '%s' is not valid. Make sure the config is wrapped in {}."
-                                            path)
-                            :error)
+        (config-notification-show! [:<> "The file " (file-link path) " s not valid."]
+                                   "Make sure the config is wrapped in {}.")
         false)
       :else
       (validate-config-map parsed-body schema path))))
+
+(defn detect-deprecations
+  "Detects config keys that will or have been deprecated"
+  [path content]
+  (let [body (try (edn/read-string content)
+               (catch :default _ ::failed-to-detect))
+        warnings {:editor/command-trigger
+                  "Will no longer be supported soon. Please use '/' and report bugs on it."}]
+    (cond
+      (= body ::failed-to-detect)
+      (log/info :msg "Skip deprecation check since config is not valid edn")
+
+      (not (map? body))
+      (log/info :msg "Skip deprecation check since config is not a map")
+
+      :else
+      (when-let [deprecations (seq (keep #(when (body (key %)) %) warnings))]
+        (config-notification-show! [:<> "The file " (file-link path) " has the following deprecations:"]
+                                   (error-list deprecations "text-warning")
+                                   :warning
+                                   false)))))
diff --git a/src/main/frontend/handler/file.cljs b/src/main/frontend/handler/file.cljs
@@ -89,6 +89,14 @@
     :else
     nil))
 
+(defn- detect-deprecations
+  [repo path content]
+  (when (or (= path (config/get-repo-config-path repo))
+            (and
+             (config/global-config-enabled?)
+             (= (path/dirname path) (global-config-handler/global-config-dir))))
+    (config-edn-common-handler/detect-deprecations path content)))
+
 (defn- validate-file
   "Returns true if valid and if false validator displays error message. Files
   that are not validated just return true"
@@ -129,6 +137,7 @@
                            skip-compare? false}}]
   (let [path (gp-util/path-normalize path)
         config-file? (string/ends-with? path config/config-file)
+        _ (when config-file? (detect-deprecations repo path content))
         config-valid? (and config-file? (validate-file repo path content))]
     (when-not (and config-file? (not config-valid?)) ; non-config file or valid config
       (let [opts {:new-graph? new-graph?

diff --git a/src/main/frontend/ui.cljs b/src/main/frontend/ui.cljs
@@ -218,15 +218,15 @@
     (let [svg
           (case status
             :success
-            (icon "circle-check" {:class "text-green-500" :size "22"})
+            (icon "circle-check" {:class "text-success" :size "32"})
 
             :warning
-            (icon "alert-circle" {:class "text-yellow-500" :size "22"})
+            (icon "alert-circle" {:class "text-warning" :size "32"})
 
             :error
-            (icon "circle-x" {:class "text-red-500" :size "22"})
+            (icon "circle-x" {:class "text-error" :size "32"})
 
-            (icon "info-circle" {:class "text-indigo-500" :size "22"}))]
+            (icon "info-circle" {:class "text-indigo-500" :size "32"}))]
       [:div.ui__notifications-content
        {:style
         (when (or (= state "exiting")

diff --git a/src/test/frontend/handler/common/config_edn_test.cljs b/src/test/frontend/handler/common/config_edn_test.cljs
@@ -4,16 +4,26 @@
             [frontend.handler.common.config-edn :as config-edn-common-handler]
             [frontend.schema.handler.global-config :as global-config-schema]
             [frontend.schema.handler.repo-config :as repo-config-schema]
-            [frontend.handler.notification :as notification]))
+            [frontend.handler.notification :as notification]
+            [reitit.frontend.easy :as rfe]))
 
 (defn- validation-config-error-for
   [config-body schema]
   (let [error-message (atom nil)]
-    (with-redefs [notification/show! (fn [msg _] (reset! error-message msg))]
+    (with-redefs [notification/show! (fn [msg _] (reset! error-message msg))
+                  rfe/href (constantly "")]
       (is (= false
              (config-edn-common-handler/validate-config-edn "config.edn" config-body schema)))
       (str @error-message))))
 
+(defn- deprecation-warnings-for
+  [config-body]
+  (let [error-message (atom nil)]
+    (with-redefs [notification/show! (fn [msg _] (reset! error-message msg))
+                  rfe/href (constantly "")]
+      (config-edn-common-handler/detect-deprecations "config.edn" config-body)
+      (str @error-message))))
+
 (deftest validate-config-edn
   (testing "Valid cases"
     (is (= true
@@ -47,3 +57,12 @@
       (is (string/includes?
            (validation-config-error-for "{:start-of-week 7\n:start-of-week 8}" schema)
            "The key ':start-of-week' is assigned multiple times")))))
+
+(deftest detect-deprecations
+  (is (re-find
+       #":editor/command-trigger.*Will"
+       (deprecation-warnings-for "{:preferred-workflow :todo :editor/command-trigger \",\"}"))
+      "Warning when there is a deprecation")
+
+  (is (= "" (deprecation-warnings-for "{:preferred-workflow :todo}"))
+      "No warning when there is no deprecation"))