Closed awth13 closed 3 years ago
So Org Mode probably calls a function to refontify the buffer? If this function is stored in one of the hook lists, than maybe we could simple temporarily remove this function from the hook list? Another (better?) idea is to write a wrapper of some kind, that simply doesn't call this function, when we are inside an emphasized word.
But I haven't really done anything with the Org internals, so I don't really know, whether one of these ideas is suitable or not, and how to implement this.
Org mode relies on font-lock-mode
for fontifications (and font-lock-mode
is very confusing!). As far as I can see -- and I asked around in #emacs on freenode too -- there isn't really an exposed hook list to attach to or remove stuff from.
I'm also not well-versed in the way Org mode handles fontication. I'm beginning to suspect that the only truly "clean" way to handle the issue would be to modify Org mode or font-lock-mode
source to make them aware that the relevant parts should be ignored. This would be too much for me at this point though, given that I've just started hacking on Emacs.
~In any case, I implemented another solution in the last commit, which I think is reasonable. It temporarily modifies font-lock-extra-managed-props
so that font-lock-mode
considers our silent modifications valid and doesn't revert them. If we can handle bugs/edge cases, I think we can push this solution to release, unless I come up with a better way in the process.~
Scratch that! I actually came up with a better solution, based on your suggestion to
temporarily remove... from the hook list
We can simply disable jit-lock-mode
when inside an active fragment, which turns off automatic fontification temporarily. Since I can't see any reason for the user to need jit-lock-mode
when editing inside a fragment, I think this is OK.
Support for editing inside fragments using jit-lock-mode
is implemented here. Since it works reasonably well, this will be the working solution unless/until a better one is found.
@SPFabGerman, I did find the list of functions run after changes in the buffer -- after-change-functions
. It seems, however, that you can't modify it when jit-lock-mode
is enabled.
Hi @awth13 , here gusbrs from Reddit, which made a comment about this there when you announced the package. I'm also the one who took part with Shankar in the discussion in the Org mailing list, with which you are acquainted, and also the one who raised the limitation in Shankar's solution in case of editing.
I had questioned you at Reddit about this solution to the problem, and I do believe this is indeed a big drawback. The performance toll of using org-element
is a matter of how much one is willing to go in this direction, and each one can judge if the performance is good enough for their use cases, and this is a place mileages vary quite a lot. It certainly has the advantage of not requiring any change upstream (or a patch in the init file, since that didn't come anyway).
What I didn't mention at Reddit is that I had reached a solution to the problem, based on the Shankar/text-properties approach. And I didn't mention because Shankar is the author and I had already sent it back to him, and was hoping he would come forward with it. That would certainly have been more appropriate. But, since he didn't, here I am.
I don't have the time at the moment to try to adapt it to the context of your org-element
approach, but I'd be glad to share what I've reached so that you could try it. The overall approaches are different, but the fact that you could make yours work by disabling jit-lock-mode
makes me think my solution might be applicable, because it essentially boils down to ensuring font-lock is done at appropriate regions before querying for an emphasized region. Would that be welcome?
Hey, @gusbrs! Thank you for reaching out!
Saying that org-appear
is far from perfect is an understatement, and any help or suggestions are appreciated! It would be great if you could share your solution or even just describe what it does.
To make sure that we're on the same page: the org-element
API is used only for detecting Org elements. We could swap it for another method, e.g., regular expressions, and org-appear
will still work.
Hi, @awth13 , great!
And, oh!, I did not come to criticize, quite the contrary. I do think there is a performance toll from using org-element
, but I also do agree it is the best option for a package, considering the upstream change was rejected by Org maintainers. First, it is the base official source of Org syntax. It also uses regexps to parse, so I wouldn't expect you'd be able to reach great computational economies by using something else, and using something else would certainly make thinks much more complicated and add to the maintenance burden. Second, on a short test I did here, and this is probably your feeling too, things seemed responsive enough to me. The fact that org-fragtog
exists using the same approach and is used by a number of people is good evidence that the tool is acceptable. Of course we will always find some people that think otherwise, depending on use case, hardware and preferences. The refusal upstream was based on the sole fact that the feature used post-command-hook
quite regardless of whether the content of the added hook was light or costly, and also regardless of the fact that it would be an op-in feature, not enabled by default.
However, disabling jit-lock-mode
is what I personally consider a deal-breaker. I think you are getting into a slippery slope there, and if you could solve it, it would be great, and I'd love to see org-appear
show up in Melpa some time soon.
Indeed jit-lock
, even though it should be kept, does introduce a difficulty in trying to sort this feature. As far as I understand it, this is due to the fact that under jit-lock-mode
, font-lock will run with some delay, and thus after post-command-hook
.
This is true for both the text-properties and the org-element approaches. In the case of the text-properties approach, this would even prevent us from properly identifying the emphasized region, because the text property itself was still to be placed by font-lock after a change. I don't expect this to affect the org-element approach, as I presume the org-element
API will give us reliable boundaries regardless of the state of font-locking. So I'm not sure why jit-lock-mode
affects behavior in the org-element approach, but my hunch would be that the unhiding occurs when post-command-hook
runs, but is overridden by Org defaults when the font-lock comes in sequence. Does that make sense?
In the case of the text-properties approach I was able to make things work for the editing case by running font-lock-ensure
at appropriate regions and appropriate times, particularly right before querying for the emphasized region's boundaries and before unhiding takes place. That's the "working bit" of things in what I did there, everything else is just trying to ensure this region is the smallest possible to reduce the computational cost.
I send below the state of things in my init file for org-auto-emphasis-mode
. As you know, it was originally developed by Shankar Rao, I just fixed this issue of the unhiding in case of change (plus a couple of corner cases that I've found along the way), which is the one at hand here. It's decently commented, I think, so it should be understandable with the overall perspective I just mentioned above. But, of course, if you have questions about it, just shoot them. I'll be happy to expand on how this code is meant to work, but the task of bridging it to the org-element
approach I'll leave to you. I'm not sure there is an easy bridge there, but I think it is likely that there is one, as I could easily invert the phrase:
To make sure that we're on the same page: the
org-element
API is used only for detecting Org elements. We could swap it for another method, e.g., regular expressions, andorg-appear
will still work.
And say the same thing about the text-properties. Still, even when a bridge cannot be found, perhaps you can get some fresh ideas to try in your context based on it.
(with-eval-after-load 'org
;; ----- Org-auto-emphasis-mode -----
;;
;; Developed by Shankar Rao, with improvements of my own.
;; See thread at the Org mailing list:
;; https://orgmode.org/list/CAGEgU=j+UJoWwoRKChkVxN5dmwbD4YaNTWdLS6Qgj57osZLRJA@mail.gmail.com/
;; https://orgmode.org/list/CAGEgU=httRszKo_Yj=X+U4UYKwWtbQMPCunfyM187B+QzWodKw@mail.gmail.com/
;; And at Reddit:
;; https://www.reddit.com/r/orgmode/comments/gss1g4/update_i_made_my_own_sbrorgemphasizemode_that/
(customize-set-variable 'org-hide-emphasis-markers t)
(add-hook 'org-mode-hook #'org-auto-emphasis-mode)
(defcustom org-auto-emphasis-unhide-at-point 'right-edge
"If non-nil, unhide the emphasis markers for the text at point.
If set to the symbol `right-edge', also unhide the emphasis markers if point
is immediately after the emphasized text. The emphasis markers will be
rehidden as soon as point moves away from the emphasized text. If set to nil,
the emphasis markers always remain hidden."
:package-version '(Org . "9.5")
:type '(choice (const :tag "Never unhide emphasis markers" nil)
(const :tag "Unhide when point is inside" t)
(const :tag "Unhide when point is inside or at right edge" right-edge))
:group 'org-appearance)
(defun org-do-emphasis-faces (limit)
"Run through the buffer and emphasize strings."
(let ((quick-re (format "\\([%s]\\|^\\)\\([~=*/_+]\\)"
(car org-emphasis-regexp-components))))
(catch :exit
(while (re-search-forward quick-re limit t)
(let* ((marker (match-string 2))
(verbatim? (member marker '("~" "="))))
(when (save-excursion
(goto-char (match-beginning 0))
(and
;; Do not match table hlines.
(not (and (equal marker "+")
(org-match-line
"[ \t]*\\(|[-+]+|?\\|\\+[-+]+\\+\\)[ \t]*$")))
;; Do not match headline stars. Do not consider
;; stars of a headline as closing marker for bold
;; markup either.
(not (and (equal marker "*")
(save-excursion
(forward-char)
(skip-chars-backward "*")
(looking-at-p org-outline-regexp-bol))))
;; Match full emphasis markup regexp.
(looking-at (if verbatim? org-verbatim-re org-emph-re))
;; Do not span over paragraph boundaries.
(not (string-match-p org-element-paragraph-separate
(match-string 2)))
;; Do not span over cells in table rows.
(not (and (save-match-data (org-match-line "[ \t]*|"))
(string-match-p "|" (match-string 4))))))
(pcase-let ((`(,_ ,face ,_) (assoc marker org-emphasis-alist))
(m (if org-hide-emphasis-markers 4 2)))
(font-lock-prepend-text-property
(match-beginning m) (match-end m) 'face face)
(when verbatim?
(org-remove-flyspell-overlays-in
(match-beginning 0) (match-end 0))
(remove-text-properties (match-beginning 2) (match-end 2)
'(display t invisible t intangible t)))
(add-text-properties (match-beginning 2) (match-end 2)
'(font-lock-multiline t org-emphasis t))
(when (and org-hide-emphasis-markers
(not (org-at-comment-p)))
(add-text-properties (match-end 4) (match-beginning 5)
'(invisible org-link))
(add-text-properties (match-beginning 3) (match-end 3)
'(invisible org-link))
;; This is the only change in `org-do-emphasis-faces'
;; required for `org-auto-emphasis-mode' to work.
(add-text-properties (match-beginning 2) (match-end 2)
`(org-emph-start
,(copy-marker (match-beginning 2))
org-emph-end
,(copy-marker (match-end 2)))))
(throw :exit t))))))))
(defvar-local org-auto-emphasis--unhidden-bounds nil
"Stores the bounds of the current emphasized region with unhidden markers.")
(defvar-local org-auto-emphasis--after-change-bounds nil
"Stores the bounds of change resulting from a given command.")
(defun org-auto-emphasis--get-bounds ()
"Return the bounds of the emphasized region at point.
If `org-auto-emphasis-unhide-at-point' is set to `right-edge' and point is
immediately after a closing emphasis marker, return the bounds
at (1- (point))."
(let* ((end-1 (when (eq org-auto-emphasis-unhide-at-point 'right-edge)
(get-text-property (max (1- (point)) (point-min))
'org-emph-end)))
(pos (if (and end-1 (= end-1 (point)))
(1- (point))
(point)))
(start (get-text-property pos 'org-emph-start))
(end (get-text-property pos 'org-emph-end)))
(when (and start end) (list start end))))
(defun org-auto-emphasis--post-command-hook ()
;; When inserting a new emphasis, either by typing the second marker
;; directly, or by running 'org-emphasize', we may not recognize point to
;; be at an emphasis if a new emphasis was created. This happens because
;; at 'post-command-hook' the font-lock has not yet been updated. As a
;; result, '(get-text-property ... 'org-emph-start/end)' when called from
;; 'post-command-hook' may return nil, so we must ensure font-lock is
;; performed in appropriate boundaries when a change occurs, before
;; calling 'org-auto-emphasis--get-bounds'.
(when org-auto-emphasis--after-change-bounds
(let* ((max-lines (nth 4 org-emphasis-regexp-components))
;; Limit the area to which the font-lock is ensured to reasonable
;; bounds.
(beg (max (car org-auto-emphasis--after-change-bounds)
(line-beginning-position (- (1- max-lines)))))
(end (min (cadr org-auto-emphasis--after-change-bounds)
(line-end-position (1+ max-lines)))))
;; Make sure we change at least one char (in case of deletions).
(font-lock-ensure beg (max end (1+ beg)))))
;; Re-hide emphasis markers when leaving an emphasized region.
(when (and org-auto-emphasis--unhidden-bounds
;; We already ensured font-lock above in case of change, no
;; need to do it again.
(not org-auto-emphasis--after-change-bounds)
;; Also, only refontify when moving from one emphasized
;; region to another.
(not (equal (org-auto-emphasis--get-bounds)
org-auto-emphasis--unhidden-bounds)))
(let ((beg (car org-auto-emphasis--unhidden-bounds))
(end (cadr org-auto-emphasis--unhidden-bounds)))
(font-lock-flush beg end)
(font-lock-ensure beg end)
(setq org-auto-emphasis--unhidden-bounds nil)))
;; Unhide emphasis markers for the current region, when needed.
(let ((cur-bounds (org-auto-emphasis--get-bounds)))
(when (and cur-bounds
(or org-auto-emphasis--after-change-bounds
(not (equal cur-bounds
org-auto-emphasis--unhidden-bounds))))
(let ((beg (car cur-bounds))
(end (cadr cur-bounds)))
(with-silent-modifications
(remove-text-properties beg (1+ beg) '(invisible nil))
(remove-text-properties (1- end) end '(invisible nil))))
(setq org-auto-emphasis--unhidden-bounds cur-bounds)))
;; Reset the change bounds
(setq org-auto-emphasis--after-change-bounds nil))
(defun org-auto-emphasis--after-change-function (start end _old-len)
;; A single command may perform several changes (eg. 'fill-paragraph',
;; even 'org-emphasize' performs 2), so we accumulate here the extreme
;; values of start and end to do whatever we need just once in
;; 'post-command-hook' according to these values. Note that
;; 'after-change-functions' run *before* 'post-command-hook'.
(if org-auto-emphasis--after-change-bounds
(setq org-auto-emphasis--after-change-bounds
(list (min start (car org-auto-emphasis--after-change-bounds))
(max end (cadr org-auto-emphasis--after-change-bounds))))
(setq org-auto-emphasis--after-change-bounds (list start end))))
(define-minor-mode org-auto-emphasis-mode
"Toggle Org Auto Emphasis mode.
This mode, when enabled, unhides emphasis markers for the text at point,
depending on the value of `org-auto-emphasis-unhide-at-point'. With a prefix
argument ARG, enable Org Auto Emphasis mode if ARG is positive, and disable it
otherwise. If called from Lisp, enable the mode if ARG is omitted or nil.
To enable this in all Org files, add the following line to init.el:
(add-hook \\='org-mode-hook #\\='org-auto-emphasis-mode)
"
;; The current implementation is very good, though perhaps a little too
;; "expensive" for the hooks it uses. The single corner case it does not
;; deal with, which I'm aware of, is that of non-nested combined
;; emphasized regions (the first region of type a ends after the second
;; region of type b has started, and type b region ends after type a has
;; ended). But this is a limitation of using text properties in this way.
;; We have only *one* 'org-emph-start'/'org-emph-end' at any given point,
;; the one which comes later gets precedence. Anyway, I see no way to
;; deal with this except parsing the context around point, which may well
;; become prohibitive. Besides, this case seems to fool both
;; '(org-in-regexp org-emph-re)' and '(org-element-context)', so I'm not
;; even sure it is legit syntax.
:init-value nil
(if org-auto-emphasis-mode
;; Turn on
(when org-hide-emphasis-markers
(setq-local font-lock-extra-managed-props
(append font-lock-extra-managed-props
'(org-emph-start org-emph-end)))
(when org-auto-emphasis-unhide-at-point
(add-hook 'post-command-hook
#'org-auto-emphasis--post-command-hook nil t)
(add-hook 'after-change-functions
#'org-auto-emphasis--after-change-function nil t))
(font-lock-flush))
;; Turn off
(remove-hook 'post-command-hook
#'org-auto-emphasis--post-command-hook t)
(remove-hook 'after-change-functions
#'org-auto-emphasis--after-change-function t))))
Thank you again for coming forward, @gusbrs, and sorry for a bit of a delay. I wanted to spend some time testing your solution before responding.
I agree that org-element
is sufficiently responsive and, even though there might be some room for optimisation in the way org-appear
uses the API, I trust that Org maintainers took performance into consideration. I certainly don't think that I can do better -- I did try to come up with my own implementation for bounds searching in the first version of org-appear
and it was less performant.
In any case, as far as I understand your solution, it unhides emphasis markers after each command, updating the bounds if necessary and calling font-lock-ensure
within the updated bounds, which prevents jit-lock-mode
from refontifying the ensured region. Is that correct?
I already have a working prototype based on your solution and it works great without disabling jit-lock-mode
! I'll do some more testing tomorrow but I think this modification can be pushed to master without any major changes to org-appear
. Thank you for sharing!
I agree that
org-element
is sufficiently responsive and, even though there might be some room for optimisation in the wayorg-appear
uses the API, I trust that Org maintainers took performance into consideration. I certainly don't think that I can do better -- I did try to come up with my own implementation for bounds searching in the first version oforg-appear
and it was less performant.
I concur. If I were in your place, I would only consider diverging from the org-element
API if it proves to be an issue in responsiveness.
In any case, as far as I understand your solution, it unhides emphasis markers after each command, updating the bounds if necessary and calling
font-lock-ensure
within the updated bounds, which preventsjit-lock-mode
from refontifying the ensured region. Is that correct?
Yes, precisely. Well, at least as far as I understand it, I can't really claim to have a full grasp of font-lock/jit-lock.
I already have a working prototype based on your solution and it works great without disabling
jit-lock-mode
! I'll do some more testing tomorrow but I think this modification can be pushed to master without any major changes toorg-appear
.
That's great to hear! When you do push it, let me know. And, if testing goes well, I do stimulate you to make it available in Melpa.
I can't really claim to have a full grasp of font-lock/jit-lock
And neither can I :D
I just pushed the implementation of org-appear
based on your solution, @gusbrs. It seems to work entirely as expected, which is great. Thank you again for suggesting it!
I am experimenting with the way org-appear
handles nested emphasis elements. In this new implementation, to make text move around less frequently, parent elements are always enabled. I want to work some more on this but, once it's done, I will definitely add org-appear
to MELPA.
I just pushed the implementation of
org-appear
based on your solution, @gusbrs. It seems to work entirely as expected, which is great. Thank you again for suggesting it!
Great! I'm quite happy about it too! I've been using this functionality for some time now and was really, really enjoying it, and I wanted this to be available to other people. The upstream response was quite frustrating. I thank you for taking the time to adapt the solution to a more packageable approach.
I am experimenting with the way org-appear handles nested emphasis elements. In this new implementation, to make text move around less frequently, parent elements are always enabled.
I've played with this too. In the end, I preferred to unhide only the innermost markers. But it is a matter of preference in this case. There were some technical issues too, regarding how many nesting levels to handle (theoretically, you can have as many as there are different kinds of emphasis markers). But in this regard org-element
is probably more capable than text-properties. You'll still have to draw an arbitrary line as to how many nesting levels are supported, I guess.
One thing you could consider adding, and which is simple enough, is an option to customize the "right-edge" behavior. I very much agree the unhiding works best as it is, but this is a traditional option in prettify-symbols-mode
, so we can presume there exist people who prefer otherwise.
I see you've met the issue of overlapping emphasized regions too. I see no solution for this in either approach. Text-properties seem to handle them somewhat more gracefully, but I agree the case should be rare enough that the readme notice is more than sufficient. And, of course, the answer to:
*Why would someone /nest emphasis* like that?/
is because testers were made to pester developers. :D
Btw, I took it for a spin, and light testing is looking very good to me. Except for the "known issues" it is working just as expected, and responsiveness seemed quite good to me too. A look at https://github.com/awth13/org-appear/commit/845be82b7a452e32caee0895e3388d5939f48eb5 also looks good to me.
I want to work some more on this but, once it's done, I will definitely add
org-appear
to MELPA.
I'm looking forward to it, and thank you for that.
I will have to think about the right edge setting, @gusbrs. It may be awkward to code since org-element
tends to consider right edges as part of the element but does not do this consistently. I understand that certain users may expect to have this setting available and I will definitely explore it but, personally, I don't find it useful in the context of org-appear
.
You'll still have to draw an arbitrary line as to how many nesting levels are supported, I guess.
The current implementation is recursive so, in theory, it supports infinite nesting. In practice, of course, it goes only as deep as org-element
does -- and org-element
"breaks" at 5 nesting levels for reasons, I assume, similar to the reason why it struggles with overlapping elements.
Regarding overlapping elements, the maintainer of org-fold told me that they are working on making that more consistent. As it stands with the current version of Org mode, though, I am not planning to prioritise fixing Org mode bugs within org-appear
, especially since this seems to be quite a rare of an edge case.
Hi @awth13 !
Regarding right edges, it was just a suggestion, at a polishing level, for you to consider, nothing else. I also very much prefer current behavior. And I was unaware of:
org-element tends to consider right edges as part of the element but does not do this consistently.
Regarding nesting, interesting, if org-element
is already recursive, you are covered. But it does beg the question of which behavior is more disruptive, and which users will prefer. Without nesting two emphasis may change state at the same time when we move from an inner to an outer emphasized region, but the overall length of the unhidden text is maintained, and kept at a cumulative maximum of 2. With nesting only ever one region would change at a time, but cumulative displacement could reach up to 10 characters (true, 5 nesting levels would not be a typical case). It is a matter of taste/preference, and just as a data point, this particular user prefers no nesting, based on my use and attempts with the text-property approach.
Regarding overlapping elements, I very much agree with your stance, and think that's a sensible one.
Hey, @gusbrs!
Just wanted to let you know that org-appear
is now available in MELPA. I haven't worked on implementing the right-edge behaviour yet but I have recently discovered another consequence of using org-element
. I've told you before that org-element
considers the right edge as part of an element. It also allows the right edge to be arbitrarily large (read: any amount of whitespace is allowed). This leads to an unfortunate behaviour where the cursor is several empty lines after the element and the element is still toggled.
Because of this, I am now seriously considering either dropping org-element
and implementing an entirely new way to detect elements or reworking the way org-appear
interfaces with org-element
. If I go with the former, the right-edge option might make more sense.
Finally, thank you for your elaborate thoughts on nesting! Do you think it would be better if org-appear
had an option that would allow the user to choose the way children elements are toggled?
Hi @awth13 !
Just wanted to let you know that
org-appear
is now available in MELPA.
This is great news! Thank you for making it available. In my experience this functionality is very useful, I'm glad Org users in general now will have access to it.
I haven't worked on implementing the right-edge behaviour yet but I have recently discovered another consequence of using
org-element
. I've told you before thatorg-element
considers the right edge as part of an element. It also allows the right edge to be arbitrarily large (read: any amount of whitespace is allowed). This leads to an unfortunate behaviour where the cursor is several empty lines after the element and the element is still toggled.Because of this, I am now seriously considering either dropping
org-element
and implementing an entirely new way to detect elements or reworking the wayorg-appear
interfaces withorg-element
. If I go with the former, the right-edge option might make more sense.
In my opinion, dropping org-element
would be a bad move... It's your package, though, of course.
A couple of thoughts. org-element
is pretty much aware of these blank spaces it may include in the :begin
and :end
return values. It is not homogeneous in what keys it returns for different elements except for a short list, but it frequently returns keys such as :contents-begin
, :contents-end
, and :post-blank
. Those three are certainly available for "bold" (just checked), I'd presume then that the same would be true for other emphasis markers. I don't know if the same is the case for other element types org-appear
supports. So there's a good chance already gives you everything org-appear
needs.
But, even if that's not the case, you can always check the whitespace on your end. For example, suppose org-element
gives you
a region between A
and B
:
(save-excursion
(goto-char B)
(skip-chars-backward "[ \t\n]" A)
(point))
Should give you a corrected B'
. Of course, the less org-appear
has to resort to this kind of procedure, the better.
Finally, controlling for the right-edge should not be complicated regardless of the method you choose to identify the elements boundaries or of whether these include or not the last character.
Finally, thank you for your elaborate thoughts on nesting! Do you think it would be better if org-appear had an option that would allow the user to choose the way children elements are toggled?
I think that would be welcome, but not critical. Assuming whatever needs to be done for this would come cheap in terms of performance, that is. It's really up to you. And you can always keep it in mind and see if there's real demand for this and, if there is, come back to it further down the road.
The font-lock-ensure
solution appears to be working well and no changes to it are expected in the near future. As such, I am closing this issue.
Org mode fontifies text after each insertion or deletion. This conflicts with the package, causing invisible parts of fragments to disappear, even though the cursor is still inside. I can't currently think of a good solution for this. Some notes:
sit-for
) but it looks awful -- invisible parts hidden and redrawn on every insert/delete command;inhibit-modification-hooks
;