Fix quirks in `org-insert-structure-template'

* lisp/org.el (org-insert-structure-template): Fix some issues.
  Refactor code.
* testing/lisp/test-org.el (test-org/insert-structure-template):
  Update tests.

Reported-by: stardiviner <numbchild@gmail.com>
<http://lists.gnu.org/r/emacs-orgmode/2018-05/msg00553.html>

2 files changed, 42 insertions, 67 deletions

diff --git a/lisp/org.el b/lisp/org.el
index 672ce15..d2b4c26 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -11840,7 +11840,7 @@ If an element cannot be made unique, an error is raised."
 		     :key (lambda (elm) (cl-position (cdr elm) keys))))))
 
 (defun org-insert-structure-template (type)
-    "Insert a block structure of the type #+begin_foo/#+end_foo.
+  "Insert a block structure of the type #+begin_foo/#+end_foo.
 Select a block from `org-structure-template-alist' then type
 either RET, TAB or SPC to write the block type.  With an active
 region, wrap the region in the block.  Otherwise, insert an empty
@@ -11849,62 +11849,36 @@ block."
    (list (pcase (org--insert-structure-template-mks)
 	   (`("\t" . ,_) (read-string "Structure type: "))
 	   (`(,_ ,choice . ,_) choice))))
-  (let* ((region? (use-region-p))
-	 (col (current-indentation))
-	 (indent (make-string col ?\s))
-	 (special? (string-match-p "\\(src\\|export\\)\\'" type))
-	 (region-string (and region?
-			     (buffer-substring (region-beginning)
-					       (region-end))))
-	 (region-end-blank (and region?
-				(save-excursion
-				  (goto-char (region-end))
-				  (when (looking-at "[ \t]*$")
-				    (replace-match "")
-				    t))))
-	 s)
-    (when region? (delete-region (region-beginning) (region-end)))
-    (unless (save-excursion (skip-chars-backward "[ \t]") (bolp))
+  (let* ((column (current-indentation))
+	 (region? (use-region-p))
+	 (region-start (and region? (region-beginning)))
+	 (region-end (and region? (copy-marker (region-end))))
+	 (special? (string-match-p "\\`\\(src\\|export\\)\\'" type)))
+    (when region? (goto-char region-start))
+    (if (save-excursion (skip-chars-backward " \t") (bolp))
+	(beginning-of-line)
       (insert "\n"))
-    (beginning-of-line)
     (save-excursion
-      (insert
-       (with-temp-buffer
-	 (when region?
-	   (insert region-string "\n")
-	   (when (string-match-p
-		  (concat "\\`" (regexp-opt '("example" "export" "src")))
-		  type)
-	     (org-escape-code-in-region (point-min) (point-max))))
-	 (goto-char (point-min))
-	 ;; Delete trailing white-lines.
-	 (when region?
-	   (while (looking-at-p "^[ \t]*$")
-	     (delete-region (line-beginning-position)
-			    (line-beginning-position 2))))
-	 (save-excursion
-	   (while (not (eobp))
-	     (unless (looking-at-p indent)
-	       (insert indent))
-	     (forward-line)))
-	 (insert
-	  indent
-	  (format "#+begin_%s%s\n" type (if special? " " "")))
-	 (unless region? (indent-to col))
-	 (setq s (point))
-	 (goto-char (point-max))
-	 (skip-chars-backward "[ \t\n]" s)
-	 (delete-region (line-end-position) (point-max))
-	 (insert "\n" indent
-		 (format "#+end_%s" (car (split-string type)))
-		 (if region-end-blank "" "\n"))
-	 (buffer-substring (point-min) (point))))
+      (indent-to column)
+      (insert (format "#+begin_%s%s\n" type (if special? " " "")))
+      (when region?
+	(when (string-match-p (concat "\\`"
+				      (regexp-opt '("example" "export" "src")))
+			      type)
+	  (org-escape-code-in-region (point) region-end))
+	(goto-char region-end)
+	;; Ignore empty lines at the end of the region.
+	(skip-chars-backward " \r\t\n")
+	(end-of-line))
+      (unless (bolp) (insert "\n"))
+      (indent-to column)
+      (insert (format "#+end_%s" (car (split-string type))))
+      (if (looking-at "[ \t]*$") (replace-match "")
+	(insert "\n"))
       (when (and (eobp) (not (bolp))) (insert "\n")))
-    (cond (special?
-	   (end-of-line))
-	  (t
-	   (forward-line)
-	   (skip-chars-forward "[ \t]*")))))
+    (if special? (end-of-line)
+      (forward-line)
+      (skip-chars-forward " \t"))))
 
 
 ;;;; TODO, DEADLINE, Comments
diff --git a/testing/lisp/test-org.el b/testing/lisp/test-org.el
index dc0becb..ed9da2f 100644
--- a/testing/lisp/test-org.el
+++ b/testing/lisp/test-org.el
@@ -4060,18 +4060,19 @@ Text.
        (org-next-block 1 nil "^[ \t]*#\\+BEGIN_QUOTE")
        (looking-at "#\\+begin_quote")))))
 
-(ert-deftest test-org/insert-template ()
+(ert-deftest test-org/insert-structure-template ()
   "Test `org-insert-structure-template'."
   ;; Test in empty buffer.
   (should
-   (string= "#+begin_foo\n\n#+end_foo\n"
+   (string= "#+begin_foo\n#+end_foo\n"
 	    (org-test-with-temp-text ""
 	      (org-insert-structure-template "foo")
 	      (buffer-string))))
   ;; Test with multiple lines in buffer.
   (should
-   (string= "#+begin_foo\nI'm a paragraph\n#+end_foo\nI'm a second paragraph"
+   (string= "#+begin_foo\nI'm a paragraph\n#+end_foo\n\nI'm a second paragraph"
 	    (org-test-with-temp-text "I'm a paragraph\n\nI'm a second paragraph"
+	      (transient-mark-mode 1)
 	      (org-mark-element)
 	      (org-insert-structure-template "foo")
 	      (buffer-string))))
@@ -4079,12 +4080,12 @@ Text.
   (should
    (string= "#+begin_foo\nI'm a paragraph\n#+end_foo\n\nI'm a second paragraph"
 	    (org-test-with-temp-text "I'm a paragraph\n\nI'm a second paragraph"
+	      (transient-mark-mode 1)
 	      (set-mark (point-min))
 	      (end-of-line)
-	      (activate-mark)
 	      (org-insert-structure-template "foo")
 	      (buffer-string))))
-  ;; Middle of paragraph
+  ;; Middle of paragraph.
   (should
    (string= "p1\n#+begin_foo\np2\n#+end_foo\np3"
 	    (org-test-with-temp-text "p1\n<point>p2\np3"
@@ -4124,15 +4125,15 @@ Text.
 	      (buffer-string))))
   ;; Test point location.
   (should
-   (eq (length "#\\+begin_foo\n")
-       (org-test-with-temp-text ""
-	 (org-insert-structure-template "foo")
-	 (point))))
+   (string= "#+begin_foo\n"
+	    (org-test-with-temp-text ""
+	      (org-insert-structure-template "foo")
+	      (buffer-substring (point-min) (point)))))
   (should
-   (eq (length "#\\+begin_src ")
-       (org-test-with-temp-text ""
-	 (org-insert-structure-template "src")
-	 (point)))))
+   (string= "#+begin_src "
+	    (org-test-with-temp-text ""
+	      (org-insert-structure-template "src")
+	      (buffer-substring (point-min) (point))))))
 
 (ert-deftest test-org/previous-block ()
   "Test `org-previous-block' specifications."