Re: [PATCH] Add mode for automatically unhiding emphasis markers in the current region

[Kyle Meyer <kyle@kyleam.com>]

Thanks for the patch, and sorry for the slow reply.

Shankar Rao writes:

> Sorry, I've never submitted a patch before. Looking through this mailing
> list, I see that you're supposed to attach the .patch file to the e-mail,
> so here it is.

Inline is fine as well, though it can take a little more work on the
sender's end to make sure that the patch doesn't get mangled by their
mail client and that the commit message is formatted correctly.

> This patch adds a minor mode that makes emphasis markers be automatically
> unhidden when the point is inside the region of emphasis and then the
> markers are rehidden when the point is moved elsewhere. I posted this on
> /r/orgmode on reddit (
> https://www.reddit.com/r/orgmode/comments/gss1g4/update_i_made_my_own_sbrorgemphasizemode_that/),
> and people there suggested that I submit this here as a patch.

I don't prefer my emphasis markers hidden (i.e. I leave
org-hide-emphasis-markers at nil), so perhaps I'm not the best to judge,
but that does sound like a nice feature.

I'm hoping others will try this out and give their thoughts.

> On Mon, Jun 1, 2020 at 4:14 PM Shankar Rao <shankar.rao@gmail.com> wrote:
[...]
>>
>> This code was adapted from prettify-symbols-mode in prog-mode.el
>>
>> I have not yet signed the papers assigning copyright to the FSF.  I sent a
>> request for the papers to assign@gnu.org, but have not yet received a
>> response.

Are you still waiting for a response?

> Subject: [PATCH] Add mode for automatically unhiding emphasis markers in the
>  current region
[...]
>  lisp/org.el | 98 +++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 87 insertions(+), 11 deletions(-)
>
> diff --git a/lisp/org.el b/lisp/org.el
> index 7ff7ec685..870c5c958 100644

Note that this no longer applies to the current master due to a conflict
with b68090e0b (2020-06-04).  I applied this to b68090e0b^ to try this
out, and it seems to work as advertised.

Here are some comments from an initial read-through.

> --- a/lisp/org.el
> +++ b/lisp/org.el
> @@ -3644,6 +3644,19 @@ following symbols:
>    :type 'boolean
>    :safe #'booleanp)
>  
> +(defcustom org-auto-emphasis-unhide-at-point nil
> +  "If non-nil, show the unhide the emphasis markers for the region when point is on it.

s/show the// ?

Also, I find using "region" here (and elsewhere) a little confusing.
Perhaps something like "unhide the emphasis markers for the text at
point" instead?

> +If set to the symbol `right-edge', also unhide the emphasis
> +markers if point is immediately after the emphasized region.  The
> +emphasis markers will be rehidden as soon as point moves away
> +from the region.  If set to nil, the emphasis markers remain
> +hidden even when point is in the region."
> +  :version "25.1"

This can instead be

    :package-version '(Org . "9.5")

Emacs can then use customize-package-emacs-version-alist to map to the
Emacs version.

(The next release will be v9.4, but we're in a feature freeze at the
moment.)

> +  :type '(choice (const :tag "Never unhide emphasis markers" nil)

Hmm, I don't see the point of having a nil value.  That already seems
covered by toggling org-auto-emphasis-mode on and off.

I also wonder whether we can get away with not having an option here at
all.  Is one of the below values more likely to be the predominant
preference?

> +                 (const :tag "Unhide emphasis markers when point is inside" t)
> +                 (const :tag "Unhide emphasis markers when point is inside or at right edge" right-edge))

IMO these descriptions could be more concise but as clear by dropping
"emphasis markers" from the t and right-edge items.

> +(defvar-local org-auto-emphasis--current-region-bounds nil)
> +
> +(defun org-auto-emphasis--get-prop-as-list (prop)
> +  "Helper function to get org-auto-emphasis properties as a list.
> +If `org-auto-emphasis-unhide-at-point' is set to `t' then return

convention nit: Drop quotes from t.

> +(defun org-auto-emphasis--post-command-hook ()
> +  ;; Rehide emphasis markers for the previous region.
> +  (when (and org-auto-emphasis--current-region-bounds
> +	     (or (< (point) (car org-auto-emphasis--current-region-bounds))
> +		 (> (point) (cadr org-auto-emphasis--current-region-bounds))
> +		 (and (not (eq org-auto-emphasis-unhide-at-point 'right-edge))
> +		      (= (point) (cadr org-auto-emphasis--current-region-bounds)))))
> +	(apply #'font-lock-flush org-auto-emphasis--current-region-bounds)
> +	(setq org-auto-emphasis--current-region-bounds nil))
> +  ;; Unhide emphasis markers for the current region.
> +  (when-let* ((s (org-auto-emphasis--get-prop-as-list 'org-emph-start))

Our minimum supported Emacs version is 24.3, so we can't rely on
when-let and friends being available.

> +(define-minor-mode org-auto-emphasis-mode
> +  "Toggle Org Auto Emphasis mode.
> +This mode, when enabled, unhides emphasis markers for the region
> +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-mode files, add the following line to init.el:

nit: s/org-mode/Org/

> +
> +  (add-hook 'org-mode #'org-auto-emphasis-mode)

You should protect the apostrophes as \\=' so that they aren't rendered
according to text-quoting-style.

>  (defun org-emphasize (&optional char)
>    "Insert or change an emphasis, i.e. a font like bold or italic.
>  If there is an active region, change that region to a new emphasis.
> @@ -20482,16 +20560,15 @@ With ARG, repeats or can move backward if negative."
>  	   (beginning-of-line))
>  	  (_ nil)))
>        (cl-incf arg))
> -    (while (and (> arg 0) (re-search-forward regexp nil t))
> +    (while (and (> arg 0) (re-search-forward regexp nil :move))
>        (pcase (get-char-property-and-overlay (point) 'invisible)
>  	(`(outline . ,o)
>  	 (goto-char (overlay-end o))
> -	 (skip-chars-forward " \t\n")
> -	 (end-of-line))
> +	 (end-of-line 2))
>  	(_
>  	 (end-of-line)))
>        (cl-decf arg))
> -    (if (> arg 0) (goto-char (point-max)) (beginning-of-line))))
> +    (when (/= arg initial-arg) (beginning-of-line))))

Can you explain why this change to org-next-visible-heading and...

>  
>  (defun org-previous-visible-heading (arg)
>    "Move to the previous visible heading.
> @@ -20830,11 +20907,10 @@ ones already marked."
>  	(set-mark
>  	 (save-excursion
>  	   (goto-char (mark))
> -	   (goto-char (org-element-property :end (org-element-at-point)))
> -	   (point)))
> +	   (goto-char (org-element-property :end (org-element-at-point)))))
>        (let ((element (org-element-at-point)))
>  	(end-of-line)
> -	(push-mark (min (point-max) (org-element-property :end element)) t t)
> +	(push-mark (org-element-property :end element) t t)
>  	(goto-char (org-element-property :begin element))))))

... this change to org-mark-element are related/needed?