<Swing Dev> Review Request of 8137169 : [macosx] Incorrect minimal heigh of JTabbedPane with more tabs
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Wed Mar 23 10:44:11 UTC 2016
On 23/03/16 14:07, Avik Niyogi wrote:
>
>> On 23-Mar-2016, at 3:31 pm, Alexander Scherbatiy
>> <alexandr.scherbatiy at oracle.com
>> <mailto:alexandr.scherbatiy at oracle.com>> wrote:
>>
>> On 21/03/16 09:19, Avik Niyogi wrote:
>>> Hi Alexander,
>>> I agree with what you said regarding the look and feel looking
>>> different. But this bug arrises due to setting of
>>> TabbedPaneScrollLayout only. If Scroll Layout is not meant for Aqua
>>> look and feel should not the setting of this parameter instead throw
>>> a helpful error saying this parameter is not accepted
>> According to the JTabbedPane.setTabLayoutPolicy(int
>> tabLayoutPolicy) javadoc:
>> "Some look and feels might only support a subset of the possible
>> layout policies, in which case the value of this property may be
>> ignored."
>>
>> Aqua L&F ignores WRAP_TAB_LAYOUT for JTabbedPane tabs layouting and
>> always use SCROLL_TAB_LAYOUT. No exception should be thrown in this case.
>
> Actually, it is doing the other way around for Aqua L&F. It is
> defaulting WRAP_TAB_LAYOUT and setting SCROLL_TAB_LAYOUT is still
> setting a subclass of TabbedPaneLayout and not TabbedPaneScrollLayout.
According to the JTabbedPane javadoc:
SCROLL_TAB_LAYOUT: Tab layout policy for providing a subset of
available tabs when all the tabs will not fit within a single run.
WRAP_TAB_LAYOUT The tab layout policy for wrapping tabs in multiple
runs when all tabs will not fit within a single run.
The Aqua L&F uses only AquaTabbedPaneUI and AquaTabbedPaneContrastUI
for tabbed pane UI and they both returns AquaTruncatingTabbedPaneLayout
which has been designed for to place tabs according to the
SCROLL_TAB_LAYOUT.
Thanks,
Alexandr.
>
>> Thanks,
>> Alexandr.
>>
>>> instead of absorbing this parameter and letting it render itself
>>> into a dummy node which does not proceed further with this
>>> parameter? Maybe we need to discuss what the expected behaviour may
>>> be. Also, thank you for the inputs regarding how to proceed with
>>> removing duplicate code.
>>>
>>> With Regards,
>>> Avik Niyogi
>>>> On 19-Mar-2016, at 1:52 am, Alexander Scherbatiy
>>>> <alexandr.scherbatiy at oracle.com
>>>> <mailto:alexandr.scherbatiy at oracle.com>> wrote:
>>>>
>>>>
>>>> I would think about something like:
>>>> -------------
>>>> public class TabbedPaneLayout implements LayoutManager {
>>>>
>>>> protected int basePreferredTabAreaWidth(final int
>>>> tabPlacement, final int height) {
>>>> // TabbedPaneLayout preferredTabAreaWidth implementation
>>>> }
>>>>
>>>> protected int truncatingPreferredTabAreaWidth(final int
>>>> tabPlacement, final int height) {
>>>> if (tabPlacement == SwingConstants.LEFT || tabPlacement
>>>> == SwingConstants.RIGHT) {
>>>> return basePreferredTabAreaWidth(tabPlacement, height);
>>>> }
>>>>
>>>> return basePreferredTabAreaWidth(tabPlacement, height);
>>>> }
>>>>
>>>> protected int preferredTabAreaWidth(final int tabPlacement,
>>>> final int height) {
>>>> return basePreferredTabAreaWidth(tabPlacement, height);
>>>> }
>>>>
>>>> }
>>>>
>>>> class TabbedPaneScrollLayout extends TabbedPaneLayout {
>>>> @Override
>>>> protected int basePreferredTabAreaWidth(int tabPlacement,
>>>> int height) {
>>>> // TabbedPaneScrollLayout preferredTabAreaWidth
>>>> implementation
>>>> }
>>>> }
>>>>
>>>> protected class AquaTruncatingTabbedPaneLayout extends
>>>> AquaTabbedPaneCopyFromBasicUI.TabbedPaneLayout {
>>>>
>>>> @Override
>>>> protected int preferredTabAreaWidth(final int tabPlacement,
>>>> final int height) {
>>>> return truncatingPreferredTabAreaWidth(tabPlacement,
>>>> height);
>>>> }
>>>> }
>>>>
>>>> protected class AquaTruncatingTabbedScrollPaneLayout extends
>>>> AquaTabbedPaneCopyFromBasicUI.TabbedPaneScrollLayout {
>>>>
>>>> @Override
>>>> protected int preferredTabAreaWidth(final int tabPlacement,
>>>> final int height) {
>>>> return truncatingPreferredTabAreaWidth(tabPlacement,
>>>> height);
>>>> }
>>>> }
>>>> -------------
>>>>
>>>> I just have one more question. The TabbedPaneScrollLayout is only
>>>> created in AquaTabbedPaneCopyFromBasicUI. AquaLookAndFeel only use
>>>> AquaTabbedPaneUI or AquaTabbedPaneContrastUI which do not return
>>>> TabbedPaneScrollLayout.
>>>>
>>>> Are there any real cases when the TabbedPaneScrollLayout is created?
>>>>
>>>> When you enabled the AquaTruncatingTabbedScrollPaneLayout in the
>>>> AquaTabbedPaneUI the JTabbedPane L&F with SCROLL_TAB_LAYOUT does
>>>> not look similar to Aqua L&F:
>>>> http://cr.openjdk.java.net/~alexsch/8137169/aqua-scrolled-tabbed-pane.png
>>>>
>>>> The AquaTruncatingTabbedPaneLayout already contains arrows for
>>>> hidden tabs navigation.
>>>> May be the fix should update the AquaTruncatingTabbedPaneLayout only?
>>>>
>>>> Thanks,
>>>> Alexandr.
>>>>
>>>> On 18/03/16 14:21, Avik Niyogi wrote:
>>>>> Hi Alexander,
>>>>> Thank you for the inputs. I agree with you and did feel the need
>>>>> for removing duplicate code as well. But as per an earlier review
>>>>> input, changes to the super call lay outing is not accepted. This
>>>>> was the only other feasible solution. Created redundant code in
>>>>> this process, but would be maintaining with requirements with code
>>>>> impact to superclasses.
>>>>> Please provide any insight to a probable compensate to mitigate
>>>>> this dichotomy of code expectation. Thank you in advance.
>>>>>
>>>>> With Regards,
>>>>> Avik Niyogi
>>>>>> On 18-Mar-2016, at 2:42 pm, Alexander Scherbatiy
>>>>>> <alexandr.scherbatiy at oracle.com> wrote:
>>>>>>
>>>>>>
>>>>>> It is not usually a good idea to have a duplicated code which
>>>>>> should be updated every time in several places.
>>>>>>
>>>>>> Is it possible to move the methods used both in
>>>>>> AquaTruncatingTabbedPaneLayout and
>>>>>> AquaTruncatingTabbedScrollPaneLayout to TabbedPaneLayout with
>>>>>> different names and then reused?
>>>>>>
>>>>>> Thanks,
>>>>>> Alexandr.
>>>>>>
>>>>>> On 17/03/16 17:17, Avik Niyogi wrote:
>>>>>>> Hi Alexander,
>>>>>>> The issue only applies for ScrollingTabbedPane and hence this fix.
>>>>>>>
>>>>>>> With Regards,
>>>>>>> Avik Niyogi
>>>>>>>
>>>>>>>> On 16-Mar-2016, at 4:51 pm, Alexander Scherbatiy
>>>>>>>> <alexandr.scherbatiy at oracle.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Does the same issue affects the AquaTabbedPaneContrastUI?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alexandr.
>>>>>>>>
>>>>>>>> On 14/03/16 09:04, Avik Niyogi wrote:
>>>>>>>>> Hi All,
>>>>>>>>> A gentle reminder, please review code changes.
>>>>>>>>>
>>>>>>>>> With Regards,
>>>>>>>>> Avik Niyogi
>>>>>>>>>> On 08-Mar-2016, at 9:51 pm, Avik Niyogi
>>>>>>>>>> <avik.niyogi at oracle.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi All,
>>>>>>>>>>
>>>>>>>>>> Please review code changes done as with inputs provided.
>>>>>>>>>> http://cr.openjdk.java.net/~aniyogi/8137169/webrev.01/
>>>>>>>>>>
>>>>>>>>>> Also, albeit the title of issue mentioned is as above, the
>>>>>>>>>> injection of issue occurs because
>>>>>>>>>> *pane.setTabLayoutPolicy(JTabbedPane.SCROLL_TAB_LAYOUT);* is
>>>>>>>>>> not honoured.
>>>>>>>>>> In the new fix as provided, references to base class layout
>>>>>>>>>> manager is removed in current solution.
>>>>>>>>>>
>>>>>>>>>> With Regards,
>>>>>>>>>> Avik Niyogi
>>>>>>>>>>
>>>>>>>>>>> On 02-Mar-2016, at 7:50 pm, Alexander Potochkin
>>>>>>>>>>> <alexander.potochkin at oracle.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hello Avik
>>>>>>>>>>>
>>>>>>>>>>> Let me make it clear I don't approve the proposed fix
>>>>>>>>>>> and ask you to do additional evaluation.
>>>>>>>>>>>
>>>>>>>>>>> Every LookAndFeel is different and it doesn't make much sense
>>>>>>>>>>> to compare Metal LaF with AquaLaf.
>>>>>>>>>>>
>>>>>>>>>>> The AquaLaf mimics the native MacOS controls and therefore
>>>>>>>>>>> look quite different from any other Lafs.
>>>>>>>>>>>
>>>>>>>>>>> The bug you are fixing has the following subject
>>>>>>>>>>> "Incorrect minimal heigh of JTabbedPane with more tabs"
>>>>>>>>>>>
>>>>>>>>>>> Could you please fix exactly the problem with the minimal
>>>>>>>>>>> heights,
>>>>>>>>>>> without changing the UI delegate class.
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> alexp
>>>>>>>>>>>
>>>>>>>>>>>> Gentle reminder. Please review this fix.
>>>>>>>>>>>>
>>>>>>>>>>>>> On 26-Feb-2016, at 10:39 am, Avik Niyogi
>>>>>>>>>>>>> <avik.niyogi at oracle.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> The issue is with setting of TabbedPane*Scroll*Layout()
>>>>>>>>>>>>> for the option JTabbedPane.SCROLL_TAB_LAYOUT as is enabled
>>>>>>>>>>>>> in the test code
>>>>>>>>>>>>> and *not* TabbedPaneLayout() as which is the default.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The minimum size fixes itself because the *ScrollLayout*
>>>>>>>>>>>>> check fails in *setTabLayoutPolicy*() for the pane. So the
>>>>>>>>>>>>> issue is with the call to set layout manager.
>>>>>>>>>>>>> There are only two configurations that the *JTabbedPane*
>>>>>>>>>>>>> can exist in of which *SCROLL_TAB_LAYOUT* is one of them.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Fixing the minimum size in *AquaTabbedPaneUI* will fix it
>>>>>>>>>>>>> for *TabbedPaneLayout*() only which is the *WRAP_TAB_LAYOUT*.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Also, I have checked other implementations such as for
>>>>>>>>>>>>> *Metal* and *Motif* and *they have similar code for doing
>>>>>>>>>>>>> this process*.
>>>>>>>>>>>>> Hence, with in-depth analysis, this fix has no other
>>>>>>>>>>>>> impact apart from this fix.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In case the impact caused by this change has caused some
>>>>>>>>>>>>> definitive regressions, please mention them so they can be
>>>>>>>>>>>>> addressed. Thank you.
>>>>>>>>>>>>>
>>>>>>>>>>>>> With Regards,
>>>>>>>>>>>>> Avik Niyogi
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 25-Feb-2016, at 6:45 pm, Alexander Potochkin
>>>>>>>>>>>>>> <alexander.potochkin at oracle.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hello Avik
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> AquaTruncatingTabbedPaneLayout has a lot of code which is
>>>>>>>>>>>>>> specific for the AquaTabbedPaneUI.
>>>>>>>>>>>>>> I don't think setting the layout manager from the base
>>>>>>>>>>>>>> class is the right solution here.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> If there is a problem with minimum size it should be
>>>>>>>>>>>>>> fixed inside the AquaTabbedPaneUI
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>> alexp
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 2/24/2016 12:07, Avik Niyogi wrote:
>>>>>>>>>>>>>>> Hi All,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Kindly review the bug fix for JDK 9.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *Bug:*
>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8137169
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *Webrev:*
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~aniyogi/8137169/webrev.00/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *Issue:*
>>>>>>>>>>>>>>> For Aqua Look&Feel, multiple calls
>>>>>>>>>>>>>>> to pane.getMinimumSize().height causes incremental
>>>>>>>>>>>>>>> return of values.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *Cause:*
>>>>>>>>>>>>>>> The impact was caused by a major broken code within
>>>>>>>>>>>>>>> AquaTabbedPaneUI.java for createLayoutManager()
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> *Fix:*
>>>>>>>>>>>>>>> Major linking calls to super class fix done within
>>>>>>>>>>>>>>> createLayoutManager().
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> With Regards,
>>>>>>>>>>>>>>> Avik Niyogi
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160323/56b0680e/attachment.html>
More information about the swing-dev
mailing list