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

[Vladislav Karnaukhov]

Hello Pavel, all,

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

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