Open ginkoblongata opened 3 months ago
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.
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).
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; }
}
Core changed classes:
Primary change:
added TestTerminalSize
added TestTerminalPosition
added TestTerminalRectangle
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.
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....
made TerminalSize & TerminalPosition consistent with having some helper methods
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:
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:
Possibly: