* Re: Recent folding issues
2022-07-12 6:17 ` Jack Kamm
@ 2022-07-12 13:58 ` Ihor Radchenko
2022-07-13 14:50 ` Jack Kamm
0 siblings, 1 reply; 9+ messages in thread
From: Ihor Radchenko @ 2022-07-12 13:58 UTC (permalink / raw)
To: Jack Kamm; +Cc: emacs-orgmode
[-- Attachment #1: Type: text/plain, Size: 387 bytes --]
Jack Kamm <jackkamm@gmail.com> writes:
>> I cannot reproduce. Please, update your Org to the latest version, try
>> to reproduce, and provide the detailed steps required to obtain the
>> confusing behaviour you are seeing.
>
> I attach a couple files for a minimal reproducible example of both
> issues.
>
> Steps to reproduce:
Thanks!
Can you try the attached patch set?
Best,
Ihor
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Revert-org.el-Improve-performance-of-deletion-comman.patch --]
[-- Type: text/x-patch, Size: 3995 bytes --]
From a79a742cbff01b7815cffba806c28ea45a4da63c Mon Sep 17 00:00:00 2001
Message-Id: <a79a742cbff01b7815cffba806c28ea45a4da63c.1657634233.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Tue, 12 Jul 2022 15:05:45 +0800
Subject: [PATCH 1/3] Revert "org.el: Improve performance of deletion commands"
This reverts commit 46df681336c83c826b367d2803f59560165bdeba.
The optimization broke folding fragility checks.
---
lisp/org.el | 74 ++++++++++++++++++++++++++---------------------------
1 file changed, 36 insertions(+), 38 deletions(-)
diff --git a/lisp/org.el b/lisp/org.el
index 3d4de5b4f..d85b5818e 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -16156,19 +16156,18 @@ (defun org-delete-backward-char (N)
still be marked for re-alignment if the field did fill the entire column,
because, in this case the deletion might narrow the column."
(interactive "p")
- (org-fold-core-ignore-modifications
- (save-match-data
- (org-fold-check-before-invisible-edit 'delete-backward)
- (if (and (= N 1)
- (not overwrite-mode)
- (not (org-region-active-p))
- (not (eq (char-before) ?|))
- (save-excursion (skip-chars-backward " \t") (not (bolp)))
- (looking-at-p ".*?|")
- (org-at-table-p))
- (progn (forward-char -1) (org-delete-char 1))
- (backward-delete-char N)
- (org-fix-tags-on-the-fly)))))
+ (save-match-data
+ (org-fold-check-before-invisible-edit 'delete-backward)
+ (if (and (= N 1)
+ (not overwrite-mode)
+ (not (org-region-active-p))
+ (not (eq (char-before) ?|))
+ (save-excursion (skip-chars-backward " \t") (not (bolp)))
+ (looking-at-p ".*?|")
+ (org-at-table-p))
+ (progn (forward-char -1) (org-delete-char 1))
+ (backward-delete-char N)
+ (org-fix-tags-on-the-fly))))
(defun org-delete-char (N)
"Like `delete-char', but insert whitespace at field end in tables.
@@ -16177,31 +16176,30 @@ (defun org-delete-char (N)
still be marked for re-alignment if the field did fill the entire column,
because, in this case the deletion might narrow the column."
(interactive "p")
- (org-fold-core-ignore-modifications
- (save-match-data
- (org-fold-check-before-invisible-edit 'delete)
- (cond
- ((or (/= N 1)
- (eq (char-after) ?|)
- (save-excursion (skip-chars-backward " \t") (bolp))
- (not (org-at-table-p)))
- (delete-char N)
- (org-fix-tags-on-the-fly))
- ((looking-at ".\\(.*?\\)|")
- (let* ((update? org-table-may-need-update)
- (noalign (looking-at-p ".*? |")))
- (delete-char 1)
- (org-table-with-shrunk-field
- (save-excursion
- ;; Last space is `org-table-separator-space', so insert
- ;; a regular one before it instead.
- (goto-char (- (match-end 0) 2))
- (insert " ")))
- ;; If there were two spaces at the end, this field does not
- ;; determine the width of the column.
- (when noalign (setq org-table-may-need-update update?))))
- (t
- (delete-char N))))))
+ (save-match-data
+ (org-fold-check-before-invisible-edit 'delete)
+ (cond
+ ((or (/= N 1)
+ (eq (char-after) ?|)
+ (save-excursion (skip-chars-backward " \t") (bolp))
+ (not (org-at-table-p)))
+ (delete-char N)
+ (org-fix-tags-on-the-fly))
+ ((looking-at ".\\(.*?\\)|")
+ (let* ((update? org-table-may-need-update)
+ (noalign (looking-at-p ".*? |")))
+ (delete-char 1)
+ (org-table-with-shrunk-field
+ (save-excursion
+ ;; Last space is `org-table-separator-space', so insert
+ ;; a regular one before it instead.
+ (goto-char (- (match-end 0) 2))
+ (insert " ")))
+ ;; If there were two spaces at the end, this field does not
+ ;; determine the width of the column.
+ (when noalign (setq org-table-may-need-update update?))))
+ (t
+ (delete-char N)))))
;; Make `delete-selection-mode' work with Org mode and Orgtbl mode
(put 'org-self-insert-command 'delete-selection
--
2.35.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-test-org-fold-Cover-the-issue-fixed-by-previous-comm.patch --]
[-- Type: text/x-patch, Size: 1284 bytes --]
From f29ccf3161a51f700bb3375a16acab563baceb49 Mon Sep 17 00:00:00 2001
Message-Id: <f29ccf3161a51f700bb3375a16acab563baceb49.1657634233.git.yantar92@gmail.com>
In-Reply-To: <a79a742cbff01b7815cffba806c28ea45a4da63c.1657634233.git.yantar92@gmail.com>
References: <a79a742cbff01b7815cffba806c28ea45a4da63c.1657634233.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Tue, 12 Jul 2022 21:30:58 +0800
Subject: [PATCH 2/3] test-org-fold: Cover the issue fixed by previous commit
*
testing/lisp/test-org-fold.el (test-org-fold/org-fold-reveal-broken-structure):
Test `org-delete-char' instead of `delete-char'. The former is the
function used interactively.
---
testing/lisp/test-org-fold.el | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/testing/lisp/test-org-fold.el b/testing/lisp/test-org-fold.el
index 40afe55ae..a26346175 100644
--- a/testing/lisp/test-org-fold.el
+++ b/testing/lisp/test-org-fold.el
@@ -410,7 +410,7 @@ (ert-deftest test-org-fold/org-fold-reveal-broken-structure ()
(re-search-forward "Text")
(should (org-invisible-p))
(goto-char 1)
- (delete-char 1)
+ (org-delete-char 1)
(re-search-forward "Text")
(should-not (org-invisible-p)))
(org-test-with-temp-text
--
2.35.1
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-org-fold-Make-sure-that-changes-do-not-leave-partial.patch --]
[-- Type: text/x-patch, Size: 10403 bytes --]
From 8e2fed82ed9335680f6b5a7d02028786d8479ca3 Mon Sep 17 00:00:00 2001
Message-Id: <8e2fed82ed9335680f6b5a7d02028786d8479ca3.1657634233.git.yantar92@gmail.com>
In-Reply-To: <a79a742cbff01b7815cffba806c28ea45a4da63c.1657634233.git.yantar92@gmail.com>
References: <a79a742cbff01b7815cffba806c28ea45a4da63c.1657634233.git.yantar92@gmail.com>
From: Ihor Radchenko <yantar92@gmail.com>
Date: Tue, 12 Jul 2022 21:32:08 +0800
Subject: [PATCH 3/3] org-fold: Make sure that changes do not leave partially
folded headings
* lisp/org-fold-core.el (org-fold-core--fix-folded-region): Check
fragility in all the indirect buffers.
* lisp/org-fold.el (org-fold--reveal-headline-at-point): New function
revealing heading line or full heading when the heading only contains
blank lines.
(org-fold--reveal-outline-maybe): Check headline at the end of folded
region as well. Use `org-fold-core--fix-folded-region'.
* lisp/org.el (org-insert-heading): Make sure that the newline in
front of heading is revealed.
---
lisp/org-fold-core.el | 71 ++++++++++++++++++++++---------------------
lisp/org-fold.el | 52 ++++++++++++++++++++++---------
lisp/org.el | 5 ++-
3 files changed, 77 insertions(+), 51 deletions(-)
diff --git a/lisp/org-fold-core.el b/lisp/org-fold-core.el
index 23507967f..3fcacb975 100644
--- a/lisp/org-fold-core.el
+++ b/lisp/org-fold-core.el
@@ -1269,41 +1269,42 @@ (defun org-fold-core--fix-folded-region (from to _)
(let ((new-region (funcall func from to)))
(setq from (car new-region))
(setq to (cdr new-region))))
- (dolist (spec (org-fold-core-folding-spec-list))
- ;; No action is needed when :fragile is nil for the spec.
- (when (org-fold-core-get-folding-spec-property spec :fragile)
- (org-with-wide-buffer
- ;; Expand the considered region to include partially present fold.
- ;; Note: It is important to do this inside loop over all
- ;; specs. Otherwise, the region may be expanded to huge
- ;; outline fold, potentially involving majority of the
- ;; buffer. That would cause the below code to loop over
- ;; almost all the folds in buffer, which would be too slow.
- (let ((local-from from)
- (local-to to)
- (region-from (org-fold-core-get-region-at-point spec (max (point-min) (1- from))))
- (region-to (org-fold-core-get-region-at-point spec (min to (1- (point-max))))))
- (when region-from (setq local-from (car region-from)))
- (when region-to (setq local-to (cdr region-to)))
- (let ((pos local-from))
- ;; Move to the first hidden region.
- (unless (org-fold-core-get-folding-spec spec pos)
- (setq pos (org-fold-core-next-folding-state-change spec pos local-to)))
- ;; Cycle over all the folds.
- (while (< pos local-to)
- (save-match-data ; we should not clobber match-data in after-change-functions
- (let ((fold-begin (and (org-fold-core-get-folding-spec spec pos)
- pos))
- (fold-end (org-fold-core-next-folding-state-change spec pos local-to)))
- (when (and fold-begin fold-end)
- (when (save-excursion
- (funcall (org-fold-core-get-folding-spec-property spec :fragile)
- (cons fold-begin fold-end)
- spec))
- ;; Reveal completely, not just from the SPEC.
- (org-fold-core-region fold-begin fold-end nil)))))
- ;; Move to next fold.
- (setq pos (org-fold-core-next-folding-state-change spec pos local-to))))))))))))
+ (org-fold-core-cycle-over-indirect-buffers
+ (dolist (spec (org-fold-core-folding-spec-list))
+ ;; No action is needed when :fragile is nil for the spec.
+ (when (org-fold-core-get-folding-spec-property spec :fragile)
+ (org-with-wide-buffer
+ ;; Expand the considered region to include partially present fold.
+ ;; Note: It is important to do this inside loop over all
+ ;; specs. Otherwise, the region may be expanded to huge
+ ;; outline fold, potentially involving majority of the
+ ;; buffer. That would cause the below code to loop over
+ ;; almost all the folds in buffer, which would be too slow.
+ (let ((local-from from)
+ (local-to to)
+ (region-from (org-fold-core-get-region-at-point spec (max (point-min) (1- from))))
+ (region-to (org-fold-core-get-region-at-point spec (min to (1- (point-max))))))
+ (when region-from (setq local-from (car region-from)))
+ (when region-to (setq local-to (cdr region-to)))
+ (let ((pos local-from))
+ ;; Move to the first hidden region.
+ (unless (org-fold-core-get-folding-spec spec pos)
+ (setq pos (org-fold-core-next-folding-state-change spec pos local-to)))
+ ;; Cycle over all the folds.
+ (while (< pos local-to)
+ (save-match-data ; we should not clobber match-data in after-change-functions
+ (let ((fold-begin (and (org-fold-core-get-folding-spec spec pos)
+ pos))
+ (fold-end (org-fold-core-next-folding-state-change spec pos local-to)))
+ (when (and fold-begin fold-end)
+ (when (save-excursion
+ (funcall (org-fold-core-get-folding-spec-property spec :fragile)
+ (cons fold-begin fold-end)
+ spec))
+ ;; Reveal completely, not just from the SPEC.
+ (org-fold-core-region fold-begin fold-end nil)))))
+ ;; Move to next fold.
+ (setq pos (org-fold-core-next-folding-state-change spec pos local-to)))))))))))))
;;; Handling killing/yanking of folded text
diff --git a/lisp/org-fold.el b/lisp/org-fold.el
index ce8afd9b4..6ff21dfc7 100644
--- a/lisp/org-fold.el
+++ b/lisp/org-fold.el
@@ -55,6 +55,7 @@ (defvar org-link-descriptive)
(defvar org-outline-regexp-bol)
(defvar org-archive-tag)
(defvar org-custom-properties-overlays)
+(defvar org-element-headline-re)
(declare-function isearch-filter-visible "isearch" (beg end))
(declare-function org-element-type "org-element" (element))
@@ -930,6 +931,30 @@ (defun org-fold--extend-changed-region (from to)
(setq from (save-excursion (goto-char from) (line-beginning-position 0)))
(cons from to))
+(defun org-fold--reveal-headline-at-point ()
+ "Reveal header line and empty contents inside.
+Reveal the header line and, if present, also reveal its contents, when
+the contents consists of blank lines.
+
+Assume that point is located at the header line."
+ (org-with-wide-buffer
+ (beginning-of-line)
+ (org-fold-region
+ (max (point-min) (1- (point)))
+ (let ((endl (line-end-position)))
+ (save-excursion
+ (goto-char endl)
+ (skip-chars-forward "\n\t\r ")
+ ;; Unfold blank lines after newly inserted headline.
+ (if (equal (point)
+ (save-excursion
+ (goto-char endl)
+ (org-end-of-subtree)
+ (skip-chars-forward "\n\t\r ")))
+ (point)
+ endl)))
+ nil 'headline)))
+
(defun org-fold--reveal-outline-maybe (region _)
"Reveal folded outline in REGION when needed.
@@ -942,28 +967,25 @@ (defun org-fold--reveal-outline-maybe (region _)
;; headline or a list item.
(backward-char)
(beginning-of-line)
- ;; Make sure that headline is not partially hidden
+ ;; Make sure that headline is not partially hidden.
(unless (org-fold-folded-p nil 'headline)
- (org-fold-region
- (max (point-min) (1- (point)))
- (let ((endl (line-end-position)))
- (save-excursion
- (goto-char endl)
- (skip-chars-forward "\n\t\r ")
- ;; Unfold blank lines.
- (if (or (and (looking-at-p "\\*")
- (> (point) (1+ endl)))
- (eq (point) (point-max)))
- (point)
- endl)))
- nil 'headline))
+ (org-fold--reveal-headline-at-point))
;; Never hide level 1 headlines
(save-excursion
(goto-char (line-end-position))
(unless (>= (point) (cdr region))
(when (re-search-forward (rx bol "* ") (cdr region) t)
- (org-fold-region (match-beginning 0) (line-end-position) nil 'headline))))
+ (org-fold--reveal-headline-at-point))))
+ ;; Make sure that headline after is not partially hidden.
+ (goto-char (cdr region))
+ (beginning-of-line)
+ (unless (org-fold-folded-p nil 'headline)
+ (when (looking-at-p org-element-headline-re)
+ (org-fold--reveal-headline-at-point)))
;; Check the validity of headline
+ (goto-char (car region))
+ (backward-char)
+ (beginning-of-line)
(unless (let ((case-fold-search t))
(looking-at (rx-to-string
`(or (regex ,(org-item-re))
diff --git a/lisp/org.el b/lisp/org.el
index d85b5818e..1c9eaf09a 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -6141,7 +6141,10 @@ (defun org-insert-heading (&optional arg invisible-ok top)
(unless invisible-ok
(if (eq org-fold-core-style 'text-properties)
(cond
- ((org-fold-folded-p (line-beginning-position) 'headline)
+ ((org-fold-folded-p
+ (max (point-min)
+ (1- (line-beginning-position)))
+ 'headline)
(org-fold-region (line-end-position 0) (line-end-position) nil 'headline))
(t nil))
(pcase (get-char-property-and-overlay (point) 'invisible)
--
2.35.1
^ permalink raw reply related [flat|nested] 9+ messages in thread