Pos/Dim issues (v2)

[dodexahedron]

Big post incoming...

I had a much better one all typed up, but then my browser crashed and lost my text, so I have to start over. That's what I get for not writing it up in notepad++...

Here are some of the biggest points I can recall, "distilled" (a little bit, anyway) to a bulleted list.
The equality and GetHashCode issues are the most important things.\

• The Dim and Pos types have inconsistent equality mechanisms. Some are value equality and some are reference equality. Most (almost all) also first require type equality.
• What kind of equality will happen is opaque to the user, so they have no idea what's going to happen if they try to compare one Pos to another Pos or one Dim to another Dim.
• The nested types are opaque, so the reason for inequality when two dimensions or positions are otherwise identical, from the user's perspective, is very unclear.
• It is possible for a Pos or Dim that actually represent the same position or dimension, in real screen coordinates, to be not equal, even if their Equals method is value equality. This is usually because of the type check which, again, is opaque to the user and thus not clear, potentially only presenting itself at run-time.
• operator== isn't overridden in most of them, so it is just a call to System.ReferenceEquals where it isn't, even if the Equals method has been overridden to give value equality or even a slightly different means of determining reference equality.
• Pos.PosView and Dim.PosDim define GetHashCode() as just proxying to the target view's GetHashCode function. View does not override GetHashCode, nor do any types that inherit from View do so. So, that falls back to object.GetHashCode, which is unreliable, according to the .net spec.
  • object.GetHashCode(), on Windows specifically, is based on the reference itself, in the current .net runtime. That means it is theoretically not a stable value, and is thus unfit for use in anything that would ever care to call that, such as HashSets or as keys in a Dictionary.
    • It really needs to be overridden and defined by something immutable for each Pos and Dim type.
• There is an implicit cast operator defined for int to PosAbsolute, but there is no reverse of the same, either implicit or explicit.
• Pos.SetPosCombine does not do anything other than call SetNeedsDisplay when the Pos is a PosView, and it is only called in 2 places.
  • As-is, that function doesn't even touch its second parameter, and the whole function can be reduced to this statement: (left as PosView)?.Target.SetNeedsLayout (); where left is just the same name as the parameter that is used, but would simply be the reference in the caller.
    • That produces better IL too, doing in 7 more direct operations what the old code does in 16, so it's not just syntactic sugar. In-lining it removes at least 3 more opcodes per invocation, as well, including avoiding a new stack frame for the method call.
  • That function is called only by Pos.operator+ and Pos.operator- and doesn't actually do anything at all with the PosCombine it is given.
  • That function should be in-lined, examined for potential removal, or implemented as actually intended.

Here are some xUnit-style tests that show the inconsistent equality behavior:

[Fact]
public void PosAbsolute_EqualityOperator( )
{
	Pos p1 = 1;
	Pos p2 = 1;
	Assert.True ( p1 == p2 ); // This will fail. Reference equality.
}
[Fact]
public void PosAbsolute_EqualsMethod( )
{
	Pos p1 = 1;
	Pos p2 = 1;
	Assert.True ( p1.Equals ( p2 ) ); // This will pass. Equals is overridden to be type and value equality.
}

The same issues or similar, as described above, hold true for most of the Pos and Dim types.

I have a bunch more related to Pos and Dim, but I'll post a separate issue since they're related, but not directly.

[dodexahedron]

In terms of dotnet design guidelines and formality/completeness of types, in relation to casts/type conversions:

• If a lossy cast exists on a type, it should be explicit unless the "loss" is trivial and round-tripping the cast to the other type and back would yield a value-equal instance, for types where value is important, whether the type itself is a class or struct.
• If a cast exists in one direction, the inverse of it should be provided if possible and practical.
• If those casts are implicit, each one must only be defined on one class or the other or compile errors are almost guaranteed.
• Explicit casts can be more liberally applied, when and where desired, because they require user intent to invoke.
• For portability and CLS-compliance reasons, any casts defined in C# should also be provided as ToX and/or FromX static methods on the same types, using the same definition (typically, one calls the other). Not all languages understand type casts.
• When there are implicit casts on a type, IEquatable<T> is strongly recommended to be implemented on both types, where T is the type of the type that can be cast to or from. Explicit casts can also benefit from such implementations, but that's less important.
• On that, Pos, Dim, and their various sub-types could greatly benefit from just implementing IEquatable<T> for each T where T is an inheritor/descendant of the same base type or primitive they can be reasonably equated to. For example, PosAbsolute could benefit from implementing IEquatable or, even better, IComparable.
• PosAbsolute can be cast FROM int, but not TO int. In fact, the means of getting the int value back out of a PosAbsolute isn't intuitively obvious - it's the Anchor property, but that's internal, so non-friend assemblies can only get at that via reflection.
  • This one would be pretty easy to implement and pretty helpful, too.
  • As noted above, corresponding static and instance methods for ToInt32 and FromInt32 should probably also be provided, with the casts just delegating to those for consistency.
