* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
@ 2016-10-02 4:56 Leo Liu
2016-10-02 20:35 ` Stefan Monnier
0 siblings, 1 reply; 28+ messages in thread
From: Leo Liu @ 2016-10-02 4:56 UTC (permalink / raw)
To: 24585; +Cc: Stefan Monnier
,----[ Stefan Monnier ]
| > + ;; Manually run the `compilation-auto-jump' timer. Hackish but
| > + ;; everything else seems unreliable. See:
| > + ;;
| > + ;; - http://debbugs.gnu.org/13829
| > + ;; - http://debbugs.gnu.org/23987
| > + ;; - https://github.com/leoliu/ggtags/issues/89
| > + ;;
| > + (pcase (cl-find 'compilation-auto-jump timer-list :key #'timer--function)
| > + (`nil )
| > + (timer (timer-event-handler timer)))
|
| I think this deserves a bug report, where we try to figure out how to
| make it possible for ggtags.el to work properly without resorting to such
| a hideous and brittle hack.
|
| A hack will probably have to stay for compatibility with older Emacsen,
| but we should come up with a better solution for the future.
`----
The hack is due to difficulty in making sure one timer is run before
another that run nearly at the same time. And it's made harder because
timer.el prepends a timer to existing timers running at the same time,
which looks like a mistake.
I think the hack in ggtags.el may be removed by the following patch to
correct the said mistake:
diff --git a/lisp/emacs-lisp/timer.el b/lisp/emacs-lisp/timer.el
index c01ea497..337e1049 100644
--- a/lisp/emacs-lisp/timer.el
+++ b/lisp/emacs-lisp/timer.el
@@ -130,9 +130,9 @@ floating point number."
(setq delta (time-add delta (list 0 0 (or usecs 0) (or psecs 0)))))
(time-add time delta)))
-(defun timer--time-less-p (t1 t2)
+(defun timer--time-less-or-equal-p (t1 t2)
"Say whether time value T1 is less than time value T2."
- (time-less-p (timer--time t1) (timer--time t2)))
+ (not (time-less-p (timer--time t2) (timer--time t1))))
(defun timer-inc-time (timer secs &optional usecs psecs)
"Increment the time set in TIMER by SECS seconds, USECS microseconds,
@@ -172,7 +172,7 @@ fire repeatedly that many seconds apart."
(let ((timers (if idle timer-idle-list timer-list))
last)
;; Skip all timers to trigger before the new one.
- (while (and timers (timer--time-less-p (car timers) timer))
+ (while (and timers (timer--time-less-or-equal-p (car timers) timer))
(setq last timers
timers (cdr timers)))
(if reuse-cell
^ permalink raw reply related [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-02 4:56 bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer Leo Liu
@ 2016-10-02 20:35 ` Stefan Monnier
2016-10-03 3:01 ` Leo Liu
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2016-10-02 20:35 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585
> I think the hack in ggtags.el may be removed by the following patch to
> correct the said mistake:
IIUC this patch just changes the ordering for same-time timers.
Relying on either ordering is itself a hack. We need a better solution.
What are those two timers whose relative execution order matters?
Why do they care in which order they're run?
Stefan
> diff --git a/lisp/emacs-lisp/timer.el b/lisp/emacs-lisp/timer.el
> index c01ea497..337e1049 100644
> --- a/lisp/emacs-lisp/timer.el
> +++ b/lisp/emacs-lisp/timer.el
> @@ -130,9 +130,9 @@ floating point number."
> (setq delta (time-add delta (list 0 0 (or usecs 0) (or psecs 0)))))
> (time-add time delta)))
> -(defun timer--time-less-p (t1 t2)
> +(defun timer--time-less-or-equal-p (t1 t2)
> "Say whether time value T1 is less than time value T2."
> - (time-less-p (timer--time t1) (timer--time t2)))
> + (not (time-less-p (timer--time t2) (timer--time t1))))
> (defun timer-inc-time (timer secs &optional usecs psecs)
> "Increment the time set in TIMER by SECS seconds, USECS microseconds,
> @@ -172,7 +172,7 @@ fire repeatedly that many seconds apart."
> (let ((timers (if idle timer-idle-list timer-list))
> last)
> ;; Skip all timers to trigger before the new one.
> - (while (and timers (timer--time-less-p (car timers) timer))
> + (while (and timers (timer--time-less-or-equal-p (car timers) timer))
> (setq last timers
> timers (cdr timers)))
> (if reuse-cell
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-02 20:35 ` Stefan Monnier
@ 2016-10-03 3:01 ` Leo Liu
2016-10-03 13:24 ` Stefan Monnier
0 siblings, 1 reply; 28+ messages in thread
From: Leo Liu @ 2016-10-03 3:01 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585
On 2016-10-02 16:35 -0400, Stefan Monnier wrote:
> IIUC this patch just changes the ordering for same-time timers.
> Relying on either ordering is itself a hack. We need a better solution.
If you eval (progn (run-with-timer 0 ...) ; timer 1
(run-with-timer 0 ...)) ; timer 2
you expect timer 1 to be triggered first, no?
> What are those two timers whose relative execution order matters?
> Why do they care in which order they're run?
The first timer is compilation-auto-jump which is installed (by compile)
at the start of compilation.
The second timer is a cleanup timer which is installed (by ggtags) when
compilation finishes and there is 0 or 1 match.
The second timer kills the buffer (among other things) that the first
timer depends on.
Leo
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-03 3:01 ` Leo Liu
@ 2016-10-03 13:24 ` Stefan Monnier
2016-10-03 15:22 ` Leo Liu
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2016-10-03 13:24 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585
>> IIUC this patch just changes the ordering for same-time timers.
>> Relying on either ordering is itself a hack. We need a better solution.
> If you eval (progn (run-with-timer 0 ...) ; timer 1
> (run-with-timer 0 ...)) ; timer 2
> you expect timer 1 to be triggered first, no?
Not really. I think any code which relies on such a property will
sooner suffer.
>> What are those two timers whose relative execution order matters?
>> Why do they care in which order they're run?
> The first timer is compilation-auto-jump which is installed (by compile)
> at the start of compilation.
> The second timer is a cleanup timer which is installed (by ggtags) when
> compilation finishes and there is 0 or 1 match.
> The second timer kills the buffer (among other things) that the first
> timer depends on.
So we can fix the bug by making the timer's code check if the buffer is
still live?
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-03 13:24 ` Stefan Monnier
@ 2016-10-03 15:22 ` Leo Liu
2016-10-03 18:45 ` Stefan Monnier
0 siblings, 1 reply; 28+ messages in thread
From: Leo Liu @ 2016-10-03 15:22 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585
On 2016-10-03 09:24 -0400, Stefan Monnier wrote:
>> The first timer is compilation-auto-jump which is installed (by compile)
>> at the start of compilation.
>> The second timer is a cleanup timer which is installed (by ggtags) when
>> compilation finishes and there is 0 or 1 match.
>> The second timer kills the buffer (among other things) that the first
>> timer depends on.
>
> So we can fix the bug by making the timer's code check if the buffer is
> still live?
No. We still want compilation-auto-jump to happen first. Otherwise we
just find 1 match and clean up without jumping to the match.
Leo
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-03 15:22 ` Leo Liu
@ 2016-10-03 18:45 ` Stefan Monnier
2016-10-04 13:08 ` Leo Liu
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2016-10-03 18:45 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585
>>> The first timer is compilation-auto-jump which is installed (by compile)
>>> at the start of compilation.
>>> The second timer is a cleanup timer which is installed (by ggtags) when
>>> compilation finishes and there is 0 or 1 match.
>>> The second timer kills the buffer (among other things) that the first
>>> timer depends on.
>> So we can fix the bug by making the timer's code check if the buffer is
>> still live?
> No. We still want compilation-auto-jump to happen first. Otherwise we
> just find 1 match and clean up without jumping to the match.
Then why does the second timer kill the buffer so hastily?
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-03 18:45 ` Stefan Monnier
@ 2016-10-04 13:08 ` Leo Liu
2016-10-04 16:18 ` Stefan Monnier
0 siblings, 1 reply; 28+ messages in thread
From: Leo Liu @ 2016-10-04 13:08 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585
On 2016-10-03 14:45 -0400, Stefan Monnier wrote:
> Then why does the second timer kill the buffer so hastily?
>
>
> Stefan
ggtags-navigation-mode-cleanup unhides the window popped up by
compilation-auto-jump. If we postpone it we see the window configuration
on the frame messed up, which is kinda annoying.
Leo
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-04 13:08 ` Leo Liu
@ 2016-10-04 16:18 ` Stefan Monnier
2016-10-05 7:39 ` Leo Liu
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2016-10-04 16:18 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585
>> Then why does the second timer kill the buffer so hastily?
> ggtags-navigation-mode-cleanup unhides the window popped up by
> compilation-auto-jump. If we postpone it we see the window configuration
> on the frame messed up, which is kinda annoying.
That would explain why you want to remove the buffer from specific
windows (i.e. hide the buffer), but not why you need to kill it.
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-04 16:18 ` Stefan Monnier
@ 2016-10-05 7:39 ` Leo Liu
2016-10-05 10:25 ` Eli Zaretskii
0 siblings, 1 reply; 28+ messages in thread
From: Leo Liu @ 2016-10-05 7:39 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585
On 2016-10-04 12:18 -0400, Stefan Monnier wrote:
> That would explain why you want to remove the buffer from specific
> windows (i.e. hide the buffer), but not why you need to kill it.
The buffer is not needed by ggtags so kill was chosen. I could change it
to not kill but this won't fix the crux of the issue i.e.
If compilation-auto-jump is run after cleanup it will mess up things
for example pop up windows.
Leo
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-05 7:39 ` Leo Liu
@ 2016-10-05 10:25 ` Eli Zaretskii
2016-10-06 16:12 ` Leo Liu
0 siblings, 1 reply; 28+ messages in thread
From: Eli Zaretskii @ 2016-10-05 10:25 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585, monnier
> From: Leo Liu <sdl.web@gmail.com>
> Date: Wed, 05 Oct 2016 15:39:02 +0800
> Cc: 24585@debbugs.gnu.org
>
> On 2016-10-04 12:18 -0400, Stefan Monnier wrote:
> > That would explain why you want to remove the buffer from specific
> > windows (i.e. hide the buffer), but not why you need to kill it.
>
> The buffer is not needed by ggtags so kill was chosen. I could change it
> to not kill but this won't fix the crux of the issue i.e.
>
> If compilation-auto-jump is run after cleanup it will mess up things
> for example pop up windows.
Instead of adding another timer, and then dealing with their order
issues, is it possible to reuse the same time for both jobs? IOW,
just _add_ the new/additional function you need to be done to whatever
the single timer does. If you do that, you get to control the order
in which things are done by the single timer, because you write the
code of the timer function(s).
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-05 10:25 ` Eli Zaretskii
@ 2016-10-06 16:12 ` Leo Liu
2016-10-06 18:10 ` Stefan Monnier
0 siblings, 1 reply; 28+ messages in thread
From: Leo Liu @ 2016-10-06 16:12 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 24585, monnier
On 2016-10-05 13:25 +0300, Eli Zaretskii wrote:
> Instead of adding another timer, and then dealing with their order
> issues, is it possible to reuse the same time for both jobs? IOW,
> just _add_ the new/additional function you need to be done to whatever
> the single timer does. If you do that, you get to control the order
> in which things are done by the single timer, because you write the
> code of the timer function(s).
probably not.
ggtags-global-handle-exit needs to consider these situations:
1. when matches are large jump to first match should not wait until
compilation finishes
2. on failure keep the compilation window popped-up so users can see the
errors
3. 0 matches, nothing to show so clean up
4. 1 match, wait until compilation-auto-jump and clean up
A solution has to cover all of them. I think this could work:
1. In compilation-error-properties save the jump pos like this
(setq-local compilation-auto-jump-pos (match-beginning 0))
(run-with-timer 0 nil 'compilation-auto-jump
(current-buffer) compilation-auto-jump-pos)
2. Make compilation-auto-jump do nothing if the buffer is killed.
WDYT?
Leo
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-06 16:12 ` Leo Liu
@ 2016-10-06 18:10 ` Stefan Monnier
2016-10-06 18:31 ` Leo Liu
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2016-10-06 18:10 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585
> 2. Make compilation-auto-jump do nothing if the buffer is killed.
> WDYT?
I thought that's what I had proposed, so yes, I think that's
a good solution.
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-06 18:10 ` Stefan Monnier
@ 2016-10-06 18:31 ` Leo Liu
2016-10-06 18:37 ` Eli Zaretskii
0 siblings, 1 reply; 28+ messages in thread
From: Leo Liu @ 2016-10-06 18:31 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585
On 2016-10-06 14:10 -0400, Stefan Monnier wrote:
> I thought that's what I had proposed, so yes, I think that's
> a good solution.
By exposing the position I can change ggtags not to depend on the order
of auto-jump and clean up.
The patch could look like this. Any objection to installing it in
emacs-25?
diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index f2e397a4..a7804fd7 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -1052,13 +1052,16 @@ POS and RES.")
l2
(setcdr l1 (cons (list ,key) l2)))))))
+(defvar-local compilation-auto-jump-pos nil)
+
(defun compilation-auto-jump (buffer pos)
- (with-current-buffer buffer
- (goto-char pos)
- (let ((win (get-buffer-window buffer 0)))
- (if win (set-window-point win pos)))
- (if compilation-auto-jump-to-first-error
- (compile-goto-error))))
+ (when (buffer-live-p buffer)
+ (with-current-buffer buffer
+ (goto-char pos)
+ (let ((win (get-buffer-window buffer 0)))
+ (if win (set-window-point win pos)))
+ (if compilation-auto-jump-to-first-error
+ (compile-goto-error)))))
;; This function is the central driver, called when font-locking to gather
;; all information needed to later jump to corresponding source code.
@@ -1126,8 +1129,9 @@ POS and RES.")
(when (and compilation-auto-jump-to-next
(>= type compilation-skip-threshold))
(kill-local-variable 'compilation-auto-jump-to-next)
+ (setq compilation-auto-jump-pos (match-beginning 0))
(run-with-timer 0 nil 'compilation-auto-jump
- (current-buffer) (match-beginning 0)))
+ (current-buffer) compilation-auto-jump-pos))
(compilation-internal-error-properties
file line end-line col end-col type fmt)))
^ permalink raw reply related [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-06 18:31 ` Leo Liu
@ 2016-10-06 18:37 ` Eli Zaretskii
2016-10-07 1:21 ` Leo Liu
0 siblings, 1 reply; 28+ messages in thread
From: Eli Zaretskii @ 2016-10-06 18:37 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585, monnier
> From: Leo Liu <sdl.web@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 24585@debbugs.gnu.org
> Date: Fri, 07 Oct 2016 02:31:39 +0800
>
> The patch could look like this. Any objection to installing it in
> emacs-25?
That depends on how long is this problem present. Can you tell?
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-06 18:37 ` Eli Zaretskii
@ 2016-10-07 1:21 ` Leo Liu
2016-10-07 2:27 ` Leo Liu
2016-10-07 7:59 ` Eli Zaretskii
0 siblings, 2 replies; 28+ messages in thread
From: Leo Liu @ 2016-10-07 1:21 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 24585, monnier
On 2016-10-06 21:37 +0300, Eli Zaretskii wrote:
> That depends on how long is this problem present. Can you tell?
Originally I thought by inserting a (sit-for 0) I fixed the problem but
people have reported otherwise
https://github.com/leoliu/ggtags/issues/89 (which is 20 months old)
Leo
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-07 1:21 ` Leo Liu
@ 2016-10-07 2:27 ` Leo Liu
2016-10-07 8:02 ` Eli Zaretskii
2016-10-07 12:46 ` Stefan Monnier
2016-10-07 7:59 ` Eli Zaretskii
1 sibling, 2 replies; 28+ messages in thread
From: Leo Liu @ 2016-10-07 2:27 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 24585, monnier
On 2016-10-07 09:21 +0800, Leo Liu wrote:
> On 2016-10-06 21:37 +0300, Eli Zaretskii wrote:
>> That depends on how long is this problem present. Can you tell?
>
> Originally I thought by inserting a (sit-for 0) I fixed the problem but
> people have reported otherwise
> https://github.com/leoliu/ggtags/issues/89 (which is 20 months old)
>
> Leo
After some experiments the proposed patch might not work at all.
compilation-auto-jump timer is fired by the font-locking engine. By my
observation it is possible that compilation-finish-functions (from a
process sentinel) are executed before font-locking even by inserting
(compilation--ensure-parse (point-max))
in ggtags-global-handle-exit.
Do you see any anomaly in the observation?
The hack that is now in ggtags.el may not work either. None of the
people that reported https://github.com/leoliu/ggtags/issues/89 have
confirmed the fix. And I personally don't know how to reproduce it.
Leo
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-07 2:27 ` Leo Liu
@ 2016-10-07 8:02 ` Eli Zaretskii
2016-10-07 12:46 ` Stefan Monnier
1 sibling, 0 replies; 28+ messages in thread
From: Eli Zaretskii @ 2016-10-07 8:02 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585, monnier
> From: Leo Liu <sdl.web@gmail.com>
> Cc: 24585@debbugs.gnu.org, monnier@IRO.UMontreal.CA
> Date: Fri, 07 Oct 2016 10:27:49 +0800
>
> compilation-auto-jump timer is fired by the font-locking engine. By my
> observation it is possible that compilation-finish-functions (from a
> process sentinel) are executed before font-locking even by inserting
>
> (compilation--ensure-parse (point-max))
>
> in ggtags-global-handle-exit.
>
> Do you see any anomaly in the observation?
Not in these general terms, no.
> The hack that is now in ggtags.el may not work either. None of the
> people that reported https://github.com/leoliu/ggtags/issues/89 have
> confirmed the fix. And I personally don't know how to reproduce it.
So what is your proposal?
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-07 2:27 ` Leo Liu
2016-10-07 8:02 ` Eli Zaretskii
@ 2016-10-07 12:46 ` Stefan Monnier
2016-10-07 17:07 ` Leo Liu
1 sibling, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2016-10-07 12:46 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585
> compilation-auto-jump timer is fired by the font-locking engine.
Not sure what you mean by "fired", but in any case, no: font-lock is not
used for that (tho it has been used at some point before
compilation-auto-jump was introduced, IIRC). Instead, it's done via
syntax-propertize (which can be triggered in all kinds of ways,
including font-lock).
How 'bout something like the following:
- Add a new var compilation-pending-auto-jump set buffer-locally to
non-nil when compilation-error-properties calls run-with-timer.
- in compilation-auto-jump, check this var before doing anything and set
it back to nil.
- in ggtags, call compilation-auto-jump to make sure this timer is run
before yours.
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-07 12:46 ` Stefan Monnier
@ 2016-10-07 17:07 ` Leo Liu
2016-10-07 18:10 ` Stefan Monnier
0 siblings, 1 reply; 28+ messages in thread
From: Leo Liu @ 2016-10-07 17:07 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585
On 2016-10-07 08:46 -0400, Stefan Monnier wrote:
> Not sure what you mean by "fired", but in any case, no: font-lock is not
> used for that (tho it has been used at some point before
> compilation-auto-jump was introduced, IIRC). Instead, it's done via
> syntax-propertize (which can be triggered in all kinds of ways,
> including font-lock).
This
(defun compilation-mode-font-lock-keywords ()
"Return expressions to highlight in Compilation mode."
(append
'((compilation--ensure-parse))
compilation-mode-font-lock-keywords))
and the only place in compile.el that mentions syntax-propertize is in a
comment.
> How 'bout something like the following:
>
> - Add a new var compilation-pending-auto-jump set buffer-locally to
> non-nil when compilation-error-properties calls run-with-timer.
> - in compilation-auto-jump, check this var before doing anything and set
> it back to nil.
> - in ggtags, call compilation-auto-jump to make sure this timer is run
> before yours.
I think the issue is compilation-error-properties can happen after
compilation-finish-functions. And calling compilation--ensure-parse in
ggtags-global-handle-exit doesn't seem to help.
Leo
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-07 17:07 ` Leo Liu
@ 2016-10-07 18:10 ` Stefan Monnier
2016-10-08 18:10 ` Leo Liu
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2016-10-07 18:10 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585
>> Not sure what you mean by "fired", but in any case, no: font-lock is not
>> used for that (tho it has been used at some point before
>> compilation-auto-jump was introduced, IIRC). Instead, it's done via
>> syntax-propertize (which can be triggered in all kinds of ways,
>> including font-lock).
>
> This
>
> (defun compilation-mode-font-lock-keywords ()
> "Return expressions to highlight in Compilation mode."
> (append
> '((compilation--ensure-parse))
> compilation-mode-font-lock-keywords))
>
> and the only place in compile.el that mentions syntax-propertize is in a
> comment.
Duh! Indeed, sorry, I mis-remembered. Indeed, after first (ab)using
syntax-propertize I changed it to use the same approach but without
using syntax-propertize.
The point is the same, tho: the parse can be triggered by other things
than font-lock. That's important because font-lock may be turned off.
>> How 'bout something like the following:
>>
>> - Add a new var compilation-pending-auto-jump set buffer-locally to
>> non-nil when compilation-error-properties calls run-with-timer.
>> - in compilation-auto-jump, check this var before doing anything and set
>> it back to nil.
>> - in ggtags, call compilation-auto-jump to make sure this timer is run
>> before yours.
> I think the issue is compilation-error-properties can happen after
> compilation-finish-functions. And calling compilation--ensure-parse in
> ggtags-global-handle-exit doesn't seem to help.
Hmm... calling compilation--ensure-parse *should* help.
More specifically, if you call compilation--ensure-parse up to point-max
from compilation-finish-functions, I can't think of any reason why
compilation-error-properties should be called afterwards. If you can
get a backtrace of when that happens, it would help.
E.g. set a buffer-local var from compilation-finish-functions after
calling compilation--ensure-parse, and then add an assert in
compilation-error-properties to check that that var is not set yes.
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-07 18:10 ` Stefan Monnier
@ 2016-10-08 18:10 ` Leo Liu
2020-09-04 12:47 ` Lars Ingebrigtsen
0 siblings, 1 reply; 28+ messages in thread
From: Leo Liu @ 2016-10-08 18:10 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585
On 2016-10-07 14:10 -0400, Stefan Monnier wrote:
> Hmm... calling compilation--ensure-parse *should* help.
> More specifically, if you call compilation--ensure-parse up to point-max
> from compilation-finish-functions, I can't think of any reason why
> compilation-error-properties should be called afterwards. If you can
> get a backtrace of when that happens, it would help.
>
> E.g. set a buffer-local var from compilation-finish-functions after
> calling compilation--ensure-parse, and then add an assert in
> compilation-error-properties to check that that var is not set yes.
To the best of my knowledge that seems to work with change like this to
compile.el:
diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index f2e397a4..3a4f4b8b 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -1052,13 +1052,18 @@ POS and RES.")
l2
(setcdr l1 (cons (list ,key) l2)))))))
-(defun compilation-auto-jump (buffer pos)
- (with-current-buffer buffer
- (goto-char pos)
- (let ((win (get-buffer-window buffer 0)))
- (if win (set-window-point win pos)))
- (if compilation-auto-jump-to-first-error
- (compile-goto-error))))
+(defvar-local compilation-pending-auto-jump nil)
+
+(defun compilation-auto-jump (buffer)
+ (when (buffer-live-p buffer)
+ (with-current-buffer buffer
+ (when compilation-pending-auto-jump
+ (goto-char compilation-pending-auto-jump)
+ (let ((win (get-buffer-window buffer 0)))
+ (if win (set-window-point win compilation-pending-auto-jump)))
+ (setq compilation-pending-auto-jump nil)
+ (if compilation-auto-jump-to-first-error
+ (compile-goto-error))))))
;; This function is the central driver, called when font-locking to gather
;; all information needed to later jump to corresponding source code.
@@ -1126,8 +1131,8 @@ POS and RES.")
(when (and compilation-auto-jump-to-next
(>= type compilation-skip-threshold))
(kill-local-variable 'compilation-auto-jump-to-next)
- (run-with-timer 0 nil 'compilation-auto-jump
- (current-buffer) (match-beginning 0)))
+ (setq compilation-pending-auto-jump (match-beginning 0))
+ (run-with-timer 0 nil 'compilation-auto-jump (current-buffer)))
(compilation-internal-error-properties
file line end-line col end-col type fmt)))
^ permalink raw reply related [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-08 18:10 ` Leo Liu
@ 2020-09-04 12:47 ` Lars Ingebrigtsen
2020-09-04 14:29 ` Stefan Monnier
0 siblings, 1 reply; 28+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-04 12:47 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585, Stefan Monnier
Leo Liu <sdl.web@gmail.com> writes:
> On 2016-10-07 14:10 -0400, Stefan Monnier wrote:
>> Hmm... calling compilation--ensure-parse *should* help.
>> More specifically, if you call compilation--ensure-parse up to point-max
>> from compilation-finish-functions, I can't think of any reason why
>> compilation-error-properties should be called afterwards. If you can
>> get a backtrace of when that happens, it would help.
>>
>> E.g. set a buffer-local var from compilation-finish-functions after
>> calling compilation--ensure-parse, and then add an assert in
>> compilation-error-properties to check that that var is not set yes.
>
> To the best of my knowledge that seems to work with change like this to
> compile.el:
[...]
This was the final message in this bug report, but looking at the patch
that supposedly fixes the problem, I don't understand how it makes a
difference: Putting (match-beginning) in a buffer-local variable
instead of passing it in directly seems like a no-op?
Checking whether the buffer is alive in compilation-auto-jump seems like
a good idea, though (any async function that switches to a buffer should
check whether it's alive first).
> -(defun compilation-auto-jump (buffer pos)
> - (with-current-buffer buffer
> - (goto-char pos)
> - (let ((win (get-buffer-window buffer 0)))
> - (if win (set-window-point win pos)))
> - (if compilation-auto-jump-to-first-error
> - (compile-goto-error))))
> +(defvar-local compilation-pending-auto-jump nil)
> +
> +(defun compilation-auto-jump (buffer)
> + (when (buffer-live-p buffer)
> + (with-current-buffer buffer
> + (when compilation-pending-auto-jump
> + (goto-char compilation-pending-auto-jump)
> + (let ((win (get-buffer-window buffer 0)))
> + (if win (set-window-point win compilation-pending-auto-jump)))
> + (setq compilation-pending-auto-jump nil)
> + (if compilation-auto-jump-to-first-error
> + (compile-goto-error))))))
>
> ;; This function is the central driver, called when font-locking to gather
> ;; all information needed to later jump to corresponding source code.
> @@ -1126,8 +1131,8 @@ POS and RES.")
> (when (and compilation-auto-jump-to-next
> (>= type compilation-skip-threshold))
> (kill-local-variable 'compilation-auto-jump-to-next)
> - (run-with-timer 0 nil 'compilation-auto-jump
> - (current-buffer) (match-beginning 0)))
> + (setq compilation-pending-auto-jump (match-beginning 0))
> + (run-with-timer 0 nil 'compilation-auto-jump (current-buffer)))
>
> (compilation-internal-error-properties
> file line end-line col end-col type fmt)))
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2020-09-04 12:47 ` Lars Ingebrigtsen
@ 2020-09-04 14:29 ` Stefan Monnier
2020-09-04 15:02 ` Lars Ingebrigtsen
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2020-09-04 14:29 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 24585, Leo Liu
> This was the final message in this bug report, but looking at the patch
> that supposedly fixes the problem, I don't understand how it makes a
> difference: Putting (match-beginning) in a buffer-local variable
> instead of passing it in directly seems like a no-op?
This was too long ago and got moved to write-only swap space, and
I haven't had time to dig into it now to recover the lost context, but
I can see a potential case where it makes a difference, which is if the
timer is set multiple times before it gets a chance to be run: with the
new code only one of the timers will actually be executed.
> Checking whether the buffer is alive in compilation-auto-jump seems like
> a good idea, though (any async function that switches to a buffer should
> check whether it's alive first).
Indeed,
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2020-09-04 14:29 ` Stefan Monnier
@ 2020-09-04 15:02 ` Lars Ingebrigtsen
2020-09-04 15:45 ` Stefan Monnier
0 siblings, 1 reply; 28+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-04 15:02 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585, Leo Liu
Stefan Monnier <monnier@iro.umontreal.ca> writes:
> This was too long ago and got moved to write-only swap space, and
> I haven't had time to dig into it now to recover the lost context, but
> I can see a potential case where it makes a difference, which is if the
> timer is set multiple times before it gets a chance to be run: with the
> new code only one of the timers will actually be executed.
That's true; I didn't think about that. But wouldn't a more obvious fix
be to cancel the previous timer, if there's already a timer in-flight?
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2020-09-04 15:02 ` Lars Ingebrigtsen
@ 2020-09-04 15:45 ` Stefan Monnier
2020-09-05 12:32 ` Lars Ingebrigtsen
0 siblings, 1 reply; 28+ messages in thread
From: Stefan Monnier @ 2020-09-04 15:45 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: 24585, Leo Liu
>> This was too long ago and got moved to write-only swap space, and
>> I haven't had time to dig into it now to recover the lost context, but
>> I can see a potential case where it makes a difference, which is if the
>> timer is set multiple times before it gets a chance to be run: with the
>> new code only one of the timers will actually be executed.
>
> That's true; I didn't think about that. But wouldn't a more obvious fix
> be to cancel the previous timer, if there's already a timer in-flight?
Could be. I don't even know if this effect was the one intended in
any case.
Stefan
^ permalink raw reply [flat|nested] 28+ messages in thread* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2020-09-04 15:45 ` Stefan Monnier
@ 2020-09-05 12:32 ` Lars Ingebrigtsen
2020-10-07 3:30 ` Lars Ingebrigtsen
0 siblings, 1 reply; 28+ messages in thread
From: Lars Ingebrigtsen @ 2020-09-05 12:32 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585, Leo Liu
Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> This was too long ago and got moved to write-only swap space, and
>>> I haven't had time to dig into it now to recover the lost context, but
>>> I can see a potential case where it makes a difference, which is if the
>>> timer is set multiple times before it gets a chance to be run: with the
>>> new code only one of the timers will actually be executed.
>>
>> That's true; I didn't think about that. But wouldn't a more obvious fix
>> be to cancel the previous timer, if there's already a timer in-flight?
>
> Could be. I don't even know if this effect was the one intended in
> any case.
Me neither. I've now applied the
+ (when (buffer-live-p buffer)
part of the patch, but not the rest. Perhaps Leo can chime in with that
the patch was trying to achieve.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2020-09-05 12:32 ` Lars Ingebrigtsen
@ 2020-10-07 3:30 ` Lars Ingebrigtsen
0 siblings, 0 replies; 28+ messages in thread
From: Lars Ingebrigtsen @ 2020-10-07 3:30 UTC (permalink / raw)
To: Stefan Monnier; +Cc: 24585, Leo Liu
Lars Ingebrigtsen <larsi@gnus.org> writes:
> Me neither. I've now applied the
>
> + (when (buffer-live-p buffer)
>
> part of the patch, but not the rest. Perhaps Leo can chime in with that
> the patch was trying to achieve.
This was a month ago, but there was no response, so I'm closing this bug
report. If there's more to be done here, a new bug report could perhaps
be opened.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 28+ messages in thread
* bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer
2016-10-07 1:21 ` Leo Liu
2016-10-07 2:27 ` Leo Liu
@ 2016-10-07 7:59 ` Eli Zaretskii
1 sibling, 0 replies; 28+ messages in thread
From: Eli Zaretskii @ 2016-10-07 7:59 UTC (permalink / raw)
To: Leo Liu; +Cc: 24585, monnier
> From: Leo Liu <sdl.web@gmail.com>
> Cc: 24585@debbugs.gnu.org, monnier@IRO.UMontreal.CA
> Date: Fri, 07 Oct 2016 09:21:12 +0800
>
> On 2016-10-06 21:37 +0300, Eli Zaretskii wrote:
> > That depends on how long is this problem present. Can you tell?
>
> Originally I thought by inserting a (sit-for 0) I fixed the problem but
> people have reported otherwise
> https://github.com/leoliu/ggtags/issues/89 (which is 20 months old)
Thanks. That's when the problem was reported, not when it was
introduced. However, it does mean the problem existed even before Feb
2015, which means it existed in Emacs 24.5.
OTOH, the fix seems quite safe. So yes, please push to emacs-25.
Thanks.
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2020-10-07 3:30 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-02 4:56 bug#24585: 25.1; avoid hack in ggtags.el to run compilation-auto-jump timer Leo Liu
2016-10-02 20:35 ` Stefan Monnier
2016-10-03 3:01 ` Leo Liu
2016-10-03 13:24 ` Stefan Monnier
2016-10-03 15:22 ` Leo Liu
2016-10-03 18:45 ` Stefan Monnier
2016-10-04 13:08 ` Leo Liu
2016-10-04 16:18 ` Stefan Monnier
2016-10-05 7:39 ` Leo Liu
2016-10-05 10:25 ` Eli Zaretskii
2016-10-06 16:12 ` Leo Liu
2016-10-06 18:10 ` Stefan Monnier
2016-10-06 18:31 ` Leo Liu
2016-10-06 18:37 ` Eli Zaretskii
2016-10-07 1:21 ` Leo Liu
2016-10-07 2:27 ` Leo Liu
2016-10-07 8:02 ` Eli Zaretskii
2016-10-07 12:46 ` Stefan Monnier
2016-10-07 17:07 ` Leo Liu
2016-10-07 18:10 ` Stefan Monnier
2016-10-08 18:10 ` Leo Liu
2020-09-04 12:47 ` Lars Ingebrigtsen
2020-09-04 14:29 ` Stefan Monnier
2020-09-04 15:02 ` Lars Ingebrigtsen
2020-09-04 15:45 ` Stefan Monnier
2020-09-05 12:32 ` Lars Ingebrigtsen
2020-10-07 3:30 ` Lars Ingebrigtsen
2016-10-07 7:59 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).