Prevent Emacs fontification failure with lots of classes - Fixes #248 #2508
Conversation
…ollider#248 What I've done to fix this: - add a sclang-class-list variable in sclang-language.el, which is a list of all the classes known to sclang. this is populated when sclang starts. - update sclang-font-lock-class-keyword-matcher in sclang-mode.el. since the sclang-class-name-regexp now will match all words that start with a capital letter (see next bullet point for that change), this function had to be updated to check to make sure that the word starting with a capital letter is in the list of classes. if it is, then we know it's a class and it gets highlighted. if it's not, then it's just something the user typed with a capital letter, so we don't highlight it. - update sclang-update-font-lock in sclang-mode.el. instead of generating a huge regexp from a list of all the classes in SuperCollider, just run the normal fontification from that function. this avoids making the regexp too big and thus prevents fontification from failing.
|
I haven't thoroughly read the elisp but the overview of this PR looks like a definite improvement! |
|
i'd recommend to get rid of the regex: when i was still using scel, i often ran into issues that the compiled regex exceeded the maximum size and highlighting classes broke completely. so highlighting symbols starting with capital letters might be the best approach in general (the highlighter in the IDE works similar) |
|
@defaultxr Unless you disagree, I think it's best if we incorporate Tim's advice as a requirement for merge. |
|
I'm not sure I understand - the only regex for classes I have is the one that matches all capitalized words. That regex will never grow long enough to break because it isn't made of the list of class names. The fontification function first tests against the capitalized word regex (which should be relatively fast), and then if it matches and slang has started (and thus we have a list of classes) it checks if the capitalized word is in the list of classes. No regex is ever generated from the class list, so it's not possible for it to break if too many are installed. I think if we have the list of classes and can highlight ones that actually exist and don't highlight the ones that don't, it provides better feedback to the user since they can see if they've made a spelling mistake or don't have a quark installed, etc. Let me know if I'm misunderstanding anything. |
|
Ah ok, I misunderstood the status of this PR. I shouldn't have tried to contribute to discussion without knowing anything about scel or emacs! |
|
I'm happy to merge this if noone has thoughts! |
|
Thanks for the contribution @defaultxr , sorry it took so long to merge! |
Hi,
These changes should prevent Emacs from failing to fontify class names when lots of classes exist in sclang (i.e. when many quarks are installed). I've elaborated in the commit messages, but the general idea of these changes is this:
sclang-class-listso we can refer back to it later during fontification (lines 223-226 in sclang-language.el).sclang-class-listthen we know that they're an actual class and we color them as such. if they're not insclang-class-listthen it's not a class, so we don't color it.I've been using these changes locally on my own machine for about a year now and have not had any problems with them so I figured it was time to submit a pull request so everyone can benefit from them. I've tested these changes myself and have not had any issues. I've made other changes to the elisp but I made sure to only stage the relevant ones to #248. I also included one minor change to line 268 in sclang-mode.el which is to just add a space between the function name and the argument, because that was bothering me.