Next chunk of comments, proposals, issues ... everything w.r.t. v31, but *without* Stefan's latest patch from bug#80973. I tried to structure that wall of text using Org/outline style head lines, hope it helps.
* Tests, in particular v30 vs v31 "J.D. Smith" <[email protected]> writes: > Biggest thing now is to > text v30 vs. v31 (which I haven't yet done), for functionality: > > - Enter/exit at correct locations for both (modulo the small difference > I pointed out w.r.t. the end). [For reference here is that difference as explained by you:] > One difference that should be expected between v30 and v31: when the > markers are unhidden (auto or via `org-inside-toggle-hidden'), you can > place point before the first marker while markers remain unhidden, but > moving after the last visible marker re-hides them. > - Editing text near boundaries (just inside, just outside) does what's > expected for both. > > - Changing buffer in windows works as expected for both. I did some tests here, but on v31 only. But TBH, with purely interactive tests such tests will always be ad-hoc in nature, and never cover your complete agenda above. Given the rather complex logic of this minor mode, and its implementation differences between v30 and v31+, maybe some ERTs would be in order here? As far as technically possible, that is. At least the cursor sensor functions should be triggered when calling functions through `ert-simulate-command', not sure whether this is enough. Without systematic ERTs I think the only thing to iron out issues (if any) is to wait for more users to test your code. Or in other words: Grinding tests manually is no fun, and I'm not ready for that. Anyway, as far as I have tested this on v31, everything feels natural, and the enter-exit behavior is indentical to what I remember from v30. Even w.r.t. "that small difference" when exiting an explicitly unhidden emph text to the rear end. That is, an explicitly unhidden emph text stays visible only while point is in a left-closed, right-opened interval like this: [=emphasized text=) If you would like to have it visible in a left-closed, right-closed interval like this (and *I* would like to have it that way): [=emphasized text=] then you need to make the overlay rear-advance as well for v31: (setq ov (make-overlay 0 0 (window-buffer win) t t)) (If I understand your mail correctly, you have been suspecting that already for v31.) * Using `org-inside-toggle-hidden' with `run-hook-with-args-until-success' > I don't fully understand your patch. The overlay is only on some text > if point is inside that text, so it seems the additional checks are > superfluous. > > [...] > > Confused again. When you narrow to a region, point is automatically > moved within that region, so the overlay is always strictly inside the > visible part of the buffer. You maybe have missed the main point of my patch, at least you do not refer to that aspect. With your latest fixes my patch for this main point got even simpler, so I repeat it here, hoping that the docstring explains its purpose: @@ -276,18 +276,23 @@ portion." (org-inside--setup)))) nil t)) +;;;###autoload (defun org-inside-toggle-hidden () "Toggle visibility of hidden text for entity at point. Operates only when inside an entity wrapped by hidden text and `org-inside-mode' is enabled. Text will be re-hidden when point leaves the entity. See `org-inside-appearance' to enable automatic unhiding or -configure other appearance settings." +configure other appearance settings. +This function returns a non-nil value if it has toggled visibilty, nil +otherwise. This makes it suitable as an entry in `org-ctrl-c-ctrl-c-hook', +for example." (interactive) - (when-let* ((ov (window-parameter nil 'org-inside-overlay)) - (_ (overlay-buffer ov))) + (and-let* ((ov (window-parameter nil 'org-inside-overlay)) + (_ (overlay-buffer ov))) (let ((inv (overlay-get ov 'invisible))) (overlay-put ov 'invisible - (if inv nil 'org-inside--not-hidden))))) + (if inv nil 'org-inside--not-hidden)) + t))) ;;;###autoload (define-minor-mode org-inside-mode [ The autoload cookie is required so that users not always using o-i-m can have the function on `org-ctrl-c-ctrl-c-hook', anyway. ] Probably Ihor had something like that in mind when he mentioned `org-reveal' up-thread. For example, o-i-t-h could be added to some new hook `org-fold-reveal-visibility-hook' which would get executed like this in `org-fold-reveal': diff --git a/lisp/org-fold.el b/lisp/org-fold.el index 58debf401..d08a14f9b 100644 --- a/lisp/org-fold.el +++ b/lisp/org-fold.el @@ -712,7 +712,8 @@ With a \\[universal-argument] \\[universal-argument] prefix, \ go to the parent and show the entire tree." (interactive "P") (run-hooks 'org-fold-reveal-start-hook) - (cond ((equal siblings '(4)) (org-fold-show-set-visibility 'canonical)) + (cond ((run-hook-with-args-until-success 'org-fold-reveal-visibility-hook)) + ((equal siblings '(4)) (org-fold-show-set-visibility 'canonical)) ((equal siblings '(16)) (save-excursion (when (org-up-heading-safe) * Other Previously Discussed Questions > Thanks, this was a subtle bug: you should not have been able to "prime" > the unhiding of the overlay. For whatever reason I recall being able to > stash an overlay and keep it out of the buffer by moving to 0,0, but > that isn't actually correct. I've switched to properly deleting and > moving (deleting would be better called "detaching"), now fixed. Thanks. BTW, the Elisp manual has: -- Function: make-overlay start end &optional buffer front-advance rear-advance This function creates and returns an overlay that belongs to BUFFER and ranges from START to END. **Both START and END must specify buffer positions**; they may be integers or markers. Accordingly: (overlay-start (make-overlay 0 0)) => 1 (overlay-end (make-overlay 0 0)) => 1 I guess that `(make-overlay 0 0)' (which you still use in `org-inside--overlay') is probably OK, but probably using `(make-overlay 1 1)' or even `(make-overlay (point-min) (point-min))' would be less confusing to other developers? AFAIU empty overlays should have the same effect (== none) on the buffer regardless where they are located. But that's not that important ... > Good point. I now loop over windows displaying the buffer where it is > being disabled/enabled. The overlay definitely needs to be > window-local, since you might be displaying the same buffer in multiple > windows, with different inside-ness status. Thanks again. > I think this is fair. No reason not to require cus-start on load, since > our file is autoloaded anyway. Probably add a comment on the `(require 'cus-start)' explaining its purpose? Just requiring `cus-start' for no apparent reason risks its removal, IMHO. >> I've noticed trailing whitespace in org-inside.el, not sure >> whether you or anybody else bothers. > Fixed. Not completely. Sorry for insisting - I have `delete-trailing-whitespace' on my `before-save-hook', so I notice these whenever I change your sources :-) [org-mode]$ grep -n '[[:space:]]$' lisp/org-inside.el 208: (walk-windows * Windows vs Buffers >> BTW, when walking windows (regardless for above issue or for >> `org-inside--reset-all'), probably consider walking with >> >> (walk-windows ... nil t) >> >> Otherwise, e.g., *customize*-in-one-frame-org-mode-buffer-in-other >> scenarios still can lead to dangling bar shaped cursors when >> customizing o-i-a. These operations should be rather infrequent, >> so more walking shouldn't be a problem. > > Fair point; done. I made the mistake of looking at your commit, and now I'm entering the murky marshes of review, where I can (again) be completely wrong; or not. - Function `org-inside--reset-all' loops over all windows. However, the functions `org-inside--(teardown|setup)' called in that loop seem to operate on the current buffer. With these both functions themselves looping over all windows of the current buffer, this combo of functions now seems to do double work. - Plus `org-inside--(teardown|setup)' do *not* loop over all frames in their calls to `get-buffer-window-list'. As a result, you get (again) stray bar shaped cursors when having a buffer open in two frames and switching o-i-m in the right (or wrong) manner. * One New Issue? The new exit overlay has one drawback: It does not automatically get deleted when the corresponding text properties get deleted. As a reproducer, use a standard `o-i-m' setup and move point to directly after a front emph marker, then `org-delete-backward-char'. The text properties get cleared immediately by the font lock machinery, but at least for me the overlay continues to exist, and the cursor is still bar shaped until I also leave the overlay. * Random and Completely New Review Notes More random observations I came across when trying to understand org-inside better. (:face (choice (face :tag "Face Name") (plist :tag "Attribute List")) :tag "Cursor Face") ^^^^^^^^^^^ Shouldn't this be "*Text* Face" or whatever, but not cursor face? (when (eq type 'emphasis) (org-rear-nonsticky-at visible-beg) (when (< emacs-major-version 31) (org-rear-nonsticky-at visible-end))) Second `when' is misindented. (defsubst org-inside--restore-cursor (&optional win) (defsubst org-inside--clear-overlay (&optional win) (defun org-inside--buffer-change (&optional win) For all these I'd made WIN mandatory - at least you always pass an argument when you call them. But that's a matter of taste, really. This might be an Emacs 30 vs 31 thing again, but I replaced below magic ;; We move the overlay when returning to the run-loop to avoid ;; the cursor-sensor race for point adjustment, since our ;; overlay and the underlying text both target the same ;; cursor-sensor-functions. (run-at-time 0 nil (lambda (buf) (with-current-buffer buf (if inside-p (move-overlay ov beg end) (delete-overlay ov)))) (current-buffer)) by a plain (if inside-p (move-overlay ov beg end) (delete-overlay ov)) and did not notice any adverse effects. Could you provide more details what IYHO would *break* w/o that `run-at-time' call and how to provoke that? Probably provide it even in the comment for the benefit of others as well? (defun org-inside--add-emphasis-props () That function does not seem to get called any longer. "Add text properties to invisible text for org-inside functionality. TYPE is the type of text being hidden. BEG, END, VISIBLE-BEG, VISIBLE-END are the buffer positions of the link text and its visible portion." This docstring probably should use more generic terms than "link text". ;; for proper point adjustment (when (eq type 'emphasis) (org-rear-nonsticky-at visible-beg) (when (< emacs-major-version 31) (org-rear-nonsticky-at visible-end))) Hm. Don't you break encapsulation here by modifying text properties that Org mode "owns"? Plus you never seem to undo these changes when o-im gets turned off, in contrast to the other text properties you add. PLUS I think your changes counter this comment from Ihor in `org-do-emphasis-faces' (Ihor's change being done on my request, BTW): ;; 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)) because you now call `org-rear-nonsticky-at' on `(match-beginning 4)' == `(match-end 3)'. I think I understand why you do that, but this seems at least to call for more comments. Phew. Thanks for reading this.
