Ihor Radchenko <[email protected]> writes:
> "J.D. Smith" <[email protected]> writes:
>
>> At least for the case of a bare subscript, there is "something" hidden
>> (the `_'/`^' chars), and therefore there is an inside/outside
>> distinction at one end (the front of the super/sub-scripted text).
>
> Makes sense.
> But "inside" is not defined in that case wither.
"half inside" :).
>> BTW, have you had a look at the manual entries? I just cleaned that up
>> a bit.
>
> Sure. Let me look. Several minor comments below.
>
>> :DESCRIPTION: When in doubt, press @kbd{C-c C-c}.
>> +:ID: ACEBFD46-09A6-43E5-A868-A4B3CF1F56AF
>
> Stray ID :)
>> +The visibility of hidden text can always toggled using {{{kbd(C-c
>
> * be always toggled
> or
> * can be toggled
>
>> +inside an entity with hidden contents. Hidden text can also be made
>> +visible automatically upon entry, with an optional delay. Text is
>
> Probably a good idea to describe org-inside-unhide-delay in the manual.
>> +;; and added to the context-dependent ctrl-c ctrl-c hook; see
>> +;; `org-inside-toggle-hidden'.
>
> `org-ctrl-c-ctrl-c-hook'
>
>> (defcustom org-inside-appearance '(:cursor bar :face org-inside-face)
>
> For all the defcustoms, please add :package-version and :safe settings.
Fixed/added all of these.
>> + (get-text-property (org-element-begin el) 'invisible))
>
> Why not `invisible-p'?
Because we are interested in whether the text at the entity's beginning
/could/ be invisible, not whether it actually is. Overlay properties &
buffer-invisibility-spec affect /actual/ invisibility.
>> + (setq org-inside--states (cl-delete state org-inside--states))))
>
> Any particular reason you are not using simple delete?
Nope, switched.
>> + (cl-callf2 push state org-inside--states))
>
> Why not just (push state org-inside--states)?
Good point: push always assigns anyway. Fixed.
>> + (let* ((sec-p (and face with-secondary-p (>= emacs-major-version 31)))
>
> I think it is worth explaining why we need this minimal version
> requirement somewhere in the top commentary.
Done.
>> + (let ((beg (org-element-begin elem))
>> + (end (- (org-element-end elem) (org-element-post-blank elem))))
>
> This is not accurate when the post-blank lines contain tabs/spaces.
I don't understand why or in what scenarios org-element "absorbs"
trailing white-space. Please suggest a workaround. I understand there
will be a new element `noblank-end' or so, but we can't use that?
>> + (when (get-text-property beg 'invisible)
>> + (setq beg (next-single-property-change beg 'invisible nil end)
>> + end (previous-single-property-change end 'invisible nil beg))
>> + (and (> end beg) (cons beg end)))))
>
> Considering that you are sometimes putting 'invisible 'totally-visible,
> invisible-p would be more reliable.
We do that only on the overlay. We are trying to find out where we are
inside the:
IIIIIIVVVVVIII
entity, where I=invisible, V=visible (real text properties only). We
want to know this independent of any other buffer setting which affects
visibility (as above). `invisible-p' is "too smart".
>> + (unless (minibuffer-window-active-p win)
>
> Why is it necessary?
If I recall, sometimes the sensor fires when you are entering the
minibuffer, which is never what we want.
>> + ;; for proper point adjustment
>> + (when (memq type '(emphasis raise))
>> + (org-rear-nonsticky-at visible-beg)
>> + (when (< emacs-major-version 31)
>> + (org-rear-nonsticky-at visible-end)))
>
> Why do you need to do it in org-inside? Maybe we can already apply
> rear-nonsticky as a part of fontification?
I'm respecting this dire warning (org.el):
;; FIXME: This would break current behavior with point
;; being adjusted before hidden emphasis marker when
;; using M-b. A proper fix would require custom
;; syntax function that will mark emphasis markers as
;; word constituents where appropriate.
;; https://orgmode.org/list/87edl41jf0.fsf@localhost
;; (org-rear-nonsticky-at (match-end 3))
I can't say I have understood what problems this caused. I'd be happy
to have the upstream do so (though I can understand if it doesn't want
to get into the subtleties of cursor-sensor differences between v30 and
v31).
>> +;; Starting in v31, buffer-local change functions are run in
>> +;; windows a buffer left as well those it entered.
>> +(unless (>= emacs-major-version 31)
>> + (add-hook 'window-buffer-change-functions #'org-inside--frame-changed))
>
> Why is this not a part of org-inside-mode setup?
> Side effects when merely loading a library should be avoided.
I assumed with our autoload in place, no one would ever load the library
except because they want to enable it. Easy enough to put it in the
setup (done). Since the mode is buffer-local, you can't turn it off
when the mode turns off. This btw is why I gate it for <=v30.
>> +(defvar org-hidden-text-functions nil
>
> We should probably announce this in news.
> Or declare it as internal hook with "--" in name.
I can imagine other tools doing something useful with this; I've added
to NEWS.
>> +(defvar org-extra-unfontify-properties nil
>
> Should be internal as well.
Done.
>> @@ -6434,21 +6465,19 @@ (defun org-raise-scripts (limit)
>> ;; Handle a_b^c
>> (when (member (char-after) '(?_ ?^)) (goto-char (1- pos)))
>> (unless (or comment-p emph-p link-p keyw-p)
>> - (put-text-property (match-beginning 3) (match-end 0)
>> - 'display
>> + (put-text-property (match-beginning 2) vend 'org-emphasis t)
>> + (add-text-properties (match-beginning 2) (match-end 2) props)
>> + (when (and (eq (char-after vbeg) ?{)
>> + (eq (char-before vend) ?}))
>> + (add-text-properties vbeg (1+ vbeg) props)
>> + (add-text-properties (1- vend) vend props)
>> + (setq vbeg (1+ vbeg) vend (1- vend)))
>> + (put-text-property vbeg vend 'display
>> (if (equal (char-after (match-beginning 2)) ?^)
>> (nth (if table-p 3 1) org-script-display)
>> (nth (if table-p 2 0) org-script-display)))
>> - (put-text-property (match-beginning 2) (match-end 3)
>> - 'org-emphasis t)
>> - (add-text-properties (match-beginning 2) (match-end 2)
>> - (list 'invisible t))
>> - (when (and (eq (char-after (match-beginning 3)) ?{)
>> - (eq (char-before (match-end 3)) ?}))
>> - (add-text-properties (match-beginning 3) (1+ (match-beginning 3))
>> - (list 'invisible t))
>> - (add-text-properties (1- (match-end 3)) (match-end 3)
>> - (list 'invisible t))))
>> + (run-hook-with-args 'org-hidden-text-functions 'raise
>> + (match-beginning 0) (match-end 0) vbeg vend))
>> t)))
>
> Do we have to change where 'org-emphasis property is applied? AFAIR,
> some third-party packages depend on the current behavior.
This is mostly just a convenience reorg to be able to pass vbeg/vend to
the new hook. We did need to change from invisible=t to
invisible=org-raise so our overlay invisibility countermanding approach
worked. And we needed to add rear-nonsticky too, for consistent point
movement.
Thanks for the review.
