Jens Schmidt <[email protected]> writes: > 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.
Thanks. My outli package auto-applied outline-minor-mode to them in GNUS, very convenient. > * 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. The logic is really not very complex, though v<=30 vs. v31 introduces some (temporary) complexity. I don't believe it merits tests, but if Ihor or other org maintainers do, I'd be open to contributions. > 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. Great. > 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=) By which you mean you cannot place point here ([X]): =emphasized text=[X] and have the markers remain visible? That's the situation we have (see below). > 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)) Oh, I think I had intended to do so! Did you test with that enabled? I agree that the symmetry of left-closed + right-closed makes the most conceptual sense. In fact it works just fine with REAR-ADVANCE even in v30. Although now testing I see the issue with that: when markers are visible with point here: =emphasized text=[X] inserting new text extends the "inside" overlay and its styling (of course; it's REAR-ADVANCE). So maybe half-open is the lesser of two evils. > * Using `org-inside-toggle-hidden' with `run-hook-with-args-until-success' > >> I don't fully understand your patch. > > 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: Understood. Seems fine to return a value; done. I just tried on `org-ctrl-c-ctrl-c-hook' and it indeed works very well, and really fits the "do something here" vibe of C-c C-c. /Great/ idea. Maybe we should add the command to this hook upon enabling the mode? Ihor? > [ 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. ] I'm agnostic on the autoload. You can always require the package before adding to a hook, and an org-inside command floating around without configuration might confuse users. What's the standard for org extensions? Generous or limited autoloading? > 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) Interesting idea. I don't use org-reveal so this could be done separately. > * Other Previously Discussed Questions > >> 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. Done. > * Windows vs Buffers > > 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. Yes it did some extra work, for org-inside buffers shown in multiple windows. I updated it to avoid that. > - 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. Thanks, had frames enabled for reset but not teardown. > * 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. Indeed, I have seen that behavior too. Exit overlays are important for things like links that are more than one hidden char on each side. Font lock does not remove the overlay, and it isn't really the place to look for entities gone bad. I think I have a solution. I have now implemented an overlay modifications-hooks function, which I use to detect (using org-element) when the overlay should "self destruct". Please give a try. Looks good in my tests. It occurred to me that some people may /like/ the earlier behavior, since when you accidentally destroy a link/emphasis through edits, you could still see a visual indication of where it was (if. you alter face, e.g. add an underline). But I think the new behavior is less surprising overall. I could be convinced to include a config option > * 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? Yes! Thanks. I had formerly hoped to change the face 'cursor using face remapping, but that face is treated specially, and remaps do not apply. See Bug#80905. I actually think something like underline face with a special color is good. Which reminds me, we will have to decide on the the defaults for `org-inside-appearance'. > (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. Fixed. > (defsubst org-inside--restore-cursor (&optional win) > (defsubst org-inside--clear-overlay (&optional win) > (defun org-inside--buffer-change (&optional win) This was vestigial. I had formerly wanted to leave win=nil as selected-window (which most command honor), but found I couldn't fully get away with it. > 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? It's a race condition primarily when auto-unhiding is on: point adjustment looks for hidden text to decide how far to move point, but moving the overlay in on cursor sensor (with its countermanding 'invisible prop) un-hides the text. The ordering of these operations affects where point will land. I find it differs between systems, versions, etc. So for stability, we defer moving the overlay to allow point adjustment to proceed repeatably. This also eases a similar hypothetical race between duplicate cursor-sensor-functions (overlay and text property). I've improved the comment. > (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". Fixed. > > ;; 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)) We certainly need the rear non-sticky for older versions, when we extend the visible end. I could replace both of these by tailored rear-nonsticky props, but since org itself is in charge of removing these properties, I thought it simpler to recyle the calls it uses. Once an entity is broken (or you refontify after disabling), the sticky properties will return as before. > 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. That was the old code (now removed), should be clearer now why we "extend" by one character for older Emacs versions. I need to catch up with what you and Stefan are doing for v31 cursor-sensor firing. > Phew. Thanks for reading this. Thanks for the extra thorough review! JD
