<Swing Dev> [7] Review request for 7123767 Wrong tooltip location in Multi-Monitor configurations

[Pavel Porvatov]

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

2. adjustInsets changes values of arguments. So return the same value 
and assigning
workingBounds = adjustInsets(workingBounds, insets);
looks unnecessary

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

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
>>>>
>>>
>>>
>>
>
>

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: bug7123767.jtr
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20120904/93444794/bug7123767.jtr>