bug#32991: 27.0.50; diff-auto-refine-mode a no-op

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> commit f8b1e40fb63b0a6bc6692cc0bc84e5f5e65c2644
> Author: Stefan Monnier <[hidden email]>
> Date:   Tue Jul 10 22:52:21 2018 -0400

>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
   
>     Remove redundant :group arguments.
>     (diff-font-lock-refine): New var.
>     (diff--refine-hunk): New function, extracted from diff-refine-hunk.
>     (diff-refine-hunk): Use it.
>     (diff--font-lock-refine--refresh): New function.
>     (diff--font-lock-refined): New function.
>     (diff-font-lock-keywords): Use it.

Looks like this commit makes diff-auto-refine-mode a no-op, since
hunks are now always refined.  Is that the intention?

Also, with automatic refinement always on, does the following part of
diff-mode.el still require refining the hunk?  It must be repeating
font lock's job (unless font lock is switched off, of course).

  (easy-mmode-define-navigation
   diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view
   (when diff-auto-refine-mode
     (unless (prog1 diff--auto-refine-data
               (setq diff--auto-refine-data
                     (cons (current-buffer) (point-marker))))
       (run-at-time 0.0 nil
                    (lambda ()
                      (when diff--auto-refine-data
                        (let ((buffer (car diff--auto-refine-data))
                              (point (cdr diff--auto-refine-data)))
                          (setq diff--auto-refine-data nil)
                          (with-local-quit
                            (when (buffer-live-p buffer)
                              (with-current-buffer buffer
                                (save-excursion
                                  (goto-char point)
                                  (diff-refine-hunk))))))))))))



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
>>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
> Looks like this commit makes diff-auto-refine-mode a no-op, since
> hunks are now always refined.  Is that the intention?

Yes.  Of course, it depends on the value of diff-font-lock-refine.

> Also, with automatic refinement always on, does the following part of
> diff-mode.el still require refining the hunk?

I believe this is the same question.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> From: Stefan Monnier <[hidden email]>
> Date: Mon, 08 Oct 2018 17:00:31 -0400
>
> >>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
> > Looks like this commit makes diff-auto-refine-mode a no-op, since
> > hunks are now always refined.  Is that the intention?
>
> Yes.  Of course, it depends on the value of diff-font-lock-refine.

Ok.  In that case, do we still need diff-auto-refine-mode?

> > Also, with automatic refinement always on, does the following part of
> > diff-mode.el still require refining the hunk?
>
> I believe this is the same question.

I'm not sure I understand.  If the default value of
diff-font-lock-refine results in hunks being automatically refined by
font-lock, why should the navigation function possibly re-do the
refining already done by font-lock?



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
>> >>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
>> > Looks like this commit makes diff-auto-refine-mode a no-op, since
>> > hunks are now always refined.  Is that the intention?
>> Yes.  Of course, it depends on the value of diff-font-lock-refine.
> Ok.  In that case, do we still need diff-auto-refine-mode?

It should probably be merged with diff-font-lock-refine (e.g. have
3 possible values).

>> > Also, with automatic refinement always on, does the following part of
>> > diff-mode.el still require refining the hunk?
>> I believe this is the same question.
> I'm not sure I understand.

I'm saying that this is the same question as that discussed in the
previous paragraph: one looks at the config var, and the other looks at
the code implementing the functionality related to the config var, but
the two are inextricably linked.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> From: Stefan Monnier <[hidden email]>
> Date: Tue, 09 Oct 2018 15:54:55 -0400
>
> >> >>     * lisp/vc/diff-mode.el: Perform hunk refinement from font-lock
> >> > Looks like this commit makes diff-auto-refine-mode a no-op, since
> >> > hunks are now always refined.  Is that the intention?
> >> Yes.  Of course, it depends on the value of diff-font-lock-refine.
> > Ok.  In that case, do we still need diff-auto-refine-mode?
>
> It should probably be merged with diff-font-lock-refine (e.g. have
> 3 possible values).

So diff-font-lock-refine could have 3 possible values, t and nil as it
has now, and 'auto for doing the refinement as you navigate to hunks
with "n" or "p".

While we're on this point, what is the use case for offering automatic
refining during navigation if we can now offer "just-in-time"
highlighting via font-lock?  The new, instant refining seems more
logical and less surprising to the user.  It could be even better if
C-c C-b could interactively toggle the refining of the hunk at point
(for those times when the refining turns out to be an eye-sore).

Another advantage of the new font-lock based approach is that with a
little change we might be able to customize how much highlighting is
done in a diff-mode buffer via "font-lock-maximum-decoration".



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
> So diff-font-lock-refine could have 3 possible values, t and nil as it
> has now, and 'auto for doing the refinement as you navigate to hunks
> with "n" or "p".

I don't see what the navigation-triggered refinement has of
"auto"matism, compared to font-lock, so I wouldn't use `auto` here.
I'd rather go with something like `nil`, `font-lock`, or `navigation`
(and default to `font-lock`).

> While we're on this point, what is the use case for offering automatic
> refining during navigation if we can now offer "just-in-time"
> highlighting via font-lock?