• Implicit casts should never throw exceptions if at all avoidable, and should do their best to make their error cases discoverable through static analysis.
• Explicit casts can throw, but should still do their best to avoid doing so excessively, while also doing their best to own their own exceptions, rather than relying on other types to throw and bubbling those up.
• Those exceptions can pretty much all be thrown in/by the ToX/FromX methods those casts delegate to, in the same type.

[tig]

Can you please combine these two issues? #3108

[dodexahedron]

If you prefer, that's fine with me.

I only separated them as this one was meant more as a list of things that I think are important issues that deserve being addressed sooner, whereas the other one is more of a list of improvements and ideas that, while closely related, I was just trying to keep separate to avoid crossing wires, since there's a lot in each one.

I kinda think all of this stuff is more appropriate as a "project" that can be broken up into smaller units of work, but however you prefer to organize it is perfectly cool. :)

[tig]

I actually didn't read it all. I thought the fact there were two was a result of you losing your work.

[dodexahedron]

Haha nah.

Just being verbose.

I'd rather brain dump everything as it comes to me than forget about it. 😅

I figure the worst case is someone says "man, that idea sucks and it threatened to kick my puppy." Otherwise, at least it's all there to discuss and act on (or not) as things move along. :)

[tig]

When I turn my focus back to Dim.Auto I'll read all this for sure!

[dodexahedron]

No rush. I'll be better able to discuss it when I'm done with current work, as well.

[tig]

• Pos.SetPosCombine does not do anything other than call SetNeedsDisplay when the Pos is a PosView, and it is only called in 2 places.

Fixed in #3410

[tig]

Same: Now that #3480 is merged and #3415 is almost ready, it would be awesome if this Issue were updated with a clear list of the remaining issues you have related to all this.

[dodexahedron]

Wait a second. Didn't I comment on this one yesterday? Or was that another issue very similar to this one?

Or the other option: Did I not actually post what I thought I did? 🤔

Gonna take a look.

[dodexahedron]

Ah yes... #3108

There's significant overlap.

I'm going to copy all the text from both into a local file so I can go through and de-dupe, nuke items that are obsolete, done, or otherwise irrelevant, etc., and then use that as a source for individual, much smaller, more focused, and more concrete issues.

[dodexahedron]

In any case, the interface thing is a better immediate piece of work for me to tackle, and will enable other work on those types to be a lot easier than, for example, if they were changed to structs and all incidental changes required by that then had to be done, but with nothing to tie them together. With appropriate interfaces in place and in use, that sort of change becomes a lot simpler, requiring fewer changes to method bodies, instead of just signatures.

Plus @tznind can then get busy with less risk of wasted work.

[tznind]

I have started to move from reflection to direct references.

For now I am keeping TGD's internal model of Pos\Dim (enum) for simplicity

• so only changing the PosExtensions and DimExtensions classes.

For example TGD represents PosCombine as 'offset'. And does things like unpacking nested PosCombine and eliminating redundant stuff like '+ 0'.

First goal is to get tests passing with existing UI and options then look to updates and new types.

On Sat, 25 May 2024, 06:28 dodexahedron, @.***> wrote:

In any case, the interface thing is a better immediate piece of work for me to tackle, and will enable other work on those types to be a lot easier than, for example, if they were changed to structs and all incidental changes required by that then had to be done, but with nothing to tie them together. With appropriate interfaces in place and in use, that sort of change becomes a lot simpler, requiring fewer changes to method bodies, instead of just signatures.

Plus @tznind https://github.com/tznind can then get busy with less risk of wasted work.

— Reply to this email directly, view it on GitHub https://github.com/gui-cs/Terminal.Gui/issues/3107#issuecomment-2130785656, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHO3C5FIUPCJZYOJHX6TQ4TZEAORTAVCNFSM6AAAAABBIMFES2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZQG44DKNRVGY . You are receiving this because you were mentioned.Message ID: @.***>