<Swing Dev> [9] Review request for 8167176 Exported elements referring to inaccessible types in java.desktop

[Philip Race]

+1 from me ...

-phil.

On 10/19/16, 8:22 AM, Alexandr Scherbatiy wrote:
>
> Hello,
>
> Could you review the updated fix:
>   http://cr.openjdk.java.net/~alexsch/8167176/webrev.02
>
>   - JRootPane.defaultPressAction/defaultReleaseAction fields are removed
>   - MetalBorders.bumps and MetalScrollBarUI.bumps are made private
>   - MetalFileChooserUI.createDirectoryComboBoxRenderer() method is 
> made private
>
> Thanks,
> Alexandr.
>
> On 10/18/2016 8:23 PM, Phil Race wrote:
>>
>>
>> On 10/18/2016 03:54 AM, Alexandr Scherbatiy wrote:
>>> On 10/18/2016 12:42 AM, Philip Race wrote:
>>>> Hi,
>>>>
>>>> First note that any of the alternatives here require an approved 
>>>> CCC before pushing.
>>>>
>>>> As I wrote in the bug the fields in JRootPane are not used. Making 
>>>> it a public supertype
>>>> is no more useful than just deleting it. This like hiding the peers 
>>>> which was a much
>>>> bigger change so please just delete these fields.
>>>>
>>>> I don't understand the reasoning behind changing "bumps" to be of 
>>>> the super-class type Icon
>>>> and introducing a new variable that is what is then used.
>>>>
>>>> Since "MetalBumps" is package private we should be able to safely 
>>>> assume no one has
>>>> been using these fields without the aid of reflection and I doubt 
>>>> anyone found a need.
>>>> And that won't work in JDK 9 with upcoming jigsaw changes
>>>> If you wanted people to usefully access these then you'd need to 
>>>> make MetalBumps public.
>>>> I *am not* suggesting that .. just pointing out that the 
>>>> alternatives I see are either
>>>> make it entirely public, or entirely hide is as being a mistake.
>>>> So make the fields all private and do not provide the non-useful 
>>>> public replacement.
>>>> And I mean remove today. In JDK 9. This is not as radical as it 
>>>> seems since left as
>>>> it is they are useless to anyone as you cannot use them.
>>>>
>>>> Also createDirectoryComboBoxRenderer(..) can be made private too.
>>>   All these fields and methods can be used with the public 
>>> super-class type in a user code.
>>>   For example the code below successfully compiled with JDK 8 and 9:
>>>   -------------
>>> public class Test {
>>>
>>>     static class CustomToolBarBorder extends 
>>> MetalBorders.ToolBarBorder {
>>>
>>>         public void doSomething(Graphics g) {
>>>             Icon metalBumps = bumps;
>>>             metalBumps.paintIcon(new JLabel(), g, 0, 0);
>>>         }
>>>     }
>>>
>>>     static class CustomMetalFileChooserUI extends MetalFileChooserUI {
>>>
>>>         public CustomMetalFileChooserUI(JFileChooser filechooser) {
>>>             super(filechooser);
>>>         }
>>>
>>>         public void doSomething() {
>>>             DefaultListCellRenderer listCellRenderer = 
>>> createDirectoryComboBoxRenderer(getFileChooser());
>>>             // do something with the listCellRenderer
>>>         }
>>>     }
>>
>> See if you can find any 3rd party code at all that uses any of these ...
>>
>>>
>>>     static class CustomJRootPane extends JRootPane {
>>>
>>>         public void doSomething() {
>>>             AbstractAction pressAction = defaultPressAction != null
>>>                     ? defaultPressAction
>>>                     : new CustomAction();
>>>             AbstractAction releaseAction = defaultReleaseAction != null
>>>                     ? defaultReleaseAction
>>>                     : new CustomAction();
>>>         }
>>>     }
>>> }
>>
>> This example is weird since defaultPressAction and 
>> defaultReleaseAction are ALWAYS null
>> unless the application itself set them to something.
>> It is completely acceptable to me to delete these fields.
>>
>>
>>>   -------------
>>>
>>>   I just need to be sure that the incompatible change introduced by 
>>> removing these fields or making them private is acceptable.
>>
>>>
>>>>
>>>> Finally, when these are resolved you should be able to re-enable 
>>>> lint !
>>>> http://hg.openjdk.java.net/jdk9/dev/rev/81435a812f59
>>>   I prepared the hint enabling fix but I believe it should be 
>>> reviewed on the build-dev and jigsaw-dev aliases after the current 
>>> fix is pushed.
>>
>> I don't think this is at all necessary. The warning has nothing to do 
>> with jigsaw and isn't a build change per se.
>> It should be included with this webrev+subsequent push, under this 
>> bug ID.
>>
>>
>> -phil.
>>
>>>
>>>   Thanks,
>>>   Alexandr.
>>>>
>>>> -phil.
>>>>
>>>> On 10/17/16, 4:22 AM, Alexandr Scherbatiy wrote:
>>>>>
>>>>>   Hello,
>>>>>
>>>>>   Could you review the updated fix:
>>>>>     http://cr.openjdk.java.net/~alexsch/8167176/webrev.01
>>>>>
>>>>>   MetalBorders.bumps and MetalScrollBarUI.bumps fields are nor 
>>>>> marked for removal any more.
>>>>>
>>>>>   Thanks,
>>>>>   Alexandr.
>>>>>
>>>>> On 10/14/2016 3:23 PM, Sergey Bylokhov wrote:
>>>>>> Is it necessary to remove tese fields? For example 
>>>>>> MetalScrollBarUI.bumps looks similar to other fields 
>>>>>> MetalScrollButton.increaseButton/decreaseButton.
>>>>>>
>>>>>> On 13.10.16 16:15, Alexander Scherbatiy wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> Could you review the fix:
>>>>>>>   bug: https://bugs.openjdk.java.net/browse/JDK-8167176
>>>>>>>   webrev: http://cr.openjdk.java.net/~alexsch/8167176/webrev.00
>>>>>>>   - Inaccessible classes returned by public API in swing are 
>>>>>>> changed to
>>>>>>> their public super classes.
>>>>>>>   - Fields which expose inaccessible classes are deprecated and 
>>>>>>> marked
>>>>>>> for removal.
>>>>>>>
>>>>>>>   Changing a public field type to its superclass is incompatible 
>>>>>>> change
>>>>>>> with previous releases. However it is not possible to use a 
>>>>>>> class which
>>>>>>> is not exported in JDK 9 because of the modularization feature.
>>>>>>>
>>>>>>>   Thanks,
>>>>>>>   Alexandr.
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>
>>
>