feathersui / feathersui-starling

User interface components for Starling Framework and Adobe AIR
https://feathersui.com/learn/as3-starling/
Other
915 stars 386 forks source link

TiledRowsLayout: fix vertical JUSTIFY not behaving as expected #1731

Closed kevinfoley closed 6 years ago

kevinfoley commented 6 years ago

The vertical align mode JUSTIFY does not behave as expected for TiledRowsLayout when we explicitly set the number of rows and columns. Items in the layout are not expanded to fill the vertical space.

Code to reproduce the issue:

this.layout = new AnchorLayout();
var lg:LayoutGroup = new LayoutGroup();
addChild(lg);
lg.layoutData = new AnchorLayoutData(0, 0, 0 , 0);

var tlr:TiledRowsLayout = new TiledRowsLayout();
tlr.verticalAlign = VerticalAlign.JUSTIFY;
tlr.tileVerticalAlign = VerticalAlign.JUSTIFY;
tlr.requestedColumnCount = 2;
tlr.requestedRowCount = 2;
tlr.padding = 0;    
lg.layout = tlr;

for (var i:int = 0; i < 4; i++ ) {
var button:Button = new Button();
button.label = "Button";
    lg.addChild(button);
}

This issue occurs because TiledRowsLayout does not correctly calculate the desired tile height in its layout() function. This pull request adds new code that correctly calculates the desired tile height when the JUSTIFY vertical alignment is used.

joshtynjala commented 6 years ago

Thanks for the pull request. Unfortunately, you've missed in the documentation that VerticalAlign.JUSTIFY is not one of the supported values accepted by the verticalAlign property. It's not that it's behaving incorrectly. It's actually an unsupported value.

The distributeHeights property is meant to have all tiles fill the available vertical space. I think that's what you're looking for.

kevinfoley commented 6 years ago

@joshtynjala These few extra lines of code should make it possible to support JUSTIFY. If you don't want to support JUSTIFY, then the layout should probably throw an error when JUSTIFY is passed in.

joshtynjala commented 6 years ago

Layout changes usually require modifications to multiple methods, so those extra lines are probably not enough, unfortunately.

In an ideal world, invalid values would result in thrown errors, but that would require literally hundreds of these checks to be added to Feathers, since AS3 doesn't support enums that the compiler can catch. It'll hurt performance too much.

I'll add more details to the documentation to say that that JUSTIFY is not supported and suggest distributeHeights instead.

kevinfoley commented 6 years ago

@joshtynjala You can emulate the behavior of enums in other languages using special-purpose classes, like this: https://github.com/kevinfoley/air-file-browser/blob/master/src/kevinfoley/airfilebrowser/SortingMode.as It keeps a few extra objects in memory, but they're small objects. Since strings are primitives, this approach can actually use less memory than using strings for enums. I haven't evaluated CPU performance vs the string comparison method but I'd be surprised if it was worse.

joshtynjala commented 6 years ago

Switching away from string values would break setting these properties in MXML when using the Feathers SDK. Not to mention that I've seen some developers share code where they actually pass in string literals, instead of using the constants, for some reason.

kevinfoley commented 6 years ago

@joshtynjala distributeHeights doesn't seem to work either in Feathers 3.4.1:

    this.layout = new AnchorLayout();
    var lg:LayoutGroup = new LayoutGroup();
    addChild(lg);
    lg.layoutData = new AnchorLayoutData(0, 0, 0 , 0);

    var tlr:TiledRowsLayout = new TiledRowsLayout();
    tlr.requestedColumnCount = 2;
    tlr.requestedRowCount = 2;
    tlr.distributeHeights = true;

    tlr.padding = 0;

    lg.layout = tlr;

    for (var i:int = 0; i < 4; i++ ) {
            var button:Button = new Button();
            button.label = "Button";
            lg.addChild(button);
    }

The buttons do not expand to fill the vertical space

joshtynjala commented 6 years ago

As mentioned in the distributeHeights documentation, it requires that useSquareTiles is set to false.

Additionally, tileVerticalAlign needs to be set to VerticalAlign.JUSTIFY to have the item inside a tile fill the entire height of the tile.

var tlr:TiledRowsLayout = new TiledRowsLayout();
tlr.requestedColumnCount = 2;
tlr.requestedRowCount = 2;
tlr.distributeHeights = true;
tlr.useSquareTiles = false;
tlr.tileVerticalAlign = VerticalAlign.JUSTIFY;

If you want to fill the available horizontal space too, you need to use distributeWidths and tileHorizontalAlign for a similar effect.

joshtynjala commented 6 years ago

I think I'm going to make setting distributeWidths or distributeHeights to true automatically change useSquareTiles to false.

kevinfoley commented 6 years ago

Thanks Josh. My IDE truncates the docs for distributeHeights so I missed that note:

flashdevelop truncated documentation

distributeHeights works correctly when following your example.