<Swing Dev> Review Request of 8137169 : [macosx] Incorrect minimal heigh of JTabbedPane with more tabs

Thu Mar 24 07:04:47 UTC 2016

Hello Avik,

x variable on line 2195 is not used anywhere. Do we need for loop also?

Regards,

Rajeev Chamyal

From: Avik Niyogi 
Sent: 24 March 2016 12:19
To: Alexander Scherbatiy
Cc: Sergey Bylokhov; Rajeev Chamyal; swing-dev at openjdk.java.net
Subject: Re: <Swing Dev> Review Request of 8137169 : [macosx] Incorrect minimal heigh of JTabbedPane with more tabs

Hi All,

Please review my code changes below as per the inputs received.

http://cr.openjdk.java.net/~aniyogi/8137169/webrev.02/

As SCROLL_TAB_LAYOUT is the default layout for Aqua LAF, with implementation within the derived AquaTruncatedTabbedPaneLayout, extending of TabbedPaneScrollLayout is not needed and management of row and column height is done within it itself. Hence preferredTabAreaWidth and preferredTabAreaHeight need not manage the number of columns and rows respectively and will remain 1 as tabs can be brought forth to the UI by the arrow buttons AQUA provides instead of placing them in another row or column. This is also the expected behaviour for AquaLAF as per javadoc.

With Regards,

Avik Niyogi

On 23-Mar-2016, at 4:14 pm, Alexander Scherbatiy <HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"alexandr.scherbatiy at oracle.com> wrote:

On 23/03/16 14:07, Avik Niyogi wrote:

On 23-Mar-2016, at 3:31 pm, Alexander Scherbatiy <HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"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.

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 <HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"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:
  HYPERLINK "http://cr.openjdk.java.net/%7Ealexsch/8137169/aqua-scrolled-tabbed-pane.png"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 <HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"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 <HYPERLINK "mailto:alexandr.scherbatiy at oracle.com"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 <HYPERLINK "mailto:avik.niyogi at oracle.com"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 <HYPERLINK "mailto:alexander.potochkin at oracle.com"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 <HYPERLINK "mailto:avik.niyogi at oracle.com"avik.niyogi at oracle.com> wrote:

The issue is with setting of TabbedPaneScrollLayout() 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 <HYPERLINK "mailto:alexander.potochkin at oracle.com"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/20160324/880f4c83/attachment.html>