FWD » Core Development » User Interface

In the Swing client, ./combo_box/combo_box9_1.p can be used to see the problem. This is only a button drawing issue. The drop-down box does scroll in response to the button presses. The issue is that the scrollbar buttons don't animate their drawing of a pressed and then released button. The button always draws in the released state.

There is another problem with GUI button pressed state and possibly related. When button is pressed in ALERT-BOX (and possibly elsewhere) and mouse is moved while holding button down, the button down draw state changes to button up. Tested in Swing GUI task branch 2677a.

The point #2 is still under resolution. Be ready later today.

Code Review evl_upd20151124d.diff

My primary concern is whether the new approach has any negative ramifications for the web client. The web client's mouse event support is quite tricky and we have been trying to make it more independent of the Java side. We go to great lengths to process as much as possible on the javascript side.

Constantin: please review.

Greg Shah wrote:

Code Review evl_upd20151124d.diff

My primary concern is whether the new approach has any negative ramifications for the web client. The web client's mouse event support is quite tricky and we have been trying to make it more independent of the Java side. We go to great lengths to process as much as possible on the javascript side.

Constantin: please review.

1. in the Web client, there is a small flash (on the scrollbar, I guess from redrawing) on initial button down of a drop-down's scroll button; otherwise the Web client responds OK.
2. the ScrollBarGuiButtonImpl.up field needs to be made volatile
3. both ScrollBarGuiImpl.mouseInputExecutor and ScrollBarGuiButton.mouseInputExecutor need to be stopped, when the widget is destroyed (otherwise there will be a thread leak).

Eugenie: please resolve items 2 and 3 from Constantin's feedback.

I'll add a task for the web client flashing issue.

Eugenie Lyzenko wrote:

This is implementation for auto-scroll mode as reaction to press and hold the mouse on scrollbar button.

Btw., why is the extra thread needed? Can't we just redraw the button back up on a mouse button released event?

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

This is implementation for auto-scroll mode as reaction to press and hold the mouse on scrollbar button.

Btw., why is the extra thread needed? Can't we just redraw the button back up on a mouse button released event?

I see there is the repeated-clicks going on.

In any case, the event click should be dispatched on the main thread, otherwise this is a potential race condition. Also any thread-local storage would not work. See how this is done in ScrollBarGuiImpl.mousePressed().

Btw., why is the extra thread needed? Can't we just redraw the button back up on a mouse button released event?

No we can't. Because mouse remains pressed during this process. No mouse release event.

I see there is the repeated-clicks going on.

This is not repeating clicks. This is repeating reaction to click. There is a difference.

Code Review evl_upd20151125a.diff

I'm OK with the changes.

Hynek/Constantin: do you have any remaining concerns?

Eugenie Lyzenko wrote:

Greg Shah wrote:

Eugenie: please resolve items 2 and 3 from Constantin's feedback.

Done with evl_upd20151125a.diff, please review.

You've made the ScrollBarGuiButton.mouseInputExecutor static ... I don't think you wanted to remain so, as you added code in destroy() to shutdown the executor.

Greg Shah wrote:

Code Review evl_upd20151125a.diff

I'm OK with the changes.

Hynek/Constantin: do you have any remaining concerns?

My remaining concern is the threading issue - the call to independentEventDrawingBracket() should go through the event queue, it must not be executed directly on the worker thread.

Eugenie, I think you can use ThinClient.invokeLater() for that or just create and post a mouse event.

Eugenie Lyzenko wrote:

The auto scroll should work only when mouse is down. We need to react immediately. And this is safe because while mouse is down we can not do anything by mouse.

I don't agree. Other events are processed when the mouse button is down. You can move the mouse, press other mouse buttons, use the keyboard, there may be server events coming in, etc. Even the logic executed by the worker thread itself posts paint events to the event queue. All the generated events are being dispatched on the main thread simultaneously with the worker thread logic and both may access the same unprotected resources.

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

I don't agree. Other events are processed when the mouse button is down. You can move the mouse, press other mouse buttons, use the keyboard, there may be server events coming in, etc. Even the logic executed by the worker thread itself posts paint events to the event queue. All the generated events are being dispatched on the main thread simultaneously with the worker thread logic and both may access the same unprotected resources.

But not for the container we are scrolling in such a way. And for the time we are scrolling.

Eugenie Lyzenko wrote:

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

I don't agree. Other events are processed when the mouse button is down. You can move the mouse, press other mouse buttons, use the keyboard, there may be server events coming in, etc. Even the logic executed by the worker thread itself posts paint events to the event queue. All the generated events are being dispatched on the main thread simultaneously with the worker thread logic and both may access the same unprotected resources.

But not for the container we are scrolling in such a way.

