Re: [7] Review request for 7123767 Wrong tooltip location in Multi-Monitor configurations

[Alexander Potochkin]

Hello Vlad

The fix looks good to me as well
(remember, you need 2 reviewers for 7u)

Thanks
alexp

On 9/12/2012 6:54 PM, Pavel Porvatov wrote:

Hi Vladislav,

The fix looks good for me. See also comments below

Hello Pavel, all,

I've reverted some of the changes, please find new webrev here: http://cr.openjdk.java.net/~vkarnauk/7123767/webrev.05/

Though the diff is smaller indeed, it makes me somewhat curious why we tend to keep old poorly written and formatted code...

There are some reasons:
1. If all lines are reformatted it's hard to review the fix
2. After such changes it's hard to track changes in files
3. It's hard to backport such changes (because of conflicts)

4. It's hard to backport another changes after such changes (because of conflicts)

Regards, Pavel

Regards,
- Vlad

On 9/12/2012 3:16 PM, Pavel Porvatov wrote:

Hi Vladislav,

Hello Pavel, all,

please find new webrev here: http://cr.openjdk.java.net/~vkarnauk/7123767/webrev.04/

Now the test passes. The last comment from me is:

could you please don't rename existing variables in the ToolTipManager.java (sorry, I noticed this only now...)? If you rename back drawingGC into gc and workingBounds into sBounds the diff will be much smaller. When you do that be sure that there is no lines that are just reformatted.

Regards, Pavel

Please see my comments inline.

Regards,
- Vlad

On 9/4/2012 8:35 PM, Pavel Porvatov wrote:

Hi Vladislav,

I have several comments for the fix:
1.
           if(preferredLocation != null) {
               location = toFind;
               if (!leftToRight) {
                   location.x -= size.width;
               }
           } else {
               location.x = screenLocation.x + mouseEvent.getX();

location.y = screenLocation.y + mouseEvent.getY() + 20;

In case "preferredLocation != null" creation of new object
Point location = new Point();
is not used, so you could put location = new Point() in else block

Agreed, fixed.

2. adjustInsets changes values of arguments. So return the same value and assigning

workingBounds = adjustInsets(workingBounds, insets);
looks unnecessary

Agreed. I've removed adjustInsets() completely.

3. I run the test via jtreg and it failed on my environment with the attached log. Could you please investigate that?

My initial approach for the test was incorrect, I've run on several issues on Mac platform. I've re-written the test completely and run it against jdk7 and jdk8 on Windows and Mac OS X.

Regards, Pavel

Hello Pavel, all,

thank you for the review, please find new webrev here:
http://cr.openjdk.java.net/~vkarnauk/7123767/webrev.03/

Please see my comments inline.

Regards,
- Vlad

On 8/31/2012 8:50 PM, Pavel Porvatov wrote:

Hi Vladislav,

I have several comments about the fix:

1. getDrawingRect: now localInsets should be requested only if localRect.contains(toFind) is true

2.

+ // Something's really wrong here - use the screen that our

+                // Component belongs to
+                if (workingBounds == null)

+ workingBounds = insideComponent.getGraphicsConfiguration().getBounds();

a. You should always use {...} for if construction

Fixed.

b. It seems we should invoke adjustInsets there. To reduce the code I think you can rename getDrawingRect into getGraphicsConfiguration and return GraphicsConfiguration there. And only when GraphicsConfiguration is found calculate adjusted bounds...

Agreed. Fixed.

3. getDrawingRect comment should have two ")" at the end

Fixed.

Abou the test:
1. You should use Toolkit.realSync after setVisible

Fixed.

2. I run the test on jdk1.8.0b51 and it passes. Looks strange...

The reason why it didn't fail on non-patched JDK8 is because line 184

 183         Component component = e.getChild();
 184         if (component.isShowing()) {

185 if (!config.equals(getGCByPoint(component.getLocationOnScreen()))) {

returned "false" as AWT event queue was processed slower than test thread, thus my check never actually happened. I've changed these logic (please see new webrev), now this condition is processed correctly. Test works on patched JDK and fails on non-patched JDK6, 7, and 8.

Regards, Pavel

Hi Pavel, all,

please find new webrev here: http://cr.openjdk.java.net/~vkarnauk/7123767/webrev.02/

Please see my comments inline.

Regards,
- Vlad

On 8/17/2012 9:10 PM, Pavel Porvatov wrote:

Hi Vladislav,

I have several comments about the fix:

1. javax.swing.ToolTipManager#showTipWindow: I think toFind variable should be initialized in if(preferredLocation != null) { ....} else {...} block. It looks more logical and code will be more compact (adjustedPreferredLocation can be removed at all, I think)

Agreed. I've re-worked this piece.

2. Could you please explain why you are using union of all rectangles in the getDrawingRect method? In case two monitors are aligned on a diagonal totalRect will contain areas that are not covered by screen devices and popups can be placed incorrectly.

Agreed again. I've re-designed getDrawingRect() function and placed additional condition into showTipWindow() function; now every situation should be handled correctly.

Automated test was added, please see the webrev.

Regards,
- Vlad

Regards, Pavel

Hello,

please review the fix for 7123767: Wrong tooltip location in Multi-Monitor configurations

jdk7 webrev: http://cr.openjdk.java.net/~vkarnauk/7123767/webrev.00/

test source: http://cr.openjdk.java.net/~vkarnauk/7123767/test/

bug description: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7123767

This is also a customer escalated issue in jdk6.

On multi-monitor configurations Component.getLocationOnScreen() may return negative values. Additionally, ToolTipManager.showTipWindow() method calculates tip window location from Component' getLocationOnScreen() return data and draws tip window using the same GraphicsConfiguration instance as component's top-left corner, which again can be of negative value. When component is stretched across multiple screen devices, this may lead to a situation when it's tooltip is drawn on wrong monitor.

To fix this we should count all GraphicsConfiguration instances and determine correct screen device to draw on. We should also take into account that:

1) tip window preferred location could be set

2) preferred location could be greater than total virtual bounds of all monitors. To make tip window visible in such case we move it to the corresponding corner of total virtual bounds 3) exact order of monitors (left-to-right and top-to-bottom) is unknown

I run jtreg against ToolTipManager tests and all tests passed (I run it several times with different monitor configurations). I also tested the fix with custom test program (please see the link provided) with 4 possible monitor configurations (left-to-right, right-to-left, top-to-bottom and bottom-to-top) and saw no issues.

I added no new automated test(s) because this at least would require multiple monitors to run.

Regards,
- Vlad