The fix looks good to me.
Thanks,
Alexandr.
On 24/03/16 13:31, Rajeev Chamyal wrote:
>
> Looks good to me.
>
> Regards,
>
> Rajeev Chamyal
>
> *From:*Avik Niyogi
> *Sent:* 24 March 2016 12:54
> *To:* Rajeev Chamyal; Alexander Scherbatiy; Sergey Bylokhov;
> 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 code changes as per inputs received.
>
> http://cr.openjdk.java.net/~aniyogi/8137169/webrev.03
> <http://cr.openjdk.java.net/%7Eaniyogi/8137169/webrev.03>
>
> 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 24-Mar-2016, at 12:34 pm, Rajeev Chamyal
> <rajeev.chamyal at oracle.com <mailto:rajeev.chamyal at oracle.com>> wrote:
>
> 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
> <mailto: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/
> <http://cr.openjdk.java.net/%7Eaniyogi/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
> <alexandr.scherbatiy at oracle.com
> <mailto: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
> <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
> <http://cr.openjdk.java.net/%7Ealexsch/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
> <mailto: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 <mailto: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 <mailto: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/
> <http://cr.openjdk.java.net/%7Eaniyogi/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
> <mailto: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
> <mailto: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
> <mailto: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/
> <http://cr.openjdk.java.net/%7Eaniyogi/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/20160329/9389b969/attachment.html>