Ihor Radchenko <[email protected]> writes:
> "J.D. Smith" <[email protected]> writes:
>
>>>>>> + ;; 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).
>>>
>>> That warning is about 'invisible specifically.
>>> But, AFAIU, you are dealing with cursor-sensor property.
>>> Why not limiting non-stickiness to that specific property?
>>
>> The commented-out position (match-end 3) is where we do now add
>> 'rear-nonsticky (in org-inside). If this final line of code in the
>> stanza above wasn't commented out, we wouldn't need that. But
>> presumably you put that big FIXME in there (in 724135dda) to prevent
>> someone from just commenting it back in :). If you think it's now OK to
>> do so, we can remove the equivalent from org-inside.
>
> That is only commented out for the purposes of 'invisible stickiness.
> But we can limit stickiness to 'cursor-sensor-functions by setting the
> value of 'rear-nonsticky to a list of properties.
I don't follow you here. Beyond cursor-sensor-functions, we do in fact
need to add 'invisible non-stickiness at visible-beg/end locations, to
achieve normal point adjustment driven movement on each side. On v31+,
the latter (visible-end) is already done for us by:
(org-rear-nonsticky-at (match-beginning 5))
To summarize, we need:
1. cursor sensor functions from visible-beg to (1+) visible-end (the 1+
only for emacs <31)
2. rns += 'invisible at visible-beg (this is what the commented out line
would do)
3. rns += 'invisible at visible end, for emacs <31.
Are you proposing to uncomment the line in org.el:
;; (org-rear-nonsticky-at (match-end 3))
That would remove the need for #2, not the others. Or something else?
>>> This is Elisp-wide convention that loading libraries should not cause
>>> side effects. To the point, that Elisp manual says
>>>
>>> • Simply loading a package should not change Emacs's editing
>>> behavior. Include a command or commands to enable and disable the
>>> feature, or to invoke it.
>>>
>>> This convention is mandatory for any file that includes custom
>>> definitions. If fixing such a file to follow this convention
>>> requires an incompatible change, go ahead and make the incompatible
>>> change; don't postpone it.
>>
>> I'm certainly aware, but like most conventions, with good reason they
>> can be broken. In any case it's already fixed. Just a bit inefficient
>> to try to add the global hook each time, but its doing so on v<31 only,
>> so tolerable.
>
> This one is particularly strongly-worded. To the point of suggesting
> breaking changes. Suggesting breaking changes is almost like cursing in
> Elisp world :)
:). Sadly right now neither our buffer-local or global functions are working,
so that's a bigger concern.
>>>>> 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.
>>>
>>> You could pass adjusted values to the hook. I do not see why changing
>>> how the actual text properties are applied is necessary.
>>>
>>>> We did need to change from invisible=t to
>>>> invisible=org-raise so our overlay invisibility countermanding approach
>>>> worked.
>>>
>>> That should be fine. I do not know any code that would depend on
>>> invisible to be exactly t. In fact, it was not t for quite a while in
>>> one of the releases.
>>
>>>> ... And we needed to add rear-nonsticky too, for consistent point
>>>> movement.
>>>
>>> See above.
>>
>> My point was, because I needed to track visible region, and also change
>> the applied properties, it made sense for efficiency and compactness to
>> pre-compute them and use directly. The end result is it reads more
>> clearly, IMO. Feel free to rework if you feel it's important.
>
> I feel that we are talking past each other.
> I am talking about
>
> (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))
>
> where you apply 'org-emphasis to (match-beginning 2) ... venv
> while it was (match-beginning 2) ... (match-end 3) in the past.
I understood this, but you are not being specific about your concern(s).
You point to the distinction between:
+ (put-text-property (match-beginning 2) vend 'org-emphasis t)
- (put-text-property (match-beginning 2) (match-end 3)
- 'org-emphasis t)
but note the enclosing:
(let* ((vend (match-end 3)) ...
So these are identical. Is your concern:
1. That the code is changing where org-applied properties (invisible)
fall?
2. That the code applies org-managed properties at the same text
locations, but the code to do so reads slightly differently?
3. That the code applies some additional/different properties
((invisible org-raise rear-nonsticky (invisible))) prior to calling
the hook?
4. Something else?
My take on 1-3:
#1: Should not be the case for invisible (counter-example welcome).
#2: The changes needed to compute data for the hook, and the extension
to applied properties suggested a slight re-org.
#3: org-inside needs these properties. Note that the rear-nonsticky
behavior is changed here for consistency with other hidden-contents
entities like emphasized text.
In any case, if I'm still not understanding you, it might be better for
you to propose new code for me to look at and test.
