Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error highlighting gives constant negative feedback #29

Open
magnars opened this issue Jan 2, 2012 · 16 comments
Open

Error highlighting gives constant negative feedback #29

magnars opened this issue Jan 2, 2012 · 16 comments

Comments

@magnars
Copy link

magnars commented Jan 2, 2012

The constant negative feedback from the error highlighting while I'm typing is bugging me. It's like the small happy green tests in TDD - except in reverse.

I've got several ideas about fixing it, but I'm unsure of how feasible they are. I've tried making the error highlighting only show up on saved files. I thought that would be easiest, but couldn't get it to work.

What I would like most of all is this:

  • Parse errors are only highlighted in saved files, or after a long pause in typing (say 15 seconds).
  • Warnings are only highlighted after a pause in typing (say 2 seconds)

And here's the crux:

  • Fixes to errors and warnings are shown immediately (on the first parse).

Do you think these fixes are feasible? Or at all possible with the way parsing works now?

I'd love to help out, but would need a little push in the right direction. :-)

@dgutov
Copy link
Collaborator

dgutov commented Jan 2, 2012

Increasing js2-idle-timer-delay value should improve the situation, but it, of course, goes against the 3rd requirement.
By the way, why 15 seconds on parse errors? I agree that errors in red are often annoying, but a delay of 2-3 seconds, same as for warnings, should be enough, I think.

Some observations:

  1. We could try showing or delaying warnings based on whether they would be applied to "new" or "preexising" parts of the buffer. As far as I know, doing that would require extracting information from buffer-undo-list -- I don't think there is a standard convenience function that would just return a list of modified buffer regions since the buffer was last saved, for example.
  2. The change in the middle of the buffer might just as well cause a parsing error at the end of the buffer -- or right after the newly inserted text. That breaks the approach above.
  3. Comparing the new errors/warnings list with a previous one won't exactly work, because the errors below the modified area are likely to have changed buffer positions.
  4. Errors and warning are text properties plus overlays. Once applied, they should move together with the existing text.

Originally, all text properties were cleared before parsing. I moved this clearing to right before the new errors are shown, after parsing. Starting with this, I see two approaches:

  1. Remove overlays only after parsing, too, and additionally delay the clear/apply phase, using a separate timer (or, rather, clear some text properties immediately, and leave these ones). The error markings on the newly added or rewritten text would be removed, but they would still show on the surrounding and related text. There would be discrepancy between error markings and echo-help, because typing inside the "red" overlay would make the text red, but it won't have the "hint".
  2. Before clearing the properties, walk through the new errors and warnings lists, and for each entry check if the buffer text at the specified position already has the help-echo property with the same message, for example. Then partition each list in two by this predicate, clear the properties and overlays from the buffer (thus applying the fixes immediately), reapply the "existing" errors and warnings, and apply the rest after a delay using run-with-idle-timer (see the example in js2-mode-reset-timer).

@magnars
Copy link
Author

magnars commented Jan 2, 2012

Interesting, seems like this could work. I have another suggestions which might be simpler, tho:

How about all errors and warnings at point or beyond are delayed for 2-3 seconds while typing? I think that would take care of most issues. It's very rare that new warnings or parse errors pop up before point.

Increasing js2-idle-timer-delay value should improve the situation, but it, of course, goes against the 3rd requirement.

Yes, this is what I have been doing for a while now. It's what made me realize the 3rd requirement is so important.

By the way, why 15 seconds on parse errors?

To avoid errors while I was thinking, breaking my concentration. :-) I'm all for simplifying and using the same delay.

@dgutov
Copy link
Collaborator

dgutov commented Jan 2, 2012

Suppose we have a couple of warnings and an error in a block of code, and we decide to write something above it.
Each time we start typing, the warnings and the error would disappear, and then return with a noticeable delay after we stop.
I think it would be distracting.

The difference between checking if the warning is before the point, and checking if the text at a certain point has the same text property value, is not that big, so I'd try the latter approach first.

@memeplex
Copy link

memeplex commented Aug 4, 2014

When auto-complete is enabled it immediately triggers the error message no matter the idle timer delay value. So I'm constantly seeing "missing ; before statement" spurious messages. Is it possible to completely disable linting until I explicitly require it? Or at least to suppress the messages triggered by autocomplete?

@dgutov
Copy link
Collaborator

dgutov commented Aug 4, 2014

@memeplex By "auto-complete", do you mean ac-js2?

Probably not: ac-js2-candidates explicitly calls js2-reparse, since it wants to use the generated AST.

You could try Tern instead.

@dgutov
Copy link
Collaborator

dgutov commented Aug 4, 2014

Ah, you can also try (setq js2-mode-show-parse-errors nil), but then the syntax errors won't be highlighted, at all ever.

@memeplex
Copy link

memeplex commented Aug 4, 2014

Ok, I will just comment the reparse line there. Maybe I will ask the author to add an option to honour the idle timer delay, too.

BTW, while debugging this issue I notice I'm getting a lot of no available AST messages (or something like that, I'm not at the pc right now). Is that expected?

@dgutov
Copy link
Collaborator

dgutov commented Aug 4, 2014

I'm getting a lot of no available AST messages... Is that expected?

After commenting out the "reparse line"? Of course. That line is there for a reason.

@memeplex
Copy link

memeplex commented Aug 4, 2014

No, I've not commented that line yet. The error is just popping up all the time while debugging.

@dgutov
Copy link
Collaborator

dgutov commented Aug 4, 2014

Then probably not. But I guess that might depend on how you're doing the "debugging".

@memeplex
Copy link

memeplex commented Aug 4, 2014

I've just instrumented the ac-candidates function and entered the edebug stepper, then after each step I get the aforementioned message, as if it were triggered by a timer. Functionality related to the AST works fine though. The message is not shown outside the debugging session.

@mkleehammer
Copy link

It doesn't seem like the delay is working at all for me. When I type "function testing" it starts turning red as soon as I type the first letter of the function name with a 5.0 second timer.

I'm on 24.4.1 on OS X with js2-mode version 20141105.154.

Any suggestions? Is there anything I can provide to troubleshoot?

@mkleehammer
Copy link

Also +1 on the idea of hiding parse errors until the file is saved.

@dgutov
Copy link
Collaborator

dgutov commented Nov 8, 2014

Any suggestions?

js2-idle-timer-delay is buffer-local, you should use setq-default to change it.

@mkleehammer
Copy link

I used the customize interface (M-x customize-group). Does that work?

@dgutov
Copy link
Collaborator

dgutov commented Nov 8, 2014

It should, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants