On Thu, 15 Jun 2023 19:33:08 GMT, Phil Race <[email protected]> wrote:

> Hmm, I did ask during the previous review if there was any reason to block 
> with _SetScrollPos or "the case below" - ie _SetSpans... but perhaps it is 
> more complex than that.

I didn't find any reason to block either.

I considered `_SetInsets` for using `InvokeFunction` or `InvokeFunctionLater`. 
I deliberately decided not to.

But I eventually missed the case where `SetInsets` *depends* on the results of 
`SetSpans` when they're called together in `WScrollPanePeer.childResized`.

(In my initial approach, I used `InvokeFunction` because I overlooked 
`InvokeFunctionLater` at first; they're not close in the header file and in the 
implementation files. Yet I was bothered by blocking EDT where it wasn't 
needed. I found `InvokeFunctionLater` and used it — the result was the same.)

> I need some clarification Before any changes, using SyncCall , _SetSpans was 
> called synchronously. After the first change to use InvokeFunctionLater, 
> _SetSpans was called asynchronously .

Yes, when using `SyncCall` both `_SetSpans` and `_SetInsets` were called 
synchronously, so `_SetInsets` could use the updated spans.

> Now with InvokeFunction, _SetSpans is again called synchronously

No, it's still called *asynchronously*. 

> So why do you also need the change to set it in native ?

It is the reason why I moved `setInsets` from Java into native implementation.

Without moving SetInsets into the native, both `_SetSpans` and `_SetInsets` 
need to blocking calls that follow each other. In my opinion, it's not a good 
approach. I elaborated on it in [my reply to Harshitha's 
comment](https://github.com/openjdk/jdk/pull/14478#issuecomment-1593817579).

 
> The difference I can see is that although it is synchronous it now requires 
> jumping on to the Toolkit thread and so other pending processing on the 
> Toolkit thread will run first.
> 
> But I'm not sure that's relevant here.
> 
> So can you expand on why you also had to move the call to setInsets() to 
> native ?

To avoid making both `_SetSpans` and `_SetInsets` synchronous calls;
To avoid another JNI call when both actions could be easily coalesced.

`SetInsets` must always be called after `SetSpans`. Why do we need to call both 
from Java side, where native implementation for `SetSpans` can as easily call 
it directly?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/14478#issuecomment-1593833346

Reply via email to