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

26 messages
12
Open this post in threaded view
|

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

 > 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))))))))))))
Open this post in threaded view
|

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

 >>     * 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
Open this post in threaded view
|

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

 > 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?
Open this post in threaded view
|

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

 >> >>     * 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
Open this post in threaded view
|

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

 > 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".
Open this post in threaded view
|

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

 > 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
Open this post in threaded view
|

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

 > 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.
Open this post in threaded view
|

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

 >> 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
Open this post in threaded view
|

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

 In reply to this post by Stefan Monnier > From: Stefan Monnier <[hidden email]> > Date: Wed, 10 Oct 2018 15:21:27 -0400 > > > 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`). After revisiting this, the values "nil", "font-lock", and "navigation" sound fine, though if we end up using those we might want to change the name of "diff-font-lock-refine" to just "diff-refine" (since "font-lock" may not be involved in the refinement).
Open this post in threaded view
|

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

Open this post in threaded view
|

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

 > -(defcustom diff-font-lock-refine t > -  "If non-nil, font-lock highlighting includes hunk refinement." > +(defcustom diff-refine 'font-lock > +  "If non-nil, enable hunk refinement. > + > +The value `font-lock' means to refine during font-lock. > +The value `navigation' means to refine each hunk as you visit it > +with `diff-hunk-next' or `diff-hunk-prev'. > + > +You can always manually refine a hunk with `diff-refine-hunk'." >    :version "27.1" > -  :type 'boolean) > +  :type '(choice (const :tag "Don't refine hunks" nil) > +                 (const :tag "Refine hunks during font-lock" font-lock) > +                 (const :tag "Refine hunks during navigation" navigation))) Good. > -(define-minor-mode diff-auto-refine-mode > -  "Toggle automatic diff hunk finer highlighting (Diff Auto Refine mode). > - > -Diff Auto Refine mode is a buffer-local minor mode used with > -`diff-mode'.  When enabled, Emacs automatically highlights > -changes in detail as the user visits hunks.  When transitioning > -from disabled to enabled, it tries to refine the current hunk, as > -well." > -  :group 'diff-mode :init-value t :lighter nil ;; " Auto-Refine" > -  (when diff-auto-refine-mode > -    (condition-case-unless-debug nil (diff-refine-hunk) (error nil)))) I think for backward compatiblity reason, we should keep this minor mode, tho mark it obsolete and change its code to set diff-refine to `navigation`. > -;; Define diff-{hunk,file}-{prev,next} > -(easy-mmode-define-navigation > - diff-hunk diff-hunk-header-re "hunk" diff-end-of-hunk diff-restrict-view [...] > +(defun diff-hunk-next (&optional count interactive) [...] > +(defun diff-hunk-prev (&optional count interactive) This change seems unrelated.  I'd rather we stick to the refinement itself. >  ;; Define smerge-next and smerge-prev >  (easy-mmode-define-navigation smerge smerge-begin-re "conflict" nil nil > -  (if diff-auto-refine-mode > +  (if (eq diff-refine 'navigation) >        (condition-case nil (smerge-refine) (error nil)))) Hmm... I want to set my `diff-refined` to `font-lock` yet I also want my smerge conflicts to be refined when I navigate to them. Maybe the test here should be just whether diff-refine is non-nil?         Stefan
Open this post in threaded view
|

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