Good question.  Maybe it's just not worth it and we should simply get
rid of the old navigation-triggered refinement.  This said, the new
font-lock thingy can be problematic if the diff takes too much time
since it happens within jit-lock with inhibit-quit set to a non-nil
value, so it completely freezes your Emacs session, whereas if it
happens during `n` you can stop it with C-g.

Hopefully, we can fix this problem by calling `diff` asynchronously so
it can't block Emacs.

> It could be even better if C-c C-b could interactively toggle the
> refining of the hunk at point (for those times when the refining turns
> out to be an eye-sore).

Sounds good (but note that diff-refine-hunk can also be useful to
*refresh* the fine highlighting, e.g. after manually editing a hunk).

> Another advantage of the new font-lock based approach is that with a
> little change we might be able to customize how much highlighting is
> done in a diff-mode buffer via "font-lock-maximum-decoration".

font-lock-maximum-decoration is fundamentally flawed in that it is
unidimensional (you can only have more or less) whereas often some users
may want more of one kind of info and less of another.

E.g. how would you order the "decoration levels" between:

    basic
    basic + refine
    basic + prettify
    basic + prettify + refine

The first and last are easy, but there's no natural ordering between the
middle two.


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Charles A. Roelli
> From: Stefan Monnier <[hidden email]>
> Date: Wed, 10 Oct 2018 15:21:27 -0400
>
> I don't see what the navigation-triggered refinement has of
> "auto"matism, compared to font-lock, so I wouldn't use `auto` here.
> I'd rather go with something like `nil`, `font-lock`, or `navigation`
> (and default to `font-lock`).

Sounds good.

> > While we're on this point, what is the use case for offering automatic
> > refining during navigation if we can now offer "just-in-time"
> > highlighting via font-lock?
>
> Good question.  Maybe it's just not worth it and we should simply get
> rid of the old navigation-triggered refinement.  This said, the new
> font-lock thingy can be problematic if the diff takes too much time
> since it happens within jit-lock with inhibit-quit set to a non-nil
> value, so it completely freezes your Emacs session, whereas if it
> happens during `n` you can stop it with C-g.
>
> Hopefully, we can fix this problem by calling `diff` asynchronously so
> it can't block Emacs.

I was able to produce such a case where Emacs froze for 30 seconds on
opening a 2000-line (junk) diff containing one large hunk.

It would indeed be convenient to have "diff" called asynchronously
(assuming this is the "diff" that runs in smerge-refine-regions).  Is
it a matter of using "start-process" instead of "call-process"?

> > It could be even better if C-c C-b could interactively toggle the
> > refining of the hunk at point (for those times when the refining turns
> > out to be an eye-sore).
>
> Sounds good (but note that diff-refine-hunk can also be useful to
> *refresh* the fine highlighting, e.g. after manually editing a hunk).

Ok, then switching off the refining might fit better on a prefix
argument, or in a new command.

> font-lock-maximum-decoration is fundamentally flawed in that it is
> unidimensional (you can only have more or less) whereas often some users
> may want more of one kind of info and less of another.
>
> E.g. how would you order the "decoration levels" between:
>
>     basic
>     basic + refine
>     basic + prettify
>     basic + prettify + refine
>
> The first and last are easy, but there's no natural ordering between the
> middle two.

Good point.  Maybe we could start with an ordering of just
"basic"/"basic + refine", since the diff-font-lock-prettify option is
brand new and seems to be more about hiding text than decorating it,
IIUC.



Reply | Threaded
Open this post in threaded view
|

bug#32991: 27.0.50; diff-auto-refine-mode a no-op

Stefan Monnier
>> Good question.  Maybe it's just not worth it and we should simply get
>> rid of the old navigation-triggered refinement.  This said, the new
>> font-lock thingy can be problematic if the diff takes too much time
>> since it happens within jit-lock with inhibit-quit set to a non-nil
>> value, so it completely freezes your Emacs session, whereas if it
>> happens during `n` you can stop it with C-g.
>> Hopefully, we can fix this problem by calling `diff` asynchronously so
>> it can't block Emacs.
> I was able to produce such a case where Emacs froze for 30 seconds on
> opening a 2000-line (junk) diff containing one large hunk.

I bumped into one a week ago where I killed `diff` after several minutes.

> Is it a matter of using "start-process" instead of "call-process"?

That's the starting point, yes, but it also involves moving the
"subsequent" code to the process's sentinel.

>> > It could be even better if C-c C-b could interactively toggle the
>> > refining of the hunk at point (for those times when the refining turns
>> > out to be an eye-sore).
>> Sounds good (but note that diff-refine-hunk can also be useful to
>> *refresh* the fine highlighting, e.g. after manually editing a hunk).
> Ok, then switching off the refining might fit better on a prefix
> argument, or in a new command.

Could be, tho maybe you can make it DWIM enough (i.e. turn off if
there's nothing to refresh).

> Good point.  Maybe we could start with an ordering of just
> "basic"/"basic + refine", since the diff-font-lock-prettify option is
> brand new and seems to be more about hiding text than decorating it,
> IIUC.

Since I find font-lock-maximum-decoration fundamentally flawed, I'm
rather in the business of deprecating it than increasing its use.


        Stefan