From mboxrd@z Thu Jan 1 00:00:00 1970 From: Carsten Dominik Subject: Re: Outline and org-mode don't insert text into folded sections logically Date: Wed, 2 Nov 2011 08:12:05 +0100 Message-ID: References: <1315077299.1785.YahooMailClassic@web121520.mail.ne1.yahoo.com> <837h5nh1df.fsf@yahoo.it> <877h3xibb0.fsf@gnu.org> <81ehy5uy0t.fsf@gmail.com> <87wrbxdter.fsf@gnu.org> <877h3n7usn.fsf@gnu.org> <87hb2ru7qf.fsf@gnu.org> <87vcr7srij.fsf@gnu.org> <87k47ns2vx.fsf@gnu.org> <6CBBC924-DD81-46F0-B8ED-EC92D9B6A2AE@gmail.com> <87ehxuzw80.fsf@gnu.org> Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: multipart/mixed; boundary=Apple-Mail-1-460825491 Return-path: Received: from eggs.gnu.org ([140.186.70.92]:47543) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLUzb-0006kN-9e for emacs-orgmode@gnu.org; Wed, 02 Nov 2011 03:12:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RLUza-0001CY-5h for emacs-orgmode@gnu.org; Wed, 02 Nov 2011 03:12:11 -0400 Received: from mail-ey0-f169.google.com ([209.85.215.169]:47359) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RLUzZ-0001CL-Qk for emacs-orgmode@gnu.org; Wed, 02 Nov 2011 03:12:10 -0400 Received: by eye4 with SMTP id 4so7510483eye.0 for ; Wed, 02 Nov 2011 00:12:08 -0700 (PDT) In-Reply-To: <87ehxuzw80.fsf@gnu.org> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Bastien Cc: emacs-orgmode@gnu.org, Kelly Dean --Apple-Mail-1-460825491 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=us-ascii Hi Bastien, please find enclosed a patch which is my proposal for rounding off this feature. It introduces a variable, applies the check to org-delete-char and org-delete-backward-char, and removes the (sit-for 1), and it never moves the cursor. I have been unsing it for a day or two, and I like the `smart' setting of the variable. Even though, I do agree with your earlier post that the default should be nil. - Carsten --Apple-Mail-1-460825491 Content-Disposition: attachment; filename=checking-for-invisible-edits.patch Content-Type: application/octet-stream; x-unix-mode=0644; name="checking-for-invisible-edits.patch" Content-Transfer-Encoding: 7bit diff --git a/lisp/org.el b/lisp/org.el index 318ccfd..28b9f1c 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -1065,6 +1065,28 @@ OK to kill that hidden subtree. When nil, kill without remorse." (const :tag "Protect hidden subtrees with a security query" t) (const :tag "Never kill a hidden subtree with C-k" error))) +(defcustom org-catch-invisible-edits nil + "Check if in invisible region before inserting or deleting a character. +Valid values are: + +nil Do not check, so just do invisible edits. +error Throw an error and do nothing. +show Make point visible, and do the requested edit. +show-and-error Make point visible, then throw an error and abort the edit. +smart Make point visible, and do insertion/deletion if it is + adjacent to visible text and the change feels predictable. + Never delete a previously invisible character or add in the + middle or right after an invisible region. Basically, this + allows insertion and backward-delete right before ellipses. + FIXME: maybe in this case we should not even show?" + :group 'org-edit-structure + :type '(choice + (const :tag "Do not check" nil) + (const :tag "Throw error when trying to edit" error) + (const :tag "Unhide, but do not do the edit" show-and-error) + (const :tag "Show invisible part and do the edit" show) + (const :tag "Be smart and do the right thing" smart))) + (defcustom org-yank-folded-subtrees t "Non-nil means when yanking subtrees, fold them. If the kill is a single subtree, or a sequence of subtrees, i.e. if @@ -17401,24 +17423,7 @@ hook. The default setting is `org-speed-command-default-hook'." If the cursor is in a table looking at whitespace, the whitespace is overwritten, and the table is not marked as requiring realignment." (interactive "p") - (let ((invisible-at-point - (car (get-char-property-and-overlay (point) 'invisible))) - (invisible-before-point - (or (bobp) (car (get-char-property-and-overlay - (1- (point)) 'invisible))))) - (when (or (eq invisible-at-point 'outline) - (eq invisible-at-point 'org-hide-block) - (eq invisible-before-point 'outline) - (eq invisible-before-point 'org-hide-block)) - (if (or (eq invisible-before-point 'outline) - (eq invisible-before-point 'org-hide-block)) - (goto-char (previous-overlay-change (point)))) - (org-cycle) - (if (or (eq invisible-before-point 'outline) - (eq invisible-before-point 'org-hide-block)) - (forward-char 1)) - (message "Unfolding invisible region around point before editing") - (sit-for 1))) + (org-check-before-invisible-edit 'insert) (cond ((and org-use-speed-commands (setq org-speed-command @@ -17470,6 +17475,53 @@ overwritten, and the table is not marked as requiring realignment." (setq org-self-insert-command-undo-counter (1+ org-self-insert-command-undo-counter)))))))) +(defun org-check-before-invisible-edit (kind) + "Check is editing if kind KIND would be dangerous with invisible text around. +The detailed reaction depends on the user option `org-catch-invisible-edits'." + ;; First, try to get out of here as quickly as possible, to reduce overhead + (if (and org-catch-invisible-edits + (or (not (boundp 'visible-mode)) (not visible-mode)) + (or (get-char-property (point) 'invisible) + (get-char-property (max (point-min) (1- (point))) 'invisible))) + ;; OK, we need to take a closer look + (let ((invisible-at-point (get-char-property (point) 'invisible)) + (invisible-before-point (if (bobp) nil (get-char-property + (1- (point)) 'invisible))) + (border-and-ok-direction + (or + ;; Check if we are acting predictably before invisible text + (and invisible-at-point (not invisible-before-point) + (memq kind '(insert delete-backward))) + ;; Check if we are acting predictably after invisible text + ;; This works not well, and I have turned it off. It seems + ;; better to always show and stop after invisible text. + ;; (and (not invisible-at-point) invisible-before-point + ;; (memq kind '(insert delete))) + ))) + + (when (or (memq invisible-at-point '(outline org-hide-block)) + (memq invisible-before-point '(outline org-hide-block))) + (if (eq org-catch-invisible-edits 'error) + (error "Editing in invisible areas is prohibited - make visible first")) + ;; Make the area visible + (save-excursion + (if invisible-before-point + (goto-char (previous-single-char-property-change + (point) 'invisible))) + (org-cycle)) + (cond + ((eq org-catch-invisible-edits 'show) + ;; That's it, we do the edit after showing + (message + "Unfolding invisible region around point before editing") + (sit-for 1)) + ((and (eq org-catch-invisible-edits 'smart) + border-and-ok-direction) + (message "Unfolding invisible region around point before editing")) + (t + ;; Don't do the edit, make the user repeat it in full visibility + (error "Edit in invisible region aborted, repeat to confirm with text visible"))))))) + (defun org-fix-tags-on-the-fly () (when (and (equal (char-after (point-at-bol)) ?*) (org-on-heading-p)) @@ -17482,6 +17534,7 @@ front of the next \"|\" separator, to keep the table aligned. The table will 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-check-before-invisible-edit 'delete-backward) (if (and (org-table-p) (eq N 1) (string-match "|" (buffer-substring (point-at-bol) (point))) @@ -17508,6 +17561,7 @@ front of the next \"|\" separator, to keep the table aligned. The table will 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-check-before-invisible-edit 'delete) (if (and (org-table-p) (not (bolp)) (not (= (char-after) ?|)) --Apple-Mail-1-460825491 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=us-ascii On 30.10.2011, at 10:04, Bastien wrote: > Hi Carsten, > > Carsten Dominik writes: > >> - This patch covers only one of many ways to make unwanted changes >> in an invisible area. Others would be delete, backspace, >> kill-region, yank, kill-line, and an arbitrarily long list of >> less obvious other commands. Full protection could only be >> done with pre-change-hooks or so, but would then prevent >> also programmed changes - something that would not be useful. > > Yes, I don't want programmed changes to be affected by this feature. > > But having such a warning for `org-delete' would also be useful IMHO. > >> `org-self-insert-command' is probably only ever used in an >> interactive way, so the patch as you have written it may very >> well function correctly. >> >> - All the code in org-self-insert-command is executed for each >> keypress, so one needs to be careful to have this function >> carry as little overhead as possible. > > I actually think there should be a user option > `org-edit-invisible-send-warning' defaulting to nil. > > The request "don't let me shoot in my foot" is a common > one, and this option would let people set this to `t'. > >> - Currently this chokes at the beginning of the buffer because >> the invisibility test is also run at (1- (point)). > > Fixed, thanks. > >> - I am not sure if I understand the positioning code: >>> (if (or (eq invisible-before-point 'outline) >>> (eq invisible-before-point 'org-hide-block)) >>> (goto-char (previous-overlay-change (point)))) >>> (org-cycle) >>> (if (or (eq invisible-before-point 'outline) >>> (eq invisible-before-point 'org-hide-block)) >>> (forward-char 1)) >> >> So when I happen to be somewhere in the middle of invisible >> text and press a character, it seems to me that the character >> will be inserted at the beginning of the invisible text, and >> not where the cursor was. >> >> Maybe a better solution would be to save point, unfold, >> go back to point, throw and error and not insert the pressed >> character. I am not sure, though. > > Throwing an error and not inserting the text was what my first > patch did. I thought it was too restrictive, though. > > With an option `org-edit-invisible-send-warning', we could have both: > `t' would just throw a warning, 'prevent would throw an error. > >> Maybe you can explain your reasoning? > > My reasoning is that, when in the "middle" of an invisible region, > the user does not know where the point is, hence he doesn't really > know where he wants to insert characters. In this case, I assume > insert at the beginning of the invisible region is a reasonable > default. > > Thanks for the feedback -- let's continue brainstorming, I think > this feature is important. > > Best, > > -- > Bastien --Apple-Mail-1-460825491--