Open this post in threaded view
|

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

 In reply to this post by Stefan Monnier >>  ;; Define smerge-next and smerge-prev >>  (easy-mmode-define-navigation smerge smerge-begin-re "conflict" nil nil >> -  (if diff-auto-refine-mode >> +  (if (eq diff-refine 'navigation) >>        (condition-case nil (smerge-refine) (error nil)))) > > Hmm... I want to set my `diff-refined` to `font-lock` yet I also want my > smerge conflicts to be refined when I navigate to them. > Maybe the test here should be just whether diff-refine is non-nil? A similar option `font-lock` also makes sense for a new customizable variable with a name like `smerge-refine` that could automatically refine all smerge conflicts. The problem is that it's difficult to remember and type such key sequences `C-c ^ n` (smerge-next) and `C-c ^ R` (smerge-refine)  It would be easier if smerge-mode refined all them automatically.
Open this post in threaded view
|

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

 > A similar option `font-lock` also makes sense for a new > customizable variable with a name like `smerge-refine` > that could automatically refine all smerge conflicts. FWIW, here's the reason why I haven't even looked into doing it automatically for smerge: IMO 2-way conflicts are worthless so I only care about 3-way conflicts, and for those I don't know how to display all 3 different "refinements" at the same time.  IOW I too often need to use the cycling behavior of smerge-refine for a "font-lock" version to be sufficient.  So if we implement a "font-lock" version of smerge-refine, we'll need to make sure it interacts well with subsequent manual smerge-refine cycling.         Stefan
Open this post in threaded view
|

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

 > From: Stefan Monnier <[hidden email]> > Date: Fri, 01 Feb 2019 02:38:51 -0500 > > > A similar option `font-lock` also makes sense for a new > > customizable variable with a name like `smerge-refine` > > that could automatically refine all smerge conflicts. > > FWIW, here's the reason why I haven't even looked into doing it > automatically for smerge: IMO 2-way conflicts are worthless so I only > care about 3-way conflicts, and for those I don't know how to display > all 3 different "refinements" at the same time.  IOW I too often need to > use the cycling behavior of smerge-refine for a "font-lock" version to > be sufficient.  So if we implement a "font-lock" version of > smerge-refine, we'll need to make sure it interacts well with subsequent > manual smerge-refine cycling. Why are 2-way conflicts worthless?  Automatically refining them could be helpful in some situations. By the way, does the updated change I sent in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32991#38 look okay to you?
Open this post in threaded view
|

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

 > Why are 2-way conflicts worthless? Because they give less info than 3-way conflicts and they're generated from 3 files so you can "always" get the superior 3-way conflicts instead of the inferior 2-way ones. > By the way, does the updated change I sent in > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32991#38 look okay > to you? Haven't had a chance to look at it yet,         Stefan
Open this post in threaded view
|

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

 > From: Stefan Monnier <[hidden email]> > Date: Sun, 03 Feb 2019 07:37:36 -0500 > > > Why are 2-way conflicts worthless? > > Because they give less info than 3-way conflicts and they're generated > from 3 files so you can "always" get the superior 3-way conflicts > instead of the inferior 2-way ones. Oh, like the ones you can get by setting Git's "merge.conflictStyle" to "diff3".  Makes sense. > > By the way, does the updated change I sent in > > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=32991#38 look okay > > to you? > > Haven't had a chance to look at it yet, > > >         Stefan Okay, no rush.
Open this post in threaded view
|

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

 In reply to this post by Charles A. Roelli >> This change seems unrelated.  I'd rather we stick to the refinement itself. > Without this change, other functions in diff-mode (such as > diff--font-lock-syntax) calling diff-hunk-next accidentally trigger > hunk refinement if 'diff-refine' is 'navigation'. Ah, right, makes sense.  Could we fix this more directly by using `called-interactively` instead? > Incidentally, I left out the auto-recentering and buffer > restriction-changing parts of the old diff-hunk-next and > diff-hunk-prev, since these behaviors do not match other navigation > commands. Indeed, I see several changes in there, which is why I'd rather we separate them into another patch. [ FWIW I never used the restriction (I coded it up only to mach the   featureset of some earlier diff-mode I'd found somewhere), but I would   miss the recentering.  More to the point, rather than removing the   recentering, I'd like to improve it (so it tries harder to make the   whole hunk visible when possible).  ] Other than that, LGTM.         Stefan
Open this post in threaded view
|

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

 In reply to this post by Charles A. Roelli > Oh, like the ones you can get by setting Git's "merge.conflictStyle" > to "diff3".  Makes sense. Is this setting still necessary?  The mind boggles!         Stefan