At the moment we only re-colorize typenames at the visible part of the screen (after full colorization when the file gets loaded). This works well for normal editing but is insufficient for the replace operation as the replaced text can be outside the visible area.
To fix this, perform full recolorization when performing replace. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1022
-- Commit Summary --
* Fix typename highlighting after using replace
-- File Changes --
M src/document.c (3)
-- Patch Links --
https://github.com/geany/geany/pull/1022.patch https://github.com/geany/geany/pull/1022.diff
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022
@techee I apologise if I wasn't clear, the highlighting has to be done *after the parser has run* after the replace has been done. Otherwise the new name isn't in the wordlist and it doesn't highlight.
I can't test until tomorrow, but I suspect that it won't work for the reason above.
I could get it to repeatedly fail by doing a replace-all of a typename that is used enough that there are occurrences off screen after Scintilla scrolls to the last replacement.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-216830438
@elextr Yeah, I guess you are right - hopefully the new patch I've just pushed should fix it.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-216883710
@techee are you sure you pushed the improvement? It looks the same to me, and I just had a chance to try it, but it still doesn't work.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-217708951
@elextr Yes, it contained the "fix" but I checked right now with some traces enabled and it didn't work the way I expected (interestingly I'm not able to reproduce the issue myself so testing just indirectly by having a look when the rehighlighting function gets called).
Hopefully the issue is really fixed now in the very last commit - could you test please?
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-218470848
(interestingly I'm not able to reproduce the issue myself so testing just indirectly by having a look when the rehighlighting function gets called).
Interesting, I'm running the GTK3 version, possibly that affects the order of callbacks.
09:14:04: Geany INFO : Geany 1.28 (git >= 24f9198), en_AU.UTF-8 09:14:04: Geany INFO : GTK 3.10.8, GLib 2.40.2
Will test ASAP.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-218627310
I have also just found another case where the highlighting goes wrong (C++).
Near top of file I had `struct Foo{};` a definition, so `Foo` was highlighted everywhere.
Changed it to a forward `struct Foo;` so `Foo` was unhighlighted since forwards don't cause highlighting (why is another question).
Scrolled down a bit and added `struct Foo { some stuff };` and all visible instances of `Foo` were highlighted, but scroll back and previous instances of `Foo` were not highlighted.
The same happens when previous instances were highlighted and a change is made that should unhighlight all instances, those before the first visible line remain highlighted.
Essentially any change to the file that causes a change in highlighting seems to not be applied before the current first visible line.
IIUC Scintilla considers the styling to be invalid from some point forward, so if we tell Scintilla that the first visible line forward is invalid, it has no reason to re-lex when scrolling back, since it was before the invalid point.
So if we make any change that may change the lexing in the file we need to re-lex the whole file, not just the visible bit forward. Such a change is uploading new wordlists and that should trigger a whole file re-lex.
Unfortunately I don't think we currently have a way of checking if the wordlists have changed, so that re-lex has to occur each parse.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-218684519
@techee discussion with @b4n on IRC it seems the problem indeed only occurs on gtk3 version.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-218923991
[…] it seems the problem indeed only occurs on gtk3 version.
I can confirm, although I find it weird it does. I guess the GTK2 version of Scintilla forces more re-highlighting for some reason?
Anyway, I see 4 options:
* either not bother optimizing (e.g. re-colorize everything), effectively reverting #575. I guess this isn't wanted :) * don't bother optimizing too much, and force re-colorizing up to the end of the visible area (`sci_colourise(sci, 0, end_of_the_visible_area)`). This is less expensive near the start of a document, but the same near the end. * detect when typenames changed, and only re-highlight (all) in this case. This would be #518 I guess? * update the visible area *until we updated to the start*. This is a fixed version of the current optimization, by continuously updating the visible area until we updated the start of the document.
I tried implementing the last option in 71b80edf04a5b5d565da45abe0a71a32debaf316 (in my [typename-highlight-fix branch](https://github.com/b4n/geany/commits/typename-highlight-fix)). It's not that hard to do actually, as Scintilla generously does it for us going forward (the part after the visible area) -- which is why we don't suffer too much from the bug at hand currently --, so we only need to track what was the lowest position we updated since the request. This should be fairly cheap as it's incremental, and might even get "coalesced" with other scheduled updates.
What do you think? Also, @techee is this (or another) option OK regarding performances? I didn't perform the stress-tests you did leading to the current optimization, yet it might be interesting to.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-218930776
I can confirm, although I find it weird it does. I guess the GTK2 version of Scintilla forces more re-highlighting for some reason?
Strange - I still don't seem to be able to reproduce it even on GTK3. But if this happens, couldn't it actually affect also fold points even when typenames actually don't change? Imagine you e.g. replace { with an empty space in the whole document and if the whole document doesn't contain any new typenames, there would be no change in typenames and the document wouldn't get re-lexed to the beginning and the fold points would be wrong. Have you tried something like that? If this is the case, then I think it's Scintilla that should be fixed.
Anyway, I see 4 options:
either not bother optimizing (e.g. re-colorize everything), effectively reverting #575. I guess this isn't wanted :)
We actually could do this if Neil accepted this patch:
https://sourceforge.net/p/scintilla/bugs/1734/
That would reduce the W*T (words in document times typename count) complexity to W*log(T) which would be fine. This is the simplest solution and we wouldn't need to do any crazy stuff in Geany. (In the pull request I was maybe pushing too hard to get the patch in and also made a mistake in absolute numbers when exactly the binary search gets better than linear search and it got eventually rejected and 40% performance loss for cases where it doesn't matter won over the 3000% performance loss in the case I tested and where the performance loss was really visible. But even if the linear part was used for some crazy number like 10000 typenames and the binary search for anything above to make really sure the result won't get slower in any case, this would solve the problem for us.)
The second option is to use SCI_SETIDENTIFIERS as Neil suggested - this works only for the cxx lexer but since almost all (if not all) languages for which we do typename highlighting use the cxx lexer, we'd probably be fine. But I haven't studied how exactly this works.
don't bother optimizing too much, and force re-colorizing up to the end of the visible area (sci_colourise(sci, 0, end_of_the_visible_area)). This is less expensive near the start of a document, but the same near the end.
That doesn't solve the scaleability issue, the complexity in average will be just one half of W*T.
detect when typenames changed, and only re-highlight (all) in this case. This would be #518 I guess?
#518 is probably too complex - we could maybe just simply compare the last typename string with the current typename string without detecting typename candidates in the document.
update the visible area until we updated to the start. This is a fixed version of the current optimization, by continuously updating the visible area until we updated the start of the document.
That's possible, the question is whether it's not too complex to workaround some Scintilla issues. I think the simpler variant of #518 might be less LOCs but the best would be to either fix Scintilla or use the SCI_SETIDENTIFIERS option if it works.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-219034491
@b4n 71b80ed seems to work for me. Can we do it as an interim until Scintilla is fixed (since Neil is not gonna do anything until he returns from your side of the world).
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-219214084
@b4n @techee an external discussion asked if this could be the source of occasional typename highlighting errors in split window view. Seems possible relationship to me, and I am not sure the above will fix it.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-219214720
@elextr my solution won't fix splitwindow as it manually queries further highlighting on scrolling on *our* view, and we have no way of detecting some other view is seeing some other location. We can fix SplitWindow one of two ways: either apply the same fix to it, or refresh the whole document's styling. Or get Scintilla to do that for us (like it seems to do somehow for GTK2) so any Scintilla view is fixed, it's not up to us.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-219286284
@b4n, then I return to my initial comment that the whole buffer should be re-styled after new wordlists. Sorry @techee if it makes your big files slow, but right is far more important than fast.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-219327052
I'll prepare some simple patch that compares the last used typename list with the current one and performs full rehighlight if the two differ. The performanace should be fine even for huge files and lots of tags as long as the user doesn't edit something that is a typename (and that changes the typename list) - which isn't that frequent.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-219826171
Closed in d95111d4cd02e51eac90f814c4cd24ed8014a4c6
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#issuecomment-222719376
Closed #1022.
--- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1022#event-677150062
github-comments@lists.geany.org