Bug 531769 - [StyledText] StyleRange to allow horizontal spacing before characters (instead of GlyphMetrics replacement)
Summary: [StyledText] StyleRange to allow horizontal spacing before characters (instea...
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.8   Edit
Hardware: All All
: P3 enhancement with 2 votes (vote)
Target Milestone: ---   Edit
Assignee: Platform-SWT-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 531768 (view as bug list)
Depends on:
Blocks: 531957 534314 538262
  Show dependency tree
 
Reported: 2018-02-27 19:11 EST by Angelo ZERR CLA
Modified: 2021-01-21 09:40 EST (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Angelo ZERR CLA 2018-02-27 19:11:36 EST
The inlined annotation support draw line content annotations by using GlyphMetrics#width which are set with StyledText#setStyleRange for a given offset when annotation is drawn (see InlinedAnnotationDrawingStrategy). The offset used contains a character (character after the GlyphMetrics) which is replaced by GlyphMetrics, in other words the character disapear. To fix this problem the draw of inlined annotation:

 * draw the content of the inlined annotation
 * redraw the character which is replaced by GlyphMetrics

This redraw is not perfect because it doesn't take care of some background color like selection, line cusrsor, etc

You can see the trouble with a simple snippet:

 * the StyledText contains "abc"
 * the insert of GlyphMetrics after the character "a" (offset of "b" character replace "b" and "b" is not visible.

Here the snippet:

-----------------------------------------------------------------
StyledText text = new StyledText(shell, SWT.BORDER | SWT.V_SCROLL);
text.setText("abc");

// insert a blank space after "a" by replacing "b" character
GlyphMetrics metrics = new GlyphMetrics(0, 0, 100);

StyleRange style = new StyleRange();
style.start = 1;
style.length = 1;
style.metrics = metrics;
text.setStyleRange(style);
-----------------------------------------------------------------

The StyledText will contain ("b" is hidden by GlyphMetrics):
-----------------------------------------------------------------
a          c
-----------------------------------------------------------------

My idea to fix this problem (and manage line content annotation with clean mean without redrawing the character "b") is that GlyphMetrics should provide a new field "insert"

-----------------------------------------------------------------
/**
 * true if metrics should be inserted in the given offset and false othewise.
 */
public boolean insert;
-----------------------------------------------------------------

Here  the same snippet with "metrics.insert = true;"

-----------------------------------------------------------------
StyledText text = new StyledText(shell, SWT.BORDER | SWT.V_SCROLL);
text.setText("abc");

// insert a blank space after "a" and before "b" ("b" is visible)
GlyphMetrics metrics = new GlyphMetrics(0, 0, 100);
metrics.insert = true;

StyleRange style = new StyleRange();
style.start = 1;
style.length = 1;
style.metrics = metrics;
text.setStyleRange(style);
-----------------------------------------------------------------

The StyledText will contain ("b" is visible):
-----------------------------------------------------------------
a          bc
-----------------------------------------------------------------

To provide this feature, the TextLayout of each OS must be changed. As I have only Windows OS, I have supported that for windows. 

I will create a gerrit patch with this snippet and support of GlyphMetrics#insert only for windows.

If you like this idea, I hope that they will have people which will update too TextLayout for other OS.

@Mickael, do you like this idea? If yes, do you think there is any chance that  we will find people who will implement GlyphMetrics#insert for other OS?

IMHO I think we should have this feature to provide a better draw of inlined line content annotation (without redrawing the character will support selection, line cursor, etc)
Comment 1 Eclipse Genie CLA 2018-02-27 19:18:54 EST
New Gerrit change created: https://git.eclipse.org/r/118321
Comment 2 Niraj Modi CLA 2018-02-28 03:17:06 EST
*** Bug 531768 has been marked as a duplicate of this bug. ***
Comment 3 Mickael Istria CLA 2018-02-28 03:42:16 EST
Do we need a new field? Isn't the current GlyphMetrics behavior a bug or is there any use-case for which it's desired? How does vertical glyphMetrics behave, is it just an "insert"?
Comment 4 Angelo ZERR CLA 2018-02-28 03:48:02 EST
> Do we need a new field?

Yes we need it to disting insert or replace.

> Isn't the current GlyphMetrics behavior a bug or is there any use-case for which it's desired? 

Here 2 samples:

 * http://www.java2s.com/Tutorial/Java/0280__SWT/StyledTextembedimages.htm
 * http://www.java2s.com/Tutorial/Java/0280__SWT/StyledTextembedcontrols.htm

Those samples replace \uFFFC character with images, control which take place with GlyphMetrics.

> How does vertical glyphMetrics behave, is it just an "insert"?

We have 2 usecases:

 * replace character like \uFFFC with images, control, etc by using GlyphMetrics
 * insert images, control, etc before a character by using GlyphMetrics
Comment 5 Mickael Istria CLA 2018-02-28 04:40:27 EST
What does happen when style.lenght == 0? I think this should be interpreted as an insert (ie replace 0 character with the GlyphMetric.width).
Comment 6 Angelo ZERR CLA 2018-02-28 05:21:56 EST
> What does happen when style.lenght == 0?

It's not supported, When you have a style with length = 0, it is ignored. (See StyledTextRenderer#updateRanges (first test).

Even if you comment this test, it doesn't work.

To be honnest with you, my fear is not about this API (I understand that you don't want to add insert flag and use length = 0) but more for update of TextLayout of the other 2 OS.

Do you know people who could do that? Please note my gerrit patch is not perfect with windows, there is a little bug when cursor is moved accross the GlyphMetrics (it doesn't position the cursor in well place).
Comment 7 Mickael Istria CLA 2018-02-28 07:16:07 EST
(In reply to Angelo ZERR from comment #6)
> To be honnest with you, my fear is not about this API (I understand that you
> don't want to add insert flag and use length = 0)

We need to focus on keeping API meaningful and easy. If we have to update stuff, we'll do it.

> more for update of TextLayout of the other 2 OS.

Does the TextLayout need to be updated? Looking at the Linux code, it seems to me that the test layout is applied even for width = 0.
Comment 8 Angelo ZERR CLA 2018-02-28 07:49:48 EST
> We need to focus on keeping API meaningful and easy. If we have to update stuff, we'll do it.

Yes sure! 

> Does the TextLayout need to be updated? Looking at the Linux code, it seems to me that the test layout is applied even for width = 0.

In win32, I'm afraid that it doesn't work (but I will study more why style.length doesn't work)

Have you tried my snippet https://git.eclipse.org/r/#/c/118321/1/examples/org.eclipse.swt.snippets/src/org/eclipse/swt/snippets/Snippet400.java (remove code metrics.insert and set style.length to 0)

If it works with your OS, you should see:

-----------------------------------------------------------------
a          bc
-----------------------------------------------------------------
Comment 9 Mickael Istria CLA 2018-02-28 10:53:05 EST
I don't think introducing an "insert" flag is a nice solution. It will make the code more complex to maintain as GlyphMetrics would suddenly have different meaning.
Either we manage to deal with case of length==0 in TextLayout or we need to introduce a whole new concept in the StyledText such as "setSpacing(offset, width)" to properly inform that we want an extra space at given offset.
Comment 10 Angelo ZERR CLA 2018-02-28 11:02:19 EST
>I don't think introducing an "insert" flag is a nice solution. It will make the code more complex to maintain as GlyphMetrics would suddenly have different meaning.

+1

> Either we manage to deal with case of length==0 in TextLayout 

+1 

> or we need to introduce a whole new concept in the StyledText such as "setSpacing(offset, width)" to properly inform that we want an extra space at given offset.

-1
Comment 11 Mickael Istria CLA 2018-02-28 11:06:10 EST
The lines which cause 0-length styles to be ignored is StyledTextRenderer:1121&1128 which override the previously defined style on the same index.
Comment 12 Mickael Istria CLA 2018-02-28 12:06:43 EST
I've got an idea of a workaround that would remain on the StyledTextRenderer layer: if we have a style with lenght==0 on the line, when creating the layout, we instead some blank characters in the line and modify the following layouts to that the glyphmetrics is applied on the invisible character. It has a big impact though on getOffsetAtPoint and family.
Overall, I'm also worried that 0-length styles may get wiped out in other layers such as "mergeStyles" here and there and that even improving stuff on SWT layer wouldn't have any effect because the received style would be erroneous.
I'm afraid there is no good solution at all with the current state.

Have you investigated trying to draw with some transparent background?
Comment 13 Angelo ZERR CLA 2018-02-28 12:10:24 EST
> I've got an idea of a workaround that would remain on the StyledTextRenderer layer: if we have a style with lenght==0 on the line, when creating the layout, we instead some blank characters in the line and modify the following layouts to that the glyphmetrics is applied on the invisible character. It has a big impact though on getOffsetAtPoint and family.

I think it's a dangerous idea to do that.

> Overall, I'm also worried that 0-length styles may get wiped out in other layers such as "mergeStyles" here and there and that even improving stuff on SWT layer wouldn't have any effect because the received style would be erroneous.
I'm afraid there is no good solution at all with the current state.

Yes exactly.

> Have you investigated trying to draw with some transparent background?

Yes it's not possible. Use of GlyphMetrics draw blank space, so it's not possible. 

Why you don't like my gerrit patch? It's because it changes the API?
Comment 14 Mickael Istria CLA 2018-02-28 12:14:16 EST
(In reply to Angelo ZERR from comment #13)
> Why you don't like my gerrit patch? It's because it changes the API?

Yes. And also because it requires to adopt this new setting in all OSs, which is much more difficult.
Comment 15 Angelo ZERR CLA 2018-02-28 12:17:42 EST
> Yes. And also because it requires to adopt this new setting in all OSs, which is much more difficult.

I'm afraid that there is not other mean to support clean draw of inlined annotation -(
Comment 16 Mickael Istria CLA 2021-01-21 09:40:26 EST
I see 2 Gerrit patches on this topic https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/118321 and https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/149673 .
@Angelo @Paul Should we keep both, none, or just one?