<Swing Dev> [7] Review request for 7123767 Wrong tooltip location in Multi-Monitor configurations
Alexander Potochkin
alexander.potochkin at oracle.com
Wed Sep 12 15:03:46 UTC 2012
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
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
More information about the swing-dev
mailing list