Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8257414: Drag n Drop target area is wrong on high DPI systems #1907

Closed
wants to merge 3 commits into from

Conversation

@omikhaltsova
Copy link

@omikhaltsova omikhaltsova commented Dec 29, 2020

Please, review this small fix for drag-n-drop on Linux with HiDPI turned on!

This bug is due to the following reason: while scaling Java recalculates resolution (W x H) according to sun.java2d.uiScale (W/SCALE x H/SCALE) and works inside these new coordinates but at the same time the events, that come from the system, continue reporting positions in the old coordinates (W x H).

The idea of the suggested fix is in division of coordinates on the scale when they come from the system to Java and multiplying them on the scale when they go back from Java to the system. It is similar to processing events from mouse and buttons.

Testing is quite complicated because for reproducing this bug the following conditions should be met:

  1. HiDPI is turned on
  2. sun.java2d.uiScale.enabled = true and sun.java2d.uiScale != 100%
  3. the source of drag-n-drop is non-java application

The step-by-step guide how to reproduce this bug is added to https://bugs.openjdk.java.net/browse/JDK-8257414.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8257414: Drag n Drop target area is wrong on high DPI systems

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1907/head:pull/1907
$ git checkout pull/1907

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 29, 2020

👋 Welcome back omikhaltcova! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Dec 29, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 29, 2020

@omikhaltsova The following label will be automatically applied to this pull request:

  • awt

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the awt label Dec 29, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 29, 2020

Webrevs

