Ihor Radchenko <[email protected]> writes: > "J.D. Smith" <[email protected]> writes: > >> Fixed/added all of these. > > Hmm. Could you also push the changes to savannah? > I still see ae0e2cdb2 as the latest commit.
Ooops, push failed before for some reason. Should be on 17e801515 now. >>>> + (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. > > Once upon a time, for a short while, link brackets were hidden using > overlays. get-text-property does not look into overlays. > You need get-char-property. Wow, that was an odd choice. Using font lock? org-inside's own overlay uses 'invisible. For this reason, it is safer to use text-properties for this. >>>> + (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. > > You code will actually work for objects. All whitespace after the end of > object is inside its post-blank, counting characters. > The problem is with paragraph-like elements - post-blank counts *lines* there. > I'd put a comment that ELEM must be an object. Then, the code is 100% correct. Comment added. >>>> + (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". > > But what happens if (get-text-property beg 'invisible) is non-nil but > we also have invisibility spec making it visible? I feel that the logic > in the code is off in such situation. That's totally fine. We're trying to discover the VVVV part of the org-element-identified element, so we can see if we are truly "inside" it. This is used only for nested hidden-marker elements. At the top level, our cursor-sensor functions find the VVVV regions for us. For nested element, we rely on org-element. So we don't care whether I=invisible is operational in the buffer or not. We care only if we are in the IIII or VVVV part. >>>> + (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. > > It is worth an explanatory comment. > If I understand you correctly, you do not want cursor/visibility changes > when user, say, types M-x while "inside" something. I thought I could remove this check, but quickly found that round-trips through the minibuffer can cause "cursor type leakage". So I reverted (with a comment). >>>> + ;; 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. >>>> +;; 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. > > 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. >> 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. > > What do you mean by "can't turn it off"? I meant only that you cannot remove a global hook when the buffer-local mode is disabled, since other buffers may still need it. So even if all buffers have org-inside disabled, it will continue to do a (tiny bit) of unnecessary work. Again, a v<31 corner case. >>> 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.
