search

JetBrains / JetBrainsRuntime

Runtime environment based on OpenJDK for running IntelliJ Platform-based products on Windows, macOS, and Linux
GNU General Public License v2.0
1.28k stars 193 forks source link

JBR-7495 Provide CursorEqualityCheck for JBR-API #436

Closed stachenov closed 1 week ago

stachenov commented 1 month ago

There seems to be a bug in macOS making nativeSetBuiltInCursor do nothing at times. This API provides a workaround, as repeated calls usually succeed.

Here's the corresponding JBR-API PR: https://github.com/JetBrains/JetBrainsRuntimeApi/pull/2

YaaZ commented 1 month ago

What causes nativeSetBuiltInCursor to do nothing? Is it possible to fix this instead?

stachenov commented 1 month ago

That would be perfect, but I've no idea how to debug it, as AppKit is closed source, and I was only able to trace it to [NSCursor set] call in the JBR. If I were to guess, the underlying cause may be the same as JBR-7481. In both cases the rounded corners screw up the expected behavior, so if macOS "thinks" that the mouse isn't really in that window, it might as well just ignore an attempt to set cursor if it originates from that window. But it's nothing more than just a wild guess.

From my observations, there aren't any differences between the cases when it works as expected and when it doesn't. You can try running the reproducer from JBR-7484 yourself and see. In both cases the cursor is set correctly, but in most cases it actually changes, but sometimes it just doesn't.

YaaZ commented 1 month ago

What if we always skip the check on Java side, is the performance regression really noticeable?

stachenov commented 1 month ago

I don't know for sure, but it could be. It's invoked on pretty much any mouse event.

YaaZ commented 1 month ago

I mean, if the behavior is inconsistent, we better focus on making it more robust, than relying on obscure workarounds. But anyway, let's see what @NikitkoCent says.

stachenov commented 1 month ago

Yes, I see your point. If we knew that it wouldn't lead to noticeable performance regressions, that would certainly be the way. Do we have to way to measure the performance cost and be able to tell for sure if it's OK?

stachenov commented 2 weeks ago

Looks like it was done in the past already, and then reverted because of performance issues:

IDEA-139791 Wrong cursor in OS X Yosemite IDEA-167733 High CPU usage due to Component.setCursor

So disabling the check completely doesn't seem to be an option. Temporarily disabling it during resize operations could be OK, but then we need this API.

YaaZ commented 1 week ago

@NikitkoCent so are we closing this review?

stachenov commented 1 week ago

I think it can be closed, yes. The details are to be discussed, but the idea of the better fix seems to be working perfectly. The new fix is here:

https://github.com/JetBrains/JetBrainsRuntime/pull/455