TestTerminalPosition, TestTerminalRectangle & TestTerminalSize and changes to those classes

[ginkoblongata]

Core changed classes:

// the three immutable classes
TerminalSize
TerminalPosition
TerminalRectangle
// tests for them
TestTerminalSize
TestTerminalPosition
TestTerminalRectangle

// the rest of the files changed are due to these, and their changes
// are all of a similar nature to each other
// of these changes, while I've put careful thought into each, I think
// possibly the static fields like OF_0x0 etc could maybe
// just be of0x0() of1x1() methods, mostly to make reading and writing easier

Primary change:

• added TestTerminalSize
• added TestTerminalPosition

    // TestTerminalPosition and TestTerminalSize tests:
    public void test_instantiation() {...
    public void testSystemIdentityHashCodeSame() {...
    public void testSystemIdentityHashCodeDifferent() {...
    public void testObjectChurnReduced_of() {...
    public void testObjectChurnReduced_as() {...
    public void testObjectChurnReduced_plus() {...
    public void testObjectChurnReduced_minus() {...
    public void testObjectChurnReduced_divide() {...
    public void testObjectChurnReduced_multiply() {...
    public void test_withColumn() {...
    public void test_withRow() {...
    public void test_withRelativeColumn() {...
    public void test_withRelativeRow() {...
    public void test_withRelative() {...
    public void testObjectChurnReduced_with() {...
    public void test_plus() {...
    public void test_minus() {...
    public void test_multiply() {...
    public void test_divide() {...
    public void test_abs() {...  // only TerminalPosition
    public void test_min() {...
    public void test_max() {...
    public void test_compareTo() {...
    public void test_hashCode() {...
    public void test_equals() {...

• added TestTerminalRectangle

public class TestTerminalRectangle {
    public void test_instantiation() {...
    public void testSystemIdentityHashCodeSame() {...
    public void testSystemIdentityHashCodeDifferent() {...
    public void testObjectChurnReduced_of() {...
    public void testObjectChurnReduced_as() {...
    public void test_withX() {...
    public void test_withY() {...
    public void test_withWidth() {...
    public void test_withHeight() {...
    public void test_withPosition() {...
    public void test_withSize() {...
    public void test_whenContains() {...
    public void test_hashCode() {...
    public void test_equals() {...

• removed public static fields from TerminalRectangle, seems methods like x() y() width() height() are almost as succinct as fields of those names, but have some benefits like smaller memory use and potentially clears the path and opens up options with interfaces and default methods

• added x(), y(), width() and height() methods to TerminalPosition, TerminalSize and TerminalRectangle, this is because they are they are succinct

• using static TerminalSize.of() TerminalPosition.of() rather than constructor, this allows for further memory optimizations like reusing TerminalPosition(0,0) TerminalSize(80,40) etc.

public static final TerminalSize of(int columns, int rows) {...

• using instance method as() used to route the rest of the methods through for returning same instance when possible, this is what the existing with() method uses although the one taking in an Object already is likely less beneficial since the parameter object would also already be instantiated....

    public TerminalSize as(int columns, int rows) {...
    public TerminalSize as(TerminalSize size) {...

• made TerminalSize & TerminalPosition consistent with having some helper methods

public class TerminalSize implements Comparable<TerminalSize> {
    public TerminalSize plus(TerminalSize other) {...
    public TerminalSize minus(TerminalSize other) {...
    public TerminalSize multiply(TerminalSize other) {...
    public TerminalSize divide(TerminalSize denominator) {...

    public TerminalSize plus(int columns, int rows) {...
    public TerminalSize minus(int columns, int rows) {...
    public TerminalSize multiply(int columns, int rows) {...
    public TerminalSize divide(int colDenom, int rowDenom) {...
    
    public TerminalSize plus(int amount) {...
    public TerminalSize minus(int amount) {...
    public TerminalSize multiply(int amount) {...
    public TerminalSize divide(int denominator) {...

public class TerminalPosition implements Comparable<TerminalPosition> {
    public TerminalPosition plus(TerminalPosition other) {...
    public TerminalPosition minus(TerminalPosition other) {...
    public TerminalPosition multiply(TerminalPosition other) {...
    public TerminalPosition divide(TerminalPosition denominator) {...

    public TerminalPosition plus(int columns, int rows) {...
    public TerminalPosition minus(int columns, int rows) {...
    public TerminalPosition multiply(int columns, int rows) {...
    public TerminalPosition divide(int colDenom, int rowDenom) {...
    
    public TerminalPosition plus(int amount) {...
    public TerminalPosition minus(int amount) {...
    public TerminalPosition multiply(int amount) {...
    public TerminalPosition divide(int denominator) {...

Reason for the different parameters on the methods is to provide ease of use.
The methods like withRelativeRows(), withRelativeColumns() are left in, but they are synonymous with plus() or minus().

Also, they now all have the logic which can return the same instance if the resulting TerminalSize or TerminalPosition were to have the same columns & rows.

Added convenience equals(int, int) method:

public class TerminalSize implements Comparable<TerminalPosition> {
     public boolean equals(int columns, int rows) {...

public class TerminalPosition implements Comparable<TerminalPosition> {
     public boolean equals(int column, int row) {...

• These hard coded static fields are not strictly necessary because the static of() method returns them anyway, but potentially it provides clearer intent in the code:

   // changed from this:
   TerminalPosition contentOffset = TerminalPosition.TOP_LEFT_CORNER;
    // to this:
   TerminalPosition contentOffset = TerminalPosition.OF_0x0;

    // changed from this:
    window.setComponent(new EmptySpace(TerminalSize.ONE));
    // to this:
    window.setComponent(new EmptySpace(TerminalSize.OF_1x1));

Possibly:

• could leave TOP_LEFT_CORNER in the code as it would just get the same instance from of() method.
• could remove OF_0x0 etc altogether and let user just call of() method
• could go for changing OF_0x0, OF_1x1, OF_1x0 to .of0x0() of1x1() etc to make easier to type (no odd shift keytorsions)

[ginkoblongata]

Let me know of anything I need to address for this merge request.
I think it's pretty good, with the size though I've anticipated a combination of further explanation or changes to do for this merge request would happen.
After this merge request then I've got a fix for SplitPanel in the pipeline, and then I'm ready to finally tackle generalized ScrollPanel capability.

[avl42]

I think, this PR is too much at once.

Try to split it up into separate topics that can be individually checked and judged.

Removing methods is not to be taken lightly (unless it were about private methods.)
getDimension() may be useful, if you write code that ought to work in either horizonal
or vertical direction - LinearLayout would first come to my mind for this.

PS: I do not have any say here - I'm only just a "contributor".
I only write this feedback, because 1.) that was my own thought after seeing your PR,
and 2.) Martin hasn't answered on his own, yet.

Edit: I removed a comment about multiply/divide when I saw these methods already existed before and that only an overload was added. That's fine (imho).

[avl42]

One more comment about the changes in TerminalSize:

1.) making the fields public is a "no go" - it contradicts "immutability".

2.) I like the static method "of", but its current implementation does 4 comparisons in the non-trivial cases, and up to 6 comparisons in the semi-trivial cases (with only one component in {0,1}).
I think, it might be better to change it to a nested if-structure roughly like that:

if (rows==0) {
    if(columns==0) { return ...} else if (columns==1) { return ...}
else if (rows==1) {
    if(columns==0) { return ...} else if (columns==1) { return ...}
}

actually, I think that constants for 0x0 and 1x1 would be enough... The use cases for 1x0 and 0x1 are pretty low imho, and having fewer branches in "of" is probably a better investment than 1,0 and 0,1 instances...

In that sense, my suggestion for the initial part of "of()" would be:

if (rows==columns) {
    if (rows==0) { return OF_0x0; } else if (rows==1) { return OF_1x1; }
}

[mabe02]

I'm overall ok with this typo of change (on master, since it breaks backward compatibility); just not sure what prompted you to start refactoring these?

Could you maybe squash the commits as well?