<Swing Dev> [7] Review request for 7123767 Wrong tooltip location in Multi-Monitor configurations
Vladislav Karnaukhov
Vladislav.Karnaukhov at oracle.com
Wed Sep 12 13:19:43 UTC 2012
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...
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
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
More information about the swing-dev
mailing list