Pivot
  1. Pivot
  2. PIVOT-75

SplitPane's splitLocation(int) property should be splitRatio(float)

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1, 1.2
    • Fix Version/s: 1.3
    • Component/s: wtk
    • Labels:
      None

      Description

      For the occasion when SplitPanes are embedded inside each other, it would be nice if I could set the inner SplitPane's to relative-resize-mode i.e. when it's size is reduced or expanded, it should adjust the splitter location to maintain the relative sizes of the top/bottom components.

      To solve this, note that currently, the splitLocation property of SplitPane specifies the absolute pixel value of the splitter within the split pane. This would be much more useful to applications (and much less error-prone) if it were specified as a ratio of one side of the split to the other (from 0.0f to 1.0f). This would have the following benefits:

      1) The caller can initialize the splitter location very easily, whereas previously, you had to presuppose how the split pane would be laid out to give a meaningul splitLocation property.

      2) When the split pane is resized, granting proper allocations to each side of the splitter becomes very easy, since the ratio stays the same.

      1. PIVOT-75-Fix.patch
        12 kB
        Niclas Hedhman
      2. PIVOT-75-Fix2.patch
        16 kB
        Niclas Hedhman
      3. split_ratio.patch
        3 kB
        Greg Brown

        Activity

        Hide
        Todd Volkert added a comment - - edited

        Actually, when Pivot started, we had support for a resize weight a-la Swing, but we just couldn't come up with a real justification for it, so we took it out – maybe we overlooked something. Can you elaborate on your example of nested split panes? Specifically what type of split pane content would demand such relative sizing?

        Show
        Todd Volkert added a comment - - edited Actually, when Pivot started, we had support for a resize weight a-la Swing, but we just couldn't come up with a real justification for it, so we took it out – maybe we overlooked something. Can you elaborate on your example of nested split panes? Specifically what type of split pane content would demand such relative sizing?
        Hide
        Noel Grandin added a comment -

        If I have 3 split panes next to each other like this:

        x | x | x

        And I drag the first divider towards the right, the last pane in the series loses all of its space.
        Makes for a very jaring experience.

        Instead of having a primary region, maybe have a resize mode - TOP_LEFT, BOTTOM_RIGHT, EQUAL ??

        where EQUAL allocates space equally to both entities when the SplitPane is resized.

        That would accomplish what I need.

        Show
        Noel Grandin added a comment - If I have 3 split panes next to each other like this: x | x | x And I drag the first divider towards the right, the last pane in the series loses all of its space. Makes for a very jaring experience. Instead of having a primary region, maybe have a resize mode - TOP_LEFT, BOTTOM_RIGHT, EQUAL ?? where EQUAL allocates space equally to both entities when the SplitPane is resized. That would accomplish what I need.
        Hide
        Todd Volkert added a comment -

        Yeah, we could do something like that.

        Show
        Todd Volkert added a comment - Yeah, we could do something like that.
        Hide
        Todd Volkert added a comment -

        As part of this change, we should change the splitLocation property of SplitPane to be a float value that represents a percentage of the width/height as opposed to an absolute value. Then when the user drags the splitter, the skin will update the percentage as opposed to updating the absolute value.

        Show
        Todd Volkert added a comment - As part of this change, we should change the splitLocation property of SplitPane to be a float value that represents a percentage of the width/height as opposed to an absolute value. Then when the user drags the splitter, the skin will update the percentage as opposed to updating the absolute value.
        Hide
        Greg Brown added a comment -

        I'm increasing the priority of this change to major, since it is an API change and we'd like to get as many API changes as we can into 1.3. Also, I think this will be a significant improvement.

        Show
        Greg Brown added a comment - I'm increasing the priority of this change to major, since it is an API change and we'd like to get as many API changes as we can into 1.3. Also, I think this will be a significant improvement.
        Hide
        Greg Brown added a comment -

        We should also change the "splitBounds" property to "splitLimits" (of type Limits) as part of this change. The Limits class will be added as part of the fix for PIVOT-207.

        Show
        Greg Brown added a comment - We should also change the "splitBounds" property to "splitLimits" (of type Limits) as part of this change. The Limits class will be added as part of the fix for PIVOT-207 .
        Hide
        Niclas Hedhman added a comment -

        Question; What are the rounding off rules involved here??

        Example; Splitter thickness = 1, width = 4, splitRatio = 0.5. Should the splitter be at second or third pixel?

        Show
        Niclas Hedhman added a comment - Question; What are the rounding off rules involved here?? Example; Splitter thickness = 1, width = 4, splitRatio = 0.5. Should the splitter be at second or third pixel?
        Hide
        Niclas Hedhman added a comment -

        I have changed the splitLocation to splitRatio, to indicate that it is 0-1 float we are looking for.

        Show
        Niclas Hedhman added a comment - I have changed the splitLocation to splitRatio, to indicate that it is 0-1 float we are looking for.
        Hide
        Niclas Hedhman added a comment -

        Here is the patch to fulfill what I know of this issue.

        Show
        Niclas Hedhman added a comment - Here is the patch to fulfill what I know of this issue.
        Hide
        Greg Brown added a comment -

        Further property naming refinements: "splitLimits" should simply be "limits" (e.g. getLimits():Limits), and we should also define "max" and "min" properties that delegate to the main setter method.

        Show
        Greg Brown added a comment - Further property naming refinements: "splitLimits" should simply be "limits" (e.g. getLimits():Limits), and we should also define "max" and "min" properties that delegate to the main setter method.
        Hide
        Greg Brown added a comment -

        Rather than defining split bounds or limits, SplitPane could simply respect the preferred size limits reported by the primary component.

        Show
        Greg Brown added a comment - Rather than defining split bounds or limits, SplitPane could simply respect the preferred size limits reported by the primary component.
        Hide
        Todd Volkert added a comment -

        The patch looks pretty good. Comments are as follows:

        1) I'd change local variable splitLocationRatio to be just splitRatio

        2) I'd change the SplitPaneListener interface to have the method be called splitRatioChanged

        3) I'd code the skin to treat splitLimits as limiting the absolute value of the splitter (vs. limiting the split ratio). While splitRatio makes more sense than splitLocation, I think the limits of the splitter make more sense as real pixel values.

        4) I like Greg's idea to bag splitLimits and make SplitPane just respect the min/max preferred width/height of its two components. If you want to make this change in the patch as well, you can, or if you want to maintain split limits in the patch and tackle one change per SVN commit, that makes sense too.

        5) With respect to the rounding question, as long as the skin's consistent in how it rounds (floor, ceil, round), then I think any of those are fine. Just make sure to not go under 0 or over the max.

        Show
        Todd Volkert added a comment - The patch looks pretty good. Comments are as follows: 1) I'd change local variable splitLocationRatio to be just splitRatio 2) I'd change the SplitPaneListener interface to have the method be called splitRatioChanged 3) I'd code the skin to treat splitLimits as limiting the absolute value of the splitter (vs. limiting the split ratio). While splitRatio makes more sense than splitLocation, I think the limits of the splitter make more sense as real pixel values. 4) I like Greg's idea to bag splitLimits and make SplitPane just respect the min/max preferred width/height of its two components. If you want to make this change in the patch as well, you can, or if you want to maintain split limits in the patch and tackle one change per SVN commit, that makes sense too. 5) With respect to the rounding question, as long as the skin's consistent in how it rounds (floor, ceil, round), then I think any of those are fine. Just make sure to not go under 0 or over the max.
        Hide
        Greg Brown added a comment -

        Re: Todd's comment #3 - I think this is actually a bug in the patch. The code appears to treat the split limits as a percentage, but they are specified by the caller as absolute pixel values. In fact, they are ints, so they can't currently represent many interesting percentage values.

        Show
        Greg Brown added a comment - Re: Todd's comment #3 - I think this is actually a bug in the patch. The code appears to treat the split limits as a percentage, but they are specified by the caller as absolute pixel values. In fact, they are ints, so they can't currently represent many interesting percentage values.
        Hide
        Niclas Hedhman added a comment -

        Thanks for the good feedback.

        Todd's list;

        1. I will change the name to splitRatio, as this was what felt good to me too, but I worried that it was "too much" for your taste.

        2. Listener methdo --> Didn't think of it, will fix.

        3/4. SplitLimits --> I also like Greg's idea, and will try to go with that. If I can't figure it out, I'll open a new issue.

        5. Ok.

        Show
        Niclas Hedhman added a comment - Thanks for the good feedback. Todd's list; 1. I will change the name to splitRatio, as this was what felt good to me too, but I worried that it was "too much" for your taste. 2. Listener methdo --> Didn't think of it, will fix. 3/4. SplitLimits --> I also like Greg's idea, and will try to go with that. If I can't figure it out, I'll open a new issue. 5. Ok.
        Hide
        Niclas Hedhman added a comment -

        Regarding MinPreferredxxx;

        Right now it looks like the min/max preferred width/height are values set by client code. And I think there is not much resolution handling when they conflict with the same attributes of the two child components.

        Shouldn't the splitPane's attributes just be derived from the children instead?

        Show
        Niclas Hedhman added a comment - Regarding MinPreferredxxx; Right now it looks like the min/max preferred width/height are values set by client code. And I think there is not much resolution handling when they conflict with the same attributes of the two child components. Shouldn't the splitPane's attributes just be derived from the children instead?
        Hide
        Niclas Hedhman added a comment -

        This patch "/Users/niclas/Desktop/PIVOT-75-Fix2.patch" contains;

        o Removed the splitLimits, and instead use the minPreferredWidth/Height (if set) of the children as the limits.

        o Added that the min/max preferred width/height is derived from the children and setting them not allowed (which Exception should be used).

        o layout() also has limits check, since it was possible to 'somehow' drag it outside the range.

        Show
        Niclas Hedhman added a comment - This patch "/Users/niclas/Desktop/ PIVOT-75 -Fix2.patch" contains; o Removed the splitLimits, and instead use the minPreferredWidth/Height (if set) of the children as the limits. o Added that the min/max preferred width/height is derived from the children and setting them not allowed (which Exception should be used). o layout() also has limits check, since it was possible to 'somehow' drag it outside the range.
        Hide
        Greg Brown added a comment -

        This looks good to me. I'd say go ahead and submit it, though Todd may have some additional comments since he was the original author.

        Minor coding style comments:

        • Don't include spaces between a cast operator and the operand
        • Don't include a space after opening or before closing parentheses
        • Don't include CRs before opening curly braces
        • Include a CR after @Override and other annotations
        • Include a CR in @param tags after the variable name
        • Some additional CRs in TerraSplitPaneSkin#limitSplitLocation() might help improve readability a bit
        • Variable name in SplitPane.SplitPaneListenerList#splitRatioChanged() is still previousSplitLocation - should be previousSplitRatio
        • Does this TODO still apply? I think it is safe to remove:

        // TODO: the size overrides are awaiting consensus from community.

        We really need to write up a coding standards doc. I do have an Eclipse formatting profile that we can post to the site, though.

        Also, you'll need to update references to "splitLocation" in WTKX files. I'll attach a patch you can apply before you check in.

        Show
        Greg Brown added a comment - This looks good to me. I'd say go ahead and submit it, though Todd may have some additional comments since he was the original author. Minor coding style comments: Don't include spaces between a cast operator and the operand Don't include a space after opening or before closing parentheses Don't include CRs before opening curly braces Include a CR after @Override and other annotations Include a CR in @param tags after the variable name Some additional CRs in TerraSplitPaneSkin#limitSplitLocation() might help improve readability a bit Variable name in SplitPane.SplitPaneListenerList#splitRatioChanged() is still previousSplitLocation - should be previousSplitRatio Does this TODO still apply? I think it is safe to remove: // TODO: the size overrides are awaiting consensus from community. We really need to write up a coding standards doc. I do have an Eclipse formatting profile that we can post to the site, though. Also, you'll need to update references to "splitLocation" in WTKX files. I'll attach a patch you can apply before you check in.
        Hide
        Todd Volkert added a comment -

        I just reviewed this as well. It looks really good - I only noticed a few things:

        1) SplitPane.setSplitRatio() still uses previousSplitLocation - should be previousSplitRatio

        2) I'd say you can remove the overrides of the getters and setters for min/max preferred size. SplitPaneSkin always reports a preferred size of 0,0 - it's meant to only be used in cases where it's either not asked for its preferred size of it's given an explicit preferred size. In either case, the min/max preferred sizes aren't really in play.

        3) In SplitPaneSkin.SplitterSkin.mouseMove(), the split ratio must be calculated after limiting the split location.

        These and Greg's points are simple enough to fix that I'd say no need to re-post a patch for review. How does this work if you have karma to commit but aren't a committer?

        Show
        Todd Volkert added a comment - I just reviewed this as well. It looks really good - I only noticed a few things: 1) SplitPane.setSplitRatio() still uses previousSplitLocation - should be previousSplitRatio 2) I'd say you can remove the overrides of the getters and setters for min/max preferred size. SplitPaneSkin always reports a preferred size of 0,0 - it's meant to only be used in cases where it's either not asked for its preferred size of it's given an explicit preferred size. In either case, the min/max preferred sizes aren't really in play. 3) In SplitPaneSkin.SplitterSkin.mouseMove(), the split ratio must be calculated after limiting the split location. These and Greg's points are simple enough to fix that I'd say no need to re-post a patch for review. How does this work if you have karma to commit but aren't a committer?
        Hide
        Niclas Hedhman added a comment -

        Regarding codestyle; I find your style confusing and not natural, so it is hard to 'comply'. On one hand you seek additional whitespace (which I am a strong proponent of) for instance when you ask for the additional CRs, but then you ask for 'compressed' Sun-like style elsewhere with very little whitespace. Hard to keep those apart, especially since your 'basis' is really far from mine... Do you have a Checkstyle configuration set up, so I could configure in IDEA? (Has Eclipse formatter been fixed yet? It used to make more mess than it solved...)

        Show
        Niclas Hedhman added a comment - Regarding codestyle; I find your style confusing and not natural, so it is hard to 'comply'. On one hand you seek additional whitespace (which I am a strong proponent of) for instance when you ask for the additional CRs, but then you ask for 'compressed' Sun-like style elsewhere with very little whitespace. Hard to keep those apart, especially since your 'basis' is really far from mine... Do you have a Checkstyle configuration set up, so I could configure in IDEA? (Has Eclipse formatter been fixed yet? It used to make more mess than it solved...)
        Hide
        Greg Brown added a comment -

        I applied Niclas's patch and modified it based on the comments above.

        Show
        Greg Brown added a comment - I applied Niclas's patch and modified it based on the comments above.
        Hide
        Todd Volkert added a comment -

        Linking to PIVOT-236

        Show
        Todd Volkert added a comment - Linking to PIVOT-236
        Hide
        Todd Volkert added a comment -

        Updating description to be more correct in the 1.3 release notes

        Show
        Todd Volkert added a comment - Updating description to be more correct in the 1.3 release notes

          People

          • Assignee:
            Niclas Hedhman
            Reporter:
            Noel Grandin
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development