diff options
| -rw-r--r-- | modules/org-webclipper.el | 35 | ||||
| -rw-r--r-- | tests/test-org-webclipper-commands.el | 70 |
2 files changed, 60 insertions, 45 deletions
diff --git a/modules/org-webclipper.el b/modules/org-webclipper.el index cf4cdbf0..2a6d7164 100644 --- a/modules/org-webclipper.el +++ b/modules/org-webclipper.el @@ -45,11 +45,16 @@ ;; Variables for storing org-protocol data -(defvar cj/webclip-current-url nil - "Temporary storage for URL passed via org-protocol.") +(defvar cj/--webclip-url nil + "URL for the active web clip, dynamically bound around `org-capture'. +The org-protocol entry point `let'-binds this for the dynamic extent of +its capture call, so the capture template and handler see it while the +capture runs, and an aborted or erroring capture unwinds the binding +instead of leaving stale state for the next capture.") -(defvar cj/webclip-current-title nil - "Temporary storage for page title passed via org-protocol.") +(defvar cj/--webclip-title nil + "Page title for the active web clip, dynamically bound around `org-capture'. +See `cj/--webclip-url' for the binding contract.") ;; Flag to track if we've done initialization (defvar cj/webclipper-initialized nil @@ -80,7 +85,7 @@ (add-to-list 'org-capture-templates '("W" "Web Clipper (Protocol)" entry (file+headline webclipped-file "Webclipped Inbox") - "* [[%(identity cj/webclip-current-url)][%(identity cj/webclip-current-title)]] :website:\nURL: %(identity cj/webclip-current-url)\nCaptured On:%U\n%(cj/org-protocol-webclip-handler)\n" + "* [[%(identity cj/--webclip-url)][%(identity cj/--webclip-title)]] :website:\nURL: %(identity cj/--webclip-url)\nCaptured On:%U\n%(cj/org-protocol-webclip-handler)\n" :prepend t :immediate-finish t) t)) @@ -133,11 +138,13 @@ downstream inside the capture handler with confusing messages." (user-error "org-protocol webclip: :title must be a string when provided, got %S" title)) - ;; Store the URL and title for the capture template to use - (setq cj/webclip-current-url url - cj/webclip-current-title (or title "Untitled")) - ;; Trigger the capture - (org-capture nil "W") + ;; Bind url+title for the dynamic extent of the capture call only, so + ;; the template and handler see them while the capture runs and an + ;; aborted/erroring capture unwinds the binding rather than leaving + ;; stale state for the next clip. + (let ((cj/--webclip-url url) + (cj/--webclip-title (or title "Untitled"))) + (org-capture nil "W")) nil)) ; Return nil to indicate we handled it (defun cj/org-protocol-webclip-handler () @@ -154,12 +161,8 @@ It fetches the page content and converts it to Org format." (require 'org-web-tools) (setq org-web-tools-pandoc-sleep-time 0.5) - (let ((url cj/webclip-current-url) - (title cj/webclip-current-title)) - ;; Clear the stored values after using them - (setq cj/webclip-current-url nil - cj/webclip-current-title nil) - + (let ((url cj/--webclip-url) + (title cj/--webclip-title)) (if (not url) (error "No URL provided for clipping") (condition-case err diff --git a/tests/test-org-webclipper-commands.el b/tests/test-org-webclipper-commands.el index 57c7b5fc..5ec5fbfe 100644 --- a/tests/test-org-webclipper-commands.el +++ b/tests/test-org-webclipper-commands.el @@ -65,62 +65,74 @@ that registers the webclip entry. Providing `'org-protocol' fires the block." ;;; cj/org-protocol-webclip -(ert-deftest test-webclipper-protocol-stores-url-title-and-captures () - "Normal: the protocol handler stores url+title and triggers `org-capture'." +(ert-deftest test-webclipper-protocol-binds-url-title-during-capture () + "Normal: the protocol handler makes url+title visible to the capture call." (let ((cj/webclipper-initialized t) - (cj/webclip-current-url nil) - (cj/webclip-current-title nil) - (capture-key nil)) + (seen-url nil) (seen-title nil) (capture-key nil)) (cl-letf (((symbol-function 'org-capture) - (lambda (_arg k) (setq capture-key k)))) + (lambda (_arg k) + (setq capture-key k + seen-url cj/--webclip-url + seen-title cj/--webclip-title)))) (cj/org-protocol-webclip '(:url "https://example.com" :title "Hello"))) - (should (equal cj/webclip-current-url "https://example.com")) - (should (equal cj/webclip-current-title "Hello")) + (should (equal seen-url "https://example.com")) + (should (equal seen-title "Hello")) (should (equal capture-key "W")))) +(ert-deftest test-webclipper-protocol-leaves-no-stale-state () + "Boundary: after the protocol capture returns, no url/title remains bound." + (let ((cj/webclipper-initialized t)) + (cl-letf (((symbol-function 'org-capture) #'ignore)) + (cj/org-protocol-webclip '(:url "https://example.com" :title "Hello"))) + (should (null cj/--webclip-url)) + (should (null cj/--webclip-title)))) + +(ert-deftest test-webclipper-protocol-aborted-capture-clears-state () + "Error: a capture that errors mid-flow still leaves no stale url/title." + (let ((cj/webclipper-initialized t)) + (cl-letf (((symbol-function 'org-capture) + (lambda (&rest _) (error "simulated capture abort")))) + (ignore-errors (cj/org-protocol-webclip '(:url "https://example.com")))) + (should (null cj/--webclip-url)) + (should (null cj/--webclip-title)))) + (ert-deftest test-webclipper-protocol-defaults-title-when-missing () - "Boundary: a missing title in INFO becomes \"Untitled\"." + "Boundary: a missing title in INFO becomes \"Untitled\" during the capture." (let ((cj/webclipper-initialized t) - (cj/webclip-current-url nil) - (cj/webclip-current-title nil)) - (cl-letf (((symbol-function 'org-capture) #'ignore)) + (seen-title nil)) + (cl-letf (((symbol-function 'org-capture) + (lambda (&rest _) (setq seen-title cj/--webclip-title)))) (cj/org-protocol-webclip '(:url "https://x.test"))) - (should (equal cj/webclip-current-title "Untitled")))) + (should (equal seen-title "Untitled")))) ;;; cj/org-protocol-webclip-handler (ert-deftest test-webclipper-protocol-handler-errors-when-no-url () - "Error: handler with no stashed url signals an error." - (let ((cj/webclip-current-url nil) - (cj/webclip-current-title "Whatever")) - (cl-letf (((symbol-function 'require) (lambda (&rest _) t)) - ((symbol-function 'setopt) (lambda (&rest _) nil))) + "Error: handler with no bound url signals an error." + (let ((cj/--webclip-url nil) + (cj/--webclip-title "Whatever")) + (cl-letf (((symbol-function 'require) (lambda (&rest _) t))) (should-error (cj/org-protocol-webclip-handler) :type 'error)))) (ert-deftest test-webclipper-protocol-handler-returns-processed-content () - "Normal: handler converts the stashed URL into processed org content." - (let ((cj/webclip-current-url "https://example.com") - (cj/webclip-current-title "Title")) + "Normal: handler converts the bound URL into processed org content." + (let ((cj/--webclip-url "https://example.com") + (cj/--webclip-title "Title")) (cl-letf (((symbol-function 'require) (lambda (&rest _) t)) - ((symbol-function 'setopt) (lambda (&rest _) nil)) ((symbol-function 'org-web-tools--url-as-readable-org) (lambda (_) "* Page Title\n** Sub heading\nBody.\n")) ((symbol-function 'message) #'ignore)) (let ((out (cj/org-protocol-webclip-handler))) ;; The first H1 is stripped, sub-heading is demoted. (should (string-match-p "^\\*\\*\\* Sub heading" out)) - (should (string-match-p "Body" out))) - ;; Stash is cleared. - (should (null cj/webclip-current-url)) - (should (null cj/webclip-current-title))))) + (should (string-match-p "Body" out)))))) (ert-deftest test-webclipper-protocol-handler-wraps-fetch-error () "Error: a fetch failure is wrapped in a clear error message." - (let ((cj/webclip-current-url "https://example.com") - (cj/webclip-current-title "Title")) + (let ((cj/--webclip-url "https://example.com") + (cj/--webclip-title "Title")) (cl-letf (((symbol-function 'require) (lambda (&rest _) t)) - ((symbol-function 'setopt) (lambda (&rest _) nil)) ((symbol-function 'org-web-tools--url-as-readable-org) (lambda (_) (error "network down")))) (let ((err (should-error (cj/org-protocol-webclip-handler) :type 'error))) |