FWD » Core Development » User Interface

Invalid Caret Position¶

The display of an empty screen value should match that of a screen value containing spaces, especially with the caret positioned at the rightmost location. In FWD, when the screen value is empty, the caret shifts one pixel to the left compared to a screen value with a space in the first position and a visible caret.

This subtle difference is challenging to notice manually but triggers failures in automated tests using Sikuli.

How to Reproduce Example¶

1. Create a character FILL-IN widget.
2. Enter a space character.
3. Move the caret to position zero. Observe the exact caret position, which, in my case, is separated by a two-pixel white gap from the left black border of the FILL-IN.
4. Press DELETE-CHARACTER to remove the space. The caret shifts one pixel left, now separated by a one-pixel white gap from the left black border of the FILL-IN.

Disappearing Caret¶

In OE, the caret remains visible within an active FILL-IN at all times. In FWD, however, if the screen value is empty and the user presses a key that does not modify the screen value, the caret becomes invisible. If the user presses such a key again, the caret reappears.

For numeric FILL-IN widgets, setting the displayed value to empty is not possible, but this issue occurs with the unknown value.

Similar to the first issue, this problem also causes failures in automated tests with Sikuli.

How to Reproduce Example¶

1. Create a date FILL-IN with the default format.
2. Press ? to reset the screen value to an empty string.
3. Enter any letter character. Since letters are not valid in dates, this action should have no effect.
4. The caret disappears.
5. Enter any letter character again.
6. The caret reappears.

Vladimir Tsichevski wrote:

The caret X position is currently calculated in FillInImpl::drawCursor as:

[...]

For an unknown reason, the offset for empty text is explicitly set to a different value.

The issue is resolved by modifying the line to:

[...]

Eugenie, as this appears to be your code, could you provide your insights on this change?

Can you tell me what my change do you mean?

A general if you think your change fixes the issue and made enough testing to see you are right - you may apply your change.

Eugenie Lyzenko wrote:

Vladimir Tsichevski wrote:

The caret X position is currently calculated in FillInImpl::drawCursor as:

[...]

For an unknown reason, the offset for empty text is explicitly set to a different value.

The issue is resolved by modifying the line to:

[...]

Eugenie, as this appears to be your code, could you provide your insights on this change?

Can you tell me what my change do you mean?

I am referring to the modification I plan to implement to resolve this issue. Since this change will remove some logic you previously added to this line, I would like to understand your reasoning behind including that logic, to ensure I do not inadvertently disrupt anything by removing it.

Vladimir Tsichevski wrote:

Eugenie Lyzenko wrote:

Vladimir Tsichevski wrote:

The caret X position is currently calculated in FillInImpl::drawCursor as:

[...]

For an unknown reason, the offset for empty text is explicitly set to a different value.

The issue is resolved by modifying the line to:

[...]

Eugenie, as this appears to be your code, could you provide your insights on this change?

Can you tell me what my change do you mean?

I am referring to the modification I plan to implement to resolve this issue. Since this change will remove some logic you previously added to this line, I would like to understand your reasoning behind including that logic, to ensure I do not inadvertently disrupt anything by removing it.

I see your point.

I not understand why you think I was the author that made this change. My last change was 3 years ago. And I just do not remember everything I did 3 years ago. Can you be more concrete pointing to at least history record or trunk commit I did.

Vladimir Tsichevski wrote:

The caret X position is currently calculated in FillInImpl::drawCursor as:

[...]

For an unknown reason, the offset for empty text is explicitly set to a different value.

I think the idea is to have cursor position just after the last offset in a case there is a text in field to avoid overlapping the cursor and the text before it. For empty text we can ignore this condition and put the cursor in a first available pixel.

Eugenie Lyzenko wrote:

Vladimir Tsichevski wrote:

Eugenie Lyzenko wrote:

Vladimir Tsichevski wrote:

The caret X position is currently calculated in FillInImpl::drawCursor as:

[...]

For an unknown reason, the offset for empty text is explicitly set to a different value.

The issue is resolved by modifying the line to:

[...]