The unprotected resources accessed by both threads are not limited to a container.

And for the time we are scrolling.

No. During the time you are scrolling, the application is still processing input.

And even if you just click on the button you also get the race condition. There is no guarantee the two threads (main and the worker thread in the executor) won't access the same piece of data at the same time.

Eugenie Lyzenko wrote:

Hynek Cihlar wrote:

No. During the time you are scrolling, the application is still processing input.

And even if you just click on the button you also get the race condition. There is no guarantee the two threads (main and the worker thread in the executor) won't access the same piece of data at the same time.

OK. Here is the new implementation.

Very well.

We also need to get rid of the ThinClient.eventDrawingBracket() call, the result of ThinClient.getInstance() is also a shared piece of memory that must not be accessed by two threads simultaneously and we want to also avoid processing the posted event on the worker thread. When you remove ThinClient.eventDrawingBracket() the posted event will be processed by the event loop on the main thread, which is what we desire.

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

Very well.

We also need to get rid of the ThinClient.eventDrawingBracket() call, the result of ThinClient.getInstance() is also a shared piece of memory that must not be accessed by two threads simultaneously and we want to also avoid processing the posted event on the worker thread. When you remove ThinClient.eventDrawingBracket() the posted event will be processed by the event loop on the main thread, which is what we desire.

This will cause the scrolling will not be visible. So this is the key point. The posting event approach ensures the events will be handled sequentially in addition to all other events are possibly coming.

Eugenie Lyzenko wrote:

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

Very well.

We also need to get rid of the ThinClient.eventDrawingBracket() call, the result of ThinClient.getInstance() is also a shared piece of memory that must not be accessed by two threads simultaneously and we want to also avoid processing the posted event on the worker thread. When you remove ThinClient.eventDrawingBracket() the posted event will be processed by the event loop on the main thread, which is what we desire.

This will cause the scrolling will not be visible.

Do you mean the scroll bar is not redrawn? Is the scroll event properly processed, is the scroll bar state updated?

Do you mean the scroll bar is not redrawn? Is the scroll event properly processed, is the scroll bar state updated?

I mean neither scrollbar state itself is changing nor scrollable panel is scrolling while mouse is pressing. Until the button is releasing. Then all become properly updated. But this behavior is not correct and confuses the user. We need scrolling while mouse is pressing, not after.

Eugenie Lyzenko wrote:

Do you mean the scroll bar is not redrawn? Is the scroll event properly processed, is the scroll bar state updated?

I mean neither scrollbar state itself is changing nor scrollable panel is scrolling while mouse is pressing. Until the button is releasing. Then all become properly updated. But this behavior is not correct and confuses the user. We need scrolling while mouse is pressing, not after.

You have to use ThinClient.postOSEvent() so that the event manager is notified about the posted events.

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

You have to use ThinClient.postOSEvent() so that the event manager is notified about the posted events.

This does not work as well.

Eugenie Lyzenko wrote:

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

You have to use ThinClient.postOSEvent() so that the event manager is notified about the posted events.

This does not work as well.

Interesting, this works for me fine. I have applied evl_upd20151125c.diff, removed eventDrawingBracket() and used ThinClient.postOSEvent() to post the scroll event. When I press left mouse button and hold the scroll bar is continuously scrolling.

Can you attach your diff?

Btw., the same problem is in ScrollBarGuiImpl.mousePressed(). EventManager.postEvent() must be replaced with ThinClient.postOSEvent(). Eugenie, will you please update this as well?

Hynek Cihlar wrote:

Btw., the same problem is in ScrollBarGuiImpl.mousePressed(). EventManager.postEvent() must be replaced with ThinClient.postOSEvent(). Eugenie, will you please update this as well?

OK.

You are right, the ThinClient.postOSEvent() works fine. Previously I used EventManager.postOSEvent() and it was problematic.

After I'll refresh 2677b put new diff here.

Eugenie Lyzenko wrote:

This is the updated diff for review.

The threading issue is resolved.

Just one thing for future consideration. For UI "animations" (like auto scrolling) we should probably need a common executor managed by the framework, rather than each individual widget (and widget instance) managing its own.

Hynek Cihlar wrote:

Eugenie Lyzenko wrote:

The threading issue is resolved.

Just one thing for future consideration. For UI "animations" (like auto scrolling) we should probably need a common executor managed by the framework, rather than each individual widget (and widget instance) managing its own.

Greg, Hynek, Constantin,

So can I commit this into 2677b?

We know there are some regressions in the GUI testcases for 2677b. Let's see how significant these are before we decide if this will be checked in to 2677b.

Go ahead and check these into 2677b.

The evl_upd20151125a.diff has been committed in 2677b as 10977.