Hello Ernie, Svata and Everyone First of all thank you for your detailed answers. I think, it should give me the direction, where to move further.
Still I wanted to answer to some of your comments (and probably ask some more questions). I will answer to both emails at once. I hope, it should not cause too much confusion. Related to OffsetBag: [Ernie]: > I haven't looked at highlighting for a long time, I seem to recall that > OffsetsBag had quite a bit of functionality (and I could be > miss-remembering); threading/listeners/... Implementing your own, rather > than using OffsetsBag, sounds risky and bug prone and extra work. Why > did you make that choice? [Svata]: > If you do not modify directly the WhitespaceHighlighting, which computes > the chunks lazily, consider using the OffsetsBag The tutorial uses OffsetBag, so I started with the implementation, which uses it, but it also updated highlights for the whole document for each change. This worked fine, but I guess, the performance is not optimal. I decided, that I want to make sure, that at least on edits only the modified part is updated (probably there are still better approaches, but this will be discussed later). When I tried to implement the new version, where only part of the OffsetBag was either removed or added, then I've got very strange effect, that highlight flickers after some text is removed (or several lines are pasted at once). The flickering happened, when one just moves the cursor around (so even the document was not modified). I look a bit into the problem and noticed, that when I remove highlights from the OffsetBag, they are not completely removed, but just become 0-length. After some fiddling with it I managed to make the highlights really be deleted, but even then flickering did not go away. At that point I decided, that I will try to implement HighlightsContainer myself. The logic is not too complicated (even though, as Svata pointed out, multithreading issues should be addressed). Regarding OffsetBag, I have no idea, why it happens... Maybe I used it wrong, but I don't think so. Unfortunately I did not keep the implementation. Probably I will try to re-create it in separate branch, if someone finds it useful. [Ernie]: > I'm lazy, I'd probably start with the whitespace implementation. Maybe > > 1. copy whitespace and just do EOL with it > 2. starting with 1. evaluate incorporating it into whitespace. > > > Possibly 1. could be made into a plugin to get some feedback, then roll > that into NB proper or merge it into whitespace. Yes, thank you. My thoughts are not exactly like this, but go in the same direction. :) All further citations are from Svata. > Looking at the impl, there will be slight performance issues, especially > with large documents w/ lots of small lines (xml, html, ...). Do you mean only on startup? Or generally. On startup - I agree, because the whole document is processed at once. I admit, that I did not profile, but I guess, that the penalty should be not to high even for several thousands of lines... on the other hand, it is very generic highlight, so it probably should survive also opening Log-files several Gb long... In this case appproach with processing only the part, which is currently shown is much better. The problem is, of course, that inside the HighlightsContainer we don't explicitly control, for which region HighlightSequence will be requested. Of course, we may hope, that the requested range is normally for only part of the text, which is shown at the moment. Then it is quite small and we can calculate it on the fly. This approach sounds for me more univeral, so I will try it (and probably also do some profiling with long files). > You probably do not need to keep *complete* index of all EOLs in the > text here; the HightlightSequence is only called for a limited amount of > text that is visible during drawing. Unfortunately in the HighlightersContainer itself we don't control it, so we cannot guarantee, that it is the case. But from common sense point of view, most likely it will always be the case, so probably we still can rely on this assumption. > If you do not modify directly the WhitespaceHighlighting, which computes > the chunks lazily, consider using the OffsetsBag: see above > callbacks like > insertUpdate/removeUpdate may happen in any thread, while hightlight > sequences are likely to execute in EDT thread. I am not 100% sure, but I beleive, highlights SPI documentation makes some statements about it... > Your code is not > thread-safe maintaining highlightsStartOffsets; at least, the > EolHighlightSequence should get an immutable copy of the list. Thank you for pointing this out. I will check it and adjust according to you suggestion. > As for concurrency: highlighters AFAIK collect events from document / > other modifications and fire highlight change *at any time* even while > the highlight sequence might be interated through by editor's painting > code (from EDT) . I think the code that throws ConcurrentModException in > your sequence will break things somewhat. This behaviour (throwing this exception) is described as required by SPI documentation. See e.g. here: http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-editor-lib2/org/netbeans/spi/editor/highlighting/HighlightsSequence.html Implementation: Any HighlightsSequence obtained from any of the classes in the Highlighting API will behave as so called fast-fail iterator. It means that it will throw ConcurrentModificationException from its methods if the underlying data (highlights) have changed since when the instance of the HighlightsSequence was obtained. > > - Handle them together with whitespaces in WhitespaceHighlighting.java. > > I'd suggest to incorporate the changes into WhitespaceHighlighting; the > newline is always part of 'traling whitespace', although not painted by > default. > >> - In the provided example the font settings are retrieved using > >> org.netbeans.api.settings.FontColorSettings.getFontColors(). But I cannot > >> easily find, where can I add my own setting EOL, so that it is > >> configurable from the Options dialog? > > Do you want to color EOLs separately from the regular 'traling > whitespace', or do you want rather to include EOL character in the > traling whitespace (when it is printed) ? > This is good point and also a question from my side. I also was thinking about just implementing highlights for EOL, which would use same attributes as "trailing whitespaces". On the other hand, the issue, which I referred to originally (NETBEANS-1678) suggest creation of new category. So, could you tell, what should be the solution from the user point of view? Should just EOL respect the defined highlight style for trailing-whitespaces? Or should it get independent category in highlights section in Options window? > >> - When I change the background of the EOL symbol, this background spans > >> till the very right edge of the text editor. Is it expected? In my opinion > >> it does not look nice. Can it be fixed somehow? I tried to pass the option > >> HighlightsContainer.ATTR_EXTENDS_EOL = Boolean.FALSE, but it did not help. > > It's strange, but I could not find even a bit of code that _reads_ that > attribute; maybe I missed something. > > Note that EOL painting is quite peculiar: the character may or may not > be displayed - which is handled by NewlineView that optionally paints > the character. IMHO ATTR_EXTENDS_EOL should be handled there, since that > code paints background to editorView.getMaxX() if the hightlight item > covers the newline character. .. well, so it looks like this point needs further investigation. By the way, this may be a problem, if we want to incorporate EOLs into trailing whitespace. I guess, that some people want to use background color to make trailing whitespaces visible (and even annoying), but if with EOL the highlighting will be spanned till the end of the line, the whole idea will be broken... > >> - Are any automated tests needed for this particular case? What is > >> considered to be useful to test? > Yes !! Absolutely. In this case, you need to check various the highlight > layer + editor view paint combinations. To ensure the rest of line is > NOT painted; you're likely the first after LONG time to use > ATTR_EXTENDS_EOL = false. OK. I will also look at the existing tests for whitespaces (now I can do it, as I found, where the functionality is implemented). As a next step, I will try to look more carefully into implementation for trailing whitespaces, and try to understand, if adding EOLs there is a complicated task. Besides, I will have a look at automated tests. The questions about highlight spanning to the right border of the editor, and about my problems with OffsetBags are still open. Maybe I will try to look into it either, but I am not sure, that I will find time soon. Thanks again to everyone, who spends his/her time to read these emails and share the thoughs. Best Regards, Dmitrii. On 2020/06/24 14:49:49, Svata Dedic <[email protected]> wrote: > Hi, > > I do not follow exactly from the start, but ... > > Dne 23. 06. 20 v 23:48 Dmitry Semikin napsal(a): > > Nevertheless, meanwhile I updated the example implementation, so that it > > implements now HighlightsContainer directly instead of using OffsetsBag. If > > someone is interested it is still available on GitHub here: > > https://github.com/DmitrySemikin/nb-eol-highlighter > > > > Looking at the impl, there will be slight performance issues, especially > with large documents w/ lots of small lines (xml, html, ...). > > You probably do not need to keep *complete* index of all EOLs in the > text here; the HightlightSequence is only called for a limited amount of > text that is visible during drawing. > > If you do not modify directly the WhitespaceHighlighting, which computes > the chunks lazily, consider using the OffsetsBag: callbacks like > insertUpdate/removeUpdate may happen in any thread, while hightlight > sequences are likely to execute in EDT thread. Your code is not > thread-safe maintaining highlightsStartOffsets; at least, the > EolHighlightSequence should get an immutable copy of the list. > > As for concurrency: highlighters AFAIK collect events from document / > other modifications and fire highlight change *at any time* even while > the highlight sequence might be interated through by editor's painting > code (from EDT) . I think the code that throws ConcurrentModException in > your sequence will break things somewhat. > > > - Handle them together with whitespaces in WhitespaceHighlighting.java. > I'd suggest to incorporate the changes into WhitespaceHighlighting; the > newline is always part of 'traling whitespace', although not painted by > default. > > >> - In the provided example the font settings are retrieved using > >> “org.netbeans.api.settings.FontColorSettings.getFontColors()”. But I > >> cannot easily find, where can I add my own setting “EOL”, so that it is > >> configurable from the Options dialog? > > Do you want to color EOLs separately from the regular 'traling > whitespace', or do you want rather to include EOL character in the > traling whitespace (when it is printed) ? > > >> - When I change the background of the EOL symbol, this background spans > >> till the very right edge of the text editor. Is it expected? In my opinion > >> it does not look nice. Can it be fixed somehow? I tried to pass the option > >> HighlightsContainer.ATTR_EXTENDS_EOL = Boolean.FALSE, but it did not help. > > It's strange, but I could not find even a bit of code that _reads_ that > attribute; maybe I missed something. > > Note that EOL painting is quite peculiar: the character may or may not > be displayed - which is handled by NewlineView that optionally paints > the character. IMHO ATTR_EXTENDS_EOL should be handled there, since that > code paints background to editorView.getMaxX() if the hightlight item > covers the newline character. > > >> - If this functionality is going to ever be integrated into Netbeans, how > >> should it be implemented: as separate module, or as part of existing > >> module? > IMHO the current state seems more like omission (combination: paint > trailing whitespace + show nonprintables). So I think yes. > > >> - If existing module, which one? > Whitespace stuff is handled in editor.lib2 > > >> - Are any automated tests needed for this particular case? What is > >> considered to be useful to test? > Yes !! Absolutely. In this case, you need to check various the highlight > layer + editor view paint combinations. To ensure the rest of line is > NOT painted; you're likely the first after LONG time to use > ATTR_EXTENDS_EOL = false. > > -Svata > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > For further information about the NetBeans mailing lists, visit: > https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists > > > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] For further information about the NetBeans mailing lists, visit: https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