Eugenie, as this appears to be your code, could you provide your insights on this change?

Can you tell me what my change do you mean?

I am referring to the modification I plan to implement to resolve this issue. Since this change will remove some logic you previously added to this line, I would like to understand your reasoning behind including that logic, to ensure I do not inadvertently disrupt anything by removing it.

I see your point.

I not understand why you think I was the author that made this change. My last change was 3 years ago. And I just do not remember everything I did 3 years ago. Can you be more concrete pointing to at least history record or trunk commit I did.

bzr blame s you :-) I said "appears to be yours", so it was rather a question.

11218.1.19  sbi@gol |    @Override
                    |    public void drawCursor()
                    |    {
12808       evl@gol |       FontCache fc = getFontCache();
11285.1.2   evl@gol |       String txt = getText();
11296.1.16  evl@gol |       // take into account possible delta shift
                    |       int xPos = txt == null || txt.isEmpty() ? 1 : 2 + editScroller.getDeltaNative();
12824       evl@gol |       int cursorHeight = Math.max(fc.fontHeight, screen().coordinates().heightToNative(1) -
                    |                                                  (nativeInsets.top + nativeInsets.bottom));
                    |       ThemeManager.getCurrentTheme().drawFillInCaret(this, gd, xPos, 0, xPos, cursorHeight - 1);
11218.1.19  sbi@gol |    }

Eugenie Lyzenko wrote:

For an unknown reason, the offset for empty text is explicitly set to a different value.

I think the idea is to have cursor position just after the last offset in a case there is a text in field to avoid overlapping the cursor and the text before it. For empty text we can ignore this condition and put the cursor in a first available pixel.

In OE, the gap between the caret and the border is one pixel, and 10323a has now been updated to match this behavior, resulting in no visible issues.

Vladimir Tsichevski wrote:

Fixed in 10323a rev. 16067. Hynek, Eugenie, please, review.

My only question: what is the reason of disabling drawCaret() in FillIn.java?

Eugenie Lyzenko wrote:

Vladimir Tsichevski wrote:

Fixed in 10323a rev. 16067. Hynek, Eugenie, please, review.

My only question: what is the reason of disabling drawCaret() in FillIn.java?

Because, if the key is ignored, then nothing is drawn. The caret is not removed prior to this call, so it should not be redrawn. Rendering it in XOR mode inadvertently removes it, which is the root cause of the issue in question.

Vladimir Tsichevski wrote:

Eugenie Lyzenko wrote:

Vladimir Tsichevski wrote:

Fixed in 10323a rev. 16067. Hynek, Eugenie, please, review.

My only question: what is the reason of disabling drawCaret() in FillIn.java?

Because, if the key is ignored, then nothing is drawn. The caret is not removed prior to this call, so it should not be redrawn. Rendering it in XOR mode inadvertently removes it, which is the root cause of the issue in question.

OK. Got it.

Hynek Cihlar wrote:

Code review 10323a. The changes look good. Please also include ChUI regression tests.

Please, help me with running ChUI testing framework for this task.

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Code review 10323a. The changes look good. Please also include ChUI regression tests.

Please, help me with running ChUI testing framework for this task.

Sure, no problem.

Hynek Cihlar wrote:

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Code review 10323a. The changes look good. Please also include ChUI regression tests.

Please, help me with running ChUI testing framework for this task.

Sure, no problem.

I have rebased the branch to the latest trunk. Hynek, could you please perform the ChUI testing? I would like this fix to be merged as soon as possible, as the issue is complicating testing with Sikuli.

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Vladimir Tsichevski wrote:

Hynek Cihlar wrote:

Code review 10323a. The changes look good. Please also include ChUI regression tests.

Please, help me with running ChUI testing framework for this task.

Sure, no problem.

I have rebased the branch to the latest trunk. Hynek, could you please perform the ChUI testing? I would like this fix to be merged as soon as possible, as the issue is complicating testing with Sikuli.

ChUI tests are running.

ChUI tests passed.

Hynek Cihlar wrote:

ChUI tests passed.

We can proceed with the merge now.