[CLOSED] Linting can get re-run many extra times if you've viewed files outside your project #6067
Comments
|
Added to Kanban board too |
|
There's a subtler issue here too: the old code relied on a single no-op change at the beginning to set up its event listeners. If we fix But this issue is easy to miss, since even without the change CodeInspection will still run anyway. I think this is because there's a second instance of this bug in the JSLint main.js -- on every currentDocumentChange it gets a no-op prefs change, which in turn causes it to call requestRun(), an alias for run(). We should probably fix that too... |
|
Although I'm also confused as to why the prefs system is generating a no-op in the latter case (JSLint main.js) given that in this case you don't have to switch to a file outside the project. Is the problem that we simply can't tell for complex-valued prefs whether there's a diff or not? Seems like that could become a broader problem. I wonder if, as part of #6578, we should change prefs events so that they're are only fired when you switch projects or make an actual pref edit. (Only clients passing the special |
|
It was at your suggestion that the prefs system notifies for possible changes rather than notifying of specific changes. So, the current system notifies whenever it sees something that might have changed. You're correct, though, that the receiver of that event has to be careful to not do the wrong thing when it receives such an event. I'll investigate this problem and think about a minimal solution and how this intersects with #6578. |
|
Great catch! FBNC to |
|
I'm now seeing CodeInspection run twice per file switch all the time, regardless of any of the repro steps above. That's much improved from the behavior where the number of runs increases constantly, but still a regression from Sprint 35. Reopening |
|
It looks like the |
|
|
|
|
Wednesday Jan 29, 2014 at 01:25 GMT
Originally opened as adobe/brackets#6676
This is a regression that I think we should fix in Sprint 36:
Result:
CodeInspection runs many extra times: each time you switch between an external file and a project file, the number of times it's run increases by one. (But that number sticks: each file switch even within the project will then run CodeInspection N times).
Eventually switching files becomes noticeably slower (extra 1+ sec per switch).
Expected:
CodeInspection only runs once per currentDocumentChange.
This is happening because a no-op pref change event is sent, causing
toggleEnabled(true)to get called when_enabledis already true, which in turn redundantly re-registers the listener. We should either maketoggleEnabled()short-circuit out of no-op changes, or makeupdateListeners()robust to redundant calls (e.g. if it used a function outside its closure then jQuery wouldn't register it multiple times).@dangoor Do you have cycles to take this? I worry that there may be more cases similar to this -- functions that previously never encountered no-ops now encountering them due to prefs. I think we should do a quick scrub of all the pref-change listeners to check for this...The text was updated successfully, but these errors were encountered: