Community
Participate
Working Groups
I create this issue for line header annotation feature which requires to takes spaces before a line to draw the line header annotation. Todat it uses GlyphMetrics#ascent but there are a lot of problem with this solution: * using Glyphmetrics remove the character where it is setted, that's line header annotation redraw this character which provides some problem (ex: selection of this character has not black foreground). * when line which have line header annototation below is selected, blue background is done for line text and line header annotation (although it should keep blank). * matching bracket square has a too big height (the line text height + line header annotation height) * same problem when mark occurences is done for method (in JDT Java Editor) * same problem with square height when refactor is done for method (in JDT Java Editor) -> https://github.com/angelozerr/jdt-codemining/issues/6 To fix those all problem, StyleRange should manage a new marginTop property: StyleRange.marginTop should initialize TextLayout#setMarginTop(int marginTop) which manage margin top (spaces before lines). As it requires changed of TextLayout, it means that each OS TexLayout must support the new setTopMargin method. A gerrit patch will follow, which works well with OS Windows. I will update TextLayout for other OS (gtk and mac os), but I cannot test it, please test it and help me to fix problems if there are problems!
+1 for adding margins capability in StyleRange.
Here a basic sample with this marginTop: ------------------------------------------------------------------- import org.eclipse.swt.SWT;a import org.eclipse.swt.custom.StyleRange; import org.eclipse.swt.custom.StyledText; import org.eclipse.swt.layout.FillLayout; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Shell; public class StyleRangeTopMarginDemo { public static void main(String[] args) { Display display = new Display(); Shell shell = new Shell(display); shell.setLayout(new FillLayout()); StyledText text = new StyledText(shell, SWT.BORDER | SWT.V_SCROLL); text.setWordWrap(true); text.setText("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\nbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"); StyleRange style = new StyleRange(); style.start = 1; style.length = 1; style.marginTop = 20; text.setStyleRange(style); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } } ------------------------------------------------------------------- You should see the margin top 20 before the text and you should see word wrap is working too: ------------------------------------------------------------------- aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb -------------------------------------------------------------------
What happens if i have a line word1 word2 word3 word4 and wordwrap is enabled with few columns so it's wrapped like word1 word2 word3 word4 and I have the margin placed on word4? My expectation would be that in such case, the margin would be shows between the 1st and the 2md "visual" line, as the styledtext is applied on specific chars. Result would look like word1 word2 word3 word4 Is this what happens with your proposal?
> Is this what happens with your proposal? No, you will have every time: ----------------------------- word1 word2 word3 word4 ----------------------------- The margin top is only for the line text even with wordwrap. The feature that you wish to support is already supported with line spacing. With lien spacing, you will have a spacing before each line which starts on the left of the widget. The margin top works with line text. Takes samples: * case 1: "word1 word2" if you set a marginTop=10 in word1 and marginTop=20 in word2, you will have a marginTop=20 before the line ------------------------------ word1 word2 ------------------------------ if you wrap the StyledText, you will see ------------------------------ <- margin top = 20 word1 word2 ------------------------------ * case 2: "word1\nword2" if you set a marginTop=10 in word1 and marginTop=20 in word2, you will have that ------------------------------ <- margin top = 10 word1 <- margin top = 20 word2 ------------------------------ if you wrap the StyledText, you will see ------------------------------ <- margin top = 10 wor d1 <- margin top = 20 wor d2 ------------------------------ It's exaclty the feature that we need for line header annotation.
In this case, then I don't think the StyleRange is the best place to set the margin. StyleRange applies to specific characters, here you're targeting a whole line margin. It would be better to implement it on the StyledText directly. StyledText.setLineTopMarging(int line, int marginHeight)
> StyledText.setLineTopMarging(int line, int marginHeight) @Mickael if we try to do that, we will return with same problem than line spacing provider and I'm afraid it will be an hard task. Today we have an unique enter point the call of setStyleRange which manages line height. Perhaps the marginTop should be renamed to lineMarginTop?
(In reply to Angelo ZERR from comment #6) > @Mickael if we try to do that, we will return with same problem than line > spacing provider which is? IIRC, the benefit for GlyphMetrics was that it allowed better factorization between header and content annotations. But your proposal would also make them diverge anyway, so a divergence or another doesn't make a big difference. > and I'm afraid it will be an hard task. I don't get how it would be harder. In the end, it's mostly about passing this extra margin value to the renderer, isn't it? Whether the extra value comes from the StyleRange, the StyledText or the LineSpacingProvider changes nothing. > Today we have an > unique enter point the call of setStyleRange which manages line height. Ok, that answers my question above. > Perhaps the marginTop should be renamed to lineMarginTop? I don't think a line-wide property should be present on the StyleRange. StyleRange is able some characters in a Range, not about the line. Leaking line info in it would be bad API.
I think your proposal is acceptable if we have something like word1 with a marginTop=10 and word4 with marginTop=20 and see, for "word1 word2 word3 word4" === // <- 20 == nax(10,20) pixels high area word1 word2 word3 word4" === when no word wrap happens and === // <- 10 pixels high space area word1 word2 word3 // <- 20 pixels high area word4 === when word wrap happens. The Style is attached to the range
> when word wrap happens. The Style is attached to the range To be honnest with you, I think it's too hard to implement because it requires to update TextLayout word wrap and runs mechanism. Each line text of StyledText are managed by an instance of TextLayout. My gerrit patch manages just the margin top of TextLayout (margin top before the line which is wrapped or not). I must study if StyledText.setLineTopMarging(int line, int marginHeight) is easy to implement.
(In reply to Angelo ZERR from comment #9) > I must study if StyledText.setLineTopMarging(int line, int marginHeight) is > easy to implement. Why don't you use the StyledTextLineSpacingProvider instead?
> Why don't you use the StyledTextLineSpacingProvider instead? We have tried but we had a lot of problems with performance if I remember like don't support spaces for the first line, rfresh was not done correctly in some cases. That's why we have moved to StyleRange. More line spacing is bad idea because it set spacing to TextLayout, it means in word wrap it set spaces for wrapped lines althouth we don't ti have this spaces for line header annotation. We need just spaces before the line (which is wrapped or not).
(In reply to Angelo ZERR from comment #11) > More line spacing is bad idea because it set spacing to TextLayout, it means > in word wrap it set spaces for wrapped lines althouth we don't ti have this > spaces for line header annotation. We need just spaces before the line > (which is wrapped or not). Ok, so at the moment, apart from GlyphMetrics, there is nothing that would allow to build the case === // 10px word1 word2 word3 // 20px word4 === Right?
> Right? Exactly. That's why I have introduced marginTop which set a margin top only before the text line (in wordwrap or not).
(In reply to Angelo ZERR from comment #13) > > Right? > > Exactly. So I believe if we want the margin on the range, then it needs to affect the rendering at the same level as GlyphMetrics so it produces similar effects (can place margin on characters when in word wrap). If we want it on the line only -which is fine- then I understand that the StyledTextLineSpacingProvider isn't the right API. The margin/offset are a new concept in the StyledText, I feel now more sure they require their own StyledText.setLineTopMarging(int line, int marginHeight) API. As for implementation, I don't think it's more difficult than what you had in mind with placing this on the StyleRange: you forward this to the same methods, it's only the source that changes (instead of querying the styleRanges on the line, you directly query the StyledTextRenderer.LineInfo). Note that this is already somewhat implementd for LineInfo.indent (which is horizontal), so it could also be called LineInfo.verticalIndent for consistency.
> nstead of querying the styleRanges on the line, you directly query the StyledTextRenderer.LineInfo StyledTextRenderer.LineInfo is used only with addLineStyleListener(LineStyleListener listener). In our case we use setStyleRange (JDT use it, etc) which cannot be used with addLineStyleListener(see javadoc " Should not be called if a LineStyleListener has been set since the listener maintains the styles.") That's why I have add marginTop in the StyleRange.
(In reply to Angelo ZERR from comment #15) > StyledTextRenderer.LineInfo is used only with > addLineStyleListener(LineStyleListener listener). So it means that setLineIndent doesn't work for JDT editor for instance?
> So it means that setLineIndent doesn't work for JDT editor for instance? To be honnest with you, I have never tried, but I can confirm you JDT Editor doesn't use LineInfo. IMHO I think it's shame to creates a new array of LineInfo just to store margin top.
I think you should mimic how indent is implemented. > IMHO I think it's shame to creates a new array of LineInfo just to store margin top. Let's discuss how to improve implementation once we have a good API ;)
> I think you should mimic how indent is implemented. When I see setLineIndent, it calls resetCache, redrawLines, etc, so many hard work that you have already improved when setStyleRanges is called. Hope it will not hard to remanage this reset cache.
(In reply to Angelo ZERR from comment #19) > > I think you should mimic how indent is implemented. > > When I see setLineIndent, it calls resetCache, redrawLines, etc, so many > hard work that you have already improved when setStyleRanges is called. Hope > it will not hard to remanage this reset cache. Invoking setLineIndent just reset the cache of target lines. I think it should be fairly comparable to setStyleRange in term of cascaded processing. We can for sure improve this so if code mining needs to call setVerticalLineIndent(line, indent) with the same line and same indent, we'll verify value differ before resetting the cache. But let's first wait for the issue to happen. I'm not pretty sure that StyledText.setVerticalLineIndent(int lineIndex, int indent) is the best API we can offer for this use-case.
See my gerrit patch https://git.eclipse.org/r/#/c/130141/ You can test it with the following snippet: --------------------------------- import org.eclipse.swt.SWT; import org.eclipse.swt.custom.StyledText; import org.eclipse.swt.layout.FillLayout; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Shell; public class StyleRangeTopMarginDemo { public static void main(String[] args) { Display display = new Display(); Shell shell = new Shell(display); shell.setLayout(new FillLayout()); StyledText text = new StyledText(shell, SWT.BORDER | SWT.V_SCROLL); text.setWordWrap(true); text.setText("word1 word2 word3 word4"); text.setLineMargingTop(0, 1, 20); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } } --------------------------------- and test codemining https://git.eclipse.org/r/#/c/130151/
Created attachment 276132 [details] Screenshot on Mac The Left StyledText uses verticalIndent=10 for lines 0 & 1. The spacing looks different. The Right StyledText uses verticalIndent=10 for line 0 & lineSpacing=10. The spacing looks the same.
Created attachment 276143 [details] codemining screenshot on mac - with patches I applied the patch from this bug and https://git.eclipse.org/r/#/c/130151/ and tried the JDT codemining feature. As seen in the screenshot, the spacing between the annotation and the line seems more than expected.
Indeed, there is a problem. The problem occurs when you open the editor or when you do only a Zoom? I think problem is inside TextLayout since it works with Windows OS. Please debug the problem, thanks! @Mickael have you the same problem on Linux OS?
(In reply to Lakshmi Shanmugam from comment #22) > Created attachment 276132 [details] > Screenshot on Mac > > The Left StyledText uses verticalIndent=10 for lines 0 & 1. The spacing > looks different. > The Right StyledText uses verticalIndent=10 for line 0 & lineSpacing=10. The > spacing looks the same. Snippet used: public class ModifiedSnippet374 { public static void main(String[] args) throws Exception { Display display = new Display(); Shell shell = new Shell(display); shell.setLayout(new FillLayout(SWT.HORIZONTAL)); shell.setText("Customize line vertical indent"); StyledText text = new StyledText(shell, SWT.BORDER | SWT.V_SCROLL); text.setWordWrap(true); text.setText("word1 word1 word1 word1 word1\nword2\nword3"); text.setLineVerticalIndent(1, 10); System.out.println(text.getLineVerticalIndent(0)); StyledText text2 = new StyledText(shell, SWT.BORDER | SWT.V_SCROLL); text2.setWordWrap(true); text2.setText("line1 line1 line1 line1 line1\nline2\nline3"); text2.setLineSpacing(10); text2.setLineVerticalIndent(0, 10); shell.pack(); shell.open(); while (!shell.isDisposed()) { if (!display.readAndDispatch()) display.sleep(); } display.dispose(); } }
I have not this problem on Windows OS -( @Mickael hav eyou this problem on Linux OS? @Lakshmi I think problem comes from Mac OS TextLayout.
Created attachment 276428 [details] line numbers - with SWT + JFace + JDT UI patches Hi Angelo, I also noticed a problem with the line numbers in the editor, the line number is shown next to the code mining, instead of the text line. This is with the SWT, JFace and JDT UI (Bug 529127) patches applied. Even without the SWT & JFace patch, only with JDT UI (Bug 529127) patch applied, I see that the line numbers are not aligned correctly to the code. It's in the middle of the code mining and code lines. Please see the below screenshot.
Created attachment 276429 [details] line numbers - with JDT UI patch only
Indeed @Lakshmi there is a problem with line number with this patch on Windows OS too: * with this patch, the line number is aligned to the top of the codemining (line header) although it should be aligned with the line text. * without this patch, it works well (I have not the same problem than you). I will try to fix problem in codemining gerrit patch https://git.eclipse.org/r/#/c/130151/
> I also noticed a problem with the line numbers in the editor @Lakshmi could you retry with my new gerrit patch https://git.eclipse.org/r/#/c/130151/
Created attachment 276431 [details] line numbers-with SWT+ JFace (patchset8) + JDT UI patch The line number has now moved down to the empty space between the mining and the code line. I think that once the extra empty space problem above the code line is fixed, the line number placing should be correct.
La> The line number has now moved down to the empty space between the mining and the code line. I think that once the extra empty space problem above the code line is fixed, the line number placing should be correct. Thanks for your feedback @Lakshmi. Yes I think problem is now in Mac OS TextLayout.
@Angelo, As you suspected, the problem is caused as result of using getScaledVerticalIndent. I think the method is not required for Mac. Replacing all occurrences of getScaledVerticalIndent with getVerticalIndent fixes the problem. Please update the patch with the required changes.
@Lakshmi thanks! I have updated the patch. Do you think it's good now to merge it? @Mickael is it working on Linux OS?
(In reply to Angelo ZERR from comment #34) @Angelo, I tested the latest patch on Mac and it fixes the line number problem. I noticed a strange problem, if I click anywhere on the line which has lineIndent set, the cursor is placed at the end on the line and not at the cursor location. I'm able to reproduce this problem in both Eclipse and SWT snippet (comment 25). Can you please take a look?
> I noticed a strange problem, if I click anywhere on the line which has lineIndent set, the cursor is placed at the end on the line and not at the cursor location. I have not this problem on Window OS, so I suppose it's a Mac OS TextLayout bug? >I'm able to reproduce this problem in both Eclipse and SWT snippet (comment 25). Can you please take a look? Very hard to fix the bug without debugging it -( @Mickael have you the same problem in Linux OS?
(In reply to Angelo ZERR from comment #36) > > I noticed a strange problem, if I click anywhere on the line which has lineIndent set, the cursor is placed at the end on the line and not at the cursor location. > > I have not this problem on Window OS, so I suppose it's a Mac OS TextLayout > bug? I did some debugging and found a problem in TextLayout.getOffset() on Mac. Please see gerrit for more details.
New Gerrit change created: https://git.eclipse.org/r/132593
Please review my N&N at https://git.eclipse.org/r/132593
This change caused Bug 541293
New Gerrit change created: https://git.eclipse.org/r/132839
Gerrit change https://git.eclipse.org/r/132593 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=65d648f80ccbfc630aa37e2216256c343ed66516