@@ -613,7 +613,7 @@ private boolean processXdndPosition(XClientMessageEvent xclient) {
if (xwindow != null) {
/* Translate mouse position from root coordinates
to the target window coordinates. */
Point p = xwindow.toLocal(x, y);
Point p = xwindow.toLocal(xwindow.scaleDown(x), xwindow.scaleDown(y));
Copy link
Member

@mrserb mrserb Jan 4, 2021

I guess some clarification here is required. It is unclear what the coordinate system is used by the x/y above. Looks like before the fix the device space was always used, and after the fix the mix of device/user's space will be used.
If the user's space should always be used(the scaled version of the device), then I suggest to scale these coordinates here:

        x = (int)(xclient.get_data(2) >> 16);
        y = (int)(xclient.get_data(2) & 0xFFFF);

But you will need to check that all usage of the new coordinate system will be valid across the method.

Copy link
Author

@omikhaltsova omikhaltsova Jan 13, 2021

Sergey, thank you! Could you give me a hint please concerning some use case of drag-and-drop to meet the (xwindow == 0) condition in XDnDDropTargetProtocol::processXdndPosition(..)? I'm unable to do it by means of the tests' apps I have.

Copy link
Member

@mrserb mrserb Jan 16, 2021

Sergey, thank you! Could you give me a hint please concerning some use case of drag-and-drop to meet the (xwindow == 0) condition in XDnDDropTargetProtocol::processXdndPosition(..)? I'm unable to do it by means of the tests' apps I have.

I think it is somehow related to the embedded frame when the event comes from some native window that is not registered as a java peer(but some external native app/window)

Copy link
Author

@omikhaltsova omikhaltsova Jan 18, 2021

I failed to test branches of XDnDDropTargetProtocol::processXdndPosition(..) under condition (xwindow == null) by means of EmbeddedFrame. In addition none of automatic regression tests (test/jdk/java/awt/dnd/, test/jdk/java/awt/xembed/, test/jdk/sun/awt) gets into these branches.

I also tried to scale coordinates (as you advised) right after:

        x = (int)(xclient.get_data(2) >> 16);
        y = (int)(xclient.get_data(2) & 0xFFFF);

as follow:

GraphicsConfiguration gc = GraphicsEnvironment
                .getLocalGraphicsEnvironment()
                .getDefaultScreenDevice()
                .getDefaultConfiguration();
        gc = SunGraphicsEnvironment.getGraphicsConfigurationAtPoint(gc, x, y);
        GraphicsDevice device = gc.getDevice();
        if (device instanceof X11GraphicsDevice) {
            int scale = ((X11GraphicsDevice) device).getScaleFactor();
            x = XlibUtil.scaleDown(x, scale);
            y = XlibUtil.scaleDown(y, scale);
        }

but found out that getGraphicsConfigurationAtPoint() also requires the already scaled coordinates.

There are 3 different places in XDnDDropTargetProtocol::processXdndPosition(..) where it is necessary to decide whether to scale coordinates or not:

  1. I'm not sure and I'm unable to test it:
if (xwindow == null)
        {
            long receiver =
                XDropTargetRegistry.getRegistry().getEmbeddedDropSite(
                    xclient.get_window(), x, y);
  1. I'm quite sure and I'm able to test it:
if (xwindow != null) {
            /* Translate mouse position from root coordinates
               to the target window coordinates. */
            Point p = xwindow.toLocal(xwindow.scaleDown(x), xwindow.scaleDown(y));
  1. I think that scaling is necessary but I'm unable to test it (additionally the question raises concerning this case, why the coordinates are not converted to local here similar to the 2 case):
if (xwindow == null) {
            if (targetXWindow != null) {
                notifyProtocolListener(targetXWindow, x, y,
                                       DnDConstants.ACTION_NONE, xclient,
                                       MouseEvent.MOUSE_EXITED);

Sergey, is it possible at the moment to fix the particular issue and leave the other use cases to work as it works before due to no possibility to test them?

Copy link
Member

@mrserb mrserb Jan 20, 2021

There are 3 different places in XDnDDropTargetProtocol::processXdndPosition(..) where it is necessary to decide whether to scale coordinates or not:

Unfortunately, this is the purpose of the whole fix to decide when the scale is required and then not, otherwise we can simply break the untested cases.

Copy link
Author

@omikhaltsova omikhaltsova Feb 2, 2021

Sergey, please, review my last changes! Finally I managed to check all necessary branches in XDnDDropTargetProtocol::processXdndPosition(..). The user's space (the scaled version of the device) must be used everywhere here. I had to devide scaling according to condition (xwindow != null) in order to use more simple and fast realization for most of cases. The scaling for the case (xwindow == null) related to the embedded frame is based on the idea that there is only one pointer (cursor) in the system and it is used for drag-n-drop operation. I tried different ways to get scale in the last case but only this one is workable. If you have some better idea, tell me, please, another way to do it!

XToolkit.awtLock();
try {
for (int i = 0; i < gdslen; i++) {
long screenRoot = XlibWrapper.RootWindow(display, i);
Copy link
Member

@mrserb mrserb Feb 6, 2021

I am not sure this will work in the multi-monitors configuration when the screen is split across a few monitors(when Xinerama is enabled for example). In such a configuration, only one root window and only one X11 screen exist. And the index of the GraphicsDevice is the index of the "virtual" monitor, so you cannot pass this index to the "XlibWrapper.RootWindow".

A few questions:

  • Are you sure that to scale the x/y in the device space you need to find where the pointer is located? And not where the x/y coordinates are located themselves?
  • Did you try to iterate over the devices->downscale the bounds of each GD to the device scale and upscale the x/y using the selected device. Such utility method can be added to the SunGraphicsEnvironment

Copy link
Author

@omikhaltsova omikhaltsova Feb 12, 2021

Sergey, thanks for the hint! Hope I understand you right. Please, review my changes!

I decided to do necessary scaling in place and not to add an utility method to SunGraphicsEnvironment due to the following doubts:
the methods from SunGraphicsEnvironment can be called from different OS and I'm going to use GraphicsConfig::getBounds() but I see that the Unix implementation includes downscaling (X11GraphicsConfig::getBounds(), src/java.desktop/unix/classes/sun/awt/X11GraphicsConfig.java) and the Windows one is not (Win32GraphicsConfig::getBounds(), src/java.desktop/windows/classes/sun/awt/Win32GraphicsConfig.java);
so for Unix I need upscaling before checking the point position but for Windows it is not needed.
Please, correct me if I'm wrong!

Concerning my previous implementation based on the cursor position, would you be so kind to take a look at XMouseInfoPeer::fillPointWithCoords(..) (src/java.desktop/unix/classes/sun/awt/X11/XMouseInfoPeer.java) in order to check whether it should be also fixed?

Copy link
Member

@mrserb mrserb Feb 23, 2021

Yes, I think the fillPointWithCoords is also broken.

I'll run the tests for this change.

mrserb
mrserb approved these changes Feb 27, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 27, 2021

@omikhaltsova This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8257414: Drag n Drop target area is wrong on high DPI systems

Reviewed-by: serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 817 new commits pushed to the master branch:

  • 20c93b3: 8261914: IfNode::fold_compares_helper faces non-canonicalized bool when running JRuby JSON workload
  • ddd550a: 8261308: C2: assert(inner->is_valid_counted_loop(T_INT) && inner->is_strip_mined()) failed: OuterStripMinedLoop should have been removed
  • 03d888f: 8261804: Remove field _processing_is_mt, calculate it instead
  • 6800ba4: 8257500: Drawing MultiResolutionImage with ImageObserver "leaks" memory
  • 65a245e: 8262329: Fix JFR parser exception messages
  • a4c2496: 8259535: ECDSA SignatureValue do not always have the specified length
  • 2515c42: 8262332: serviceability/sa/ClhsdbJhisto.java fails with Test ERROR java.lang.RuntimeException: 'ParselTongue' missing from stdout/stderr
  • 07061fc: 8256417: Exclude TestJFRWithJMX test from running with PodMan
  • c9e9189: 8262074: Consolidate the default value of MetaspaceSize
  • 05c11bc: 8262426: Change TRAPS to Thread* for find_constrained_instance_or_array_klass()
  • ... and 807 more: https://git.openjdk.java.net/jdk/compare/07c93fab8506b844e3689fad92ec355ab0dd3c54...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mrserb) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Feb 27, 2021
@omikhaltsova
Copy link
Author

@omikhaltsova omikhaltsova commented Mar 1, 2021

Sergey, thank you for reviewing!

/integrate

@openjdk openjdk bot added the sponsor label Mar 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 1, 2021

@omikhaltsova
Your change (at version d8259bd) is now ready to be sponsored by a Committer.

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 2, 2021

/sponsor

@openjdk openjdk bot closed this Mar 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 2, 2021

@mrserb @omikhaltsova Since your change was applied there have been 828 commits pushed to the master branch:

  • 353416f: 8262509: JSSE Server should check the legacy version in TLSv1.3 ClientHello
  • 642f45f: 8261839: Error creating runtime package on macos without mac-package-identifier
  • 682e120: 8262497: Delete unused utility methods in ICC_Profile class
  • 4c9adce: 8262379: Add regression test for JDK-8257746
  • 6baecf3: 8259937: guarantee(loc != NULL) failed: missing saved register with native invoker
  • c569f1d: 8262085: Hovering Metal HTML Tooltips in different windows cause IllegalArgExc on Linux
  • 75bf106: 8262028: Make InstanceKlass::implementor return InstanceKlass
  • fe8e370: 8262188: Add test to verify trace page sizes logging on Linux
  • 0a7fff4: 8261636: The test mapping in hugetlbfs_sanity_check should consider LargePageSizeInBytes
  • 702ca62: 8262185: G1: Prune collection set candidates early
  • ... and 818 more: https://git.openjdk.java.net/jdk/compare/07c93fab8506b844e3689fad92ec355ab0dd3c54...master

Your commit was automatically rebased without conflicts.

Pushed as commit d339832.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants