Get rid of custom primitives such as Rect and use base class library types

[dodexahedron]

This is related to work I'm doing in my miscellaneous cleanup change batches, in preparation for some of the more important work I have outstanding, but is significant enough that I thought it would be a good idea to throw up a formal issue for it.

The Rect struct is unnecessary and is technical debt we don't need to carry (as are others, but I'm starting with Rect).

This change is "breaking," but only insofar as any instance of Rect in external code simply needs to be changed to Rectangle or a type alias[^1] introduced if one doesn't want to update Rect to Rectangle.

It's almost identical to the base class library System.Drawing.Rectangle type, except for two minor things:

• Its ToString() override is slightly different, outputting (shown here in C# string interpolation format) ({X},{Y},{Width},{Height}) instead of {{X={X},Y={Y},Width={Width},Height={Height}}
• A couple of convenience methods that can simply be extension methods on System.Drawing.Rectangle, such as IntersectsWithInclusive, which is, as its name suggests, just the IntersectsWith method, but inclusive of its edges (which is not technically intersection, but I'm not dealing with that naming right now).
• An implementation of GetHashCode that is quite different from how it's defined in the BCL (which we have no reason/need for).
• It's missing additional functionality that has been added since this type was originally copied to Terminal.Gui a long time ago.

The change set that I will associate with this issue does not remove Rect. Instead, it renames it and makes its ToString method behave the same as the built-in Rectangle type, and the test modifications to go along with that (the associated tests get a minor upgrade with it, as well).

The reason I didn't remove it all at once is because the other types in the folder with it also need the same treatment, but I'm trying to do this in a way that is easy to follow. Thus, My intent is to make each change in a similar way as I'm doing this one - make the type compatible with the built-in type first, before removal.

The way it will look, in git, for each of these types, is two stages (per type):

• First branch:
  • Rename the type globally in the solution (only relevant for Rect, because the others use the same names as their built-in cousins).
  • Bring behavior in line with the corresponding built-in type (ToString etc).
  • Fix tests broken by different output.
  • Improvements to affected tests, as appropriate.
• Following branch, from the end of the above branch:
  • Remove the type.
  • If any namespace usings are needed, stick them in the project file as global usings, unless they're only in one or two files, in which case just do them in code in those files.
    • Do these as explicit type aliases, so they only affect the specific type being worked on (for example, <Using Include="System.Drawing.Rectangle" Alias="Rectangle" />).
    • Once all associated changes have been made to all types, these can then be removed, as they're just a temporary shim to avoid needless code changes.
  • If any custom methods are missing, copy them over as extension methods, so consuming code doesn't have to change.
  • If any internal code expects return types that are also defined internally (such as Size), define implicit casts on those custom types so that no other code in the library has to change to accommodate the transition.
  • Update any tests affected by the change and ensure validity.
    • Opportunistic enhancements to tests may be made at this point.
  • Remove the unit tests that are explicitly for the removed type, as they're no longer our responsibility.

[^1]: As easy as using Rect = System.Drawing.Rectangle; in code or <Using Include="System.Drawing.Rectangle" Alias="Rect" /> in the csproj or any other inherited MSBuild file.

[dodexahedron]

3255 lays the groundwork for all of these and is intended to be a simple PR to avoid unnecessary complication. That one is ready to go.

The work for this issue carries on as a branch from the last commit of that PR and is in https://github.com/dodexahedron/Terminal.Gui/tree/v2_cleanup_work_2, which is nearing completion of the "First Branch" work from the list above, for Rect -> Rectangle. A PR will be issued for that when it's ready.

[dodexahedron]

Another small difference with our Rect struct vs System.Drawing.Rectangle: We throw exceptions in the constructor and in property setters if a dimension is less than 0.

I rather strongly dislike that for several reasons that I've written up in the associated work item on my fork, to save noise over here, since it's verbose (I know - shocker, right? 😅).

TL;DR of that: Regardless of other reasons to get rid of Rect and other similar types, this particular issue, all by itself, has a pile of objective technical reasons to nuke them entirely.

I'm done with the removal on my branch (not pushed yet), and have everything behaving properly. However, I'm going to do an additional pass over a few important places to do some triage to see if it looks like any critical program flow is likely to change because of it.

[tig]

Thank you for taking this on. I've disliked that we had copies of these forever.

[dodexahedron]

Ok. First one was merged a couple hours ago. Now I'll rebase or merge or whatever I have to do to the subsequent work to prep that for a PR and will submit as soon as it's ready.

Next one actually removes the Rectangle type and adds operators to our Size and Point types to make them compatible without having to change stuff all over the place.

After that one is some general cleanup.

After that will be prep for removal of Point or, if it's as simple as I'm hoping it might be, actually removing it and maybe even Size all at once (and all their float variants, too).

Off to the C# machine!

[dodexahedron]

Pretty sure the main difficulty is the merge option being used.

It looks like you're doing squash merges, yeah?

That loses the commit history and makes it non-linear because it's actually creating a completely new commit. It also loses the innate tracking of contributors, which might suck for random people who submit something and might like their contribution history to be visible on their github profiles. 🤷‍♂️ It also loses accountability and authenticity. All my commits are signed by my private key, which is a guarantee that those commits came from me and that they have not been tampered with, by anyone, at any time, forever. That gets nuked in a squash merge.

Without commit history, though, nobody can trace bugs or regressions back to when they actually happened, which is a pretty big drawback. The only history that exists is the merge commit and it isn't traceable back to the original branch on the source fork, even if the submitter hasn't deleted that branch already anyway.

In short, merges across forks being done in a non-linear fashion breaks the main purpose of source control.

For example, the parent commit of the merge that was done for the preparation to remove Rect is https://github.com/gui-cs/Terminal.Gui/commit/16055c53b05418b929845810889a5ac31201a983, which does not reflect what actually happened and provides no means of backtracking through the changes, especially once I delete the source branch. A normal merge would have both that commit and the final commit on the source branch as parents, preserving the tree and history.

This actually has bitten us/me before on this repo, for some previous work I did. However, I still had a local copy of the original branch involved, so I was able to re-push that branch back to my fork and cherry-pick the commits I needed to bring forward that otherwise would have been clobbered.

You still get the same number of commits (one) per merge, on v2_develop (or whatever target branch), but history is not lost/re-written.

And then re-bases also would not duplicate commits, like they like to do when it's non-linear, and would be better at conflict resolution, since they wouldn't actually be conflicts, in the first place, if it knew the history.

[dodexahedron]

Now that I think about it, this is actually the majority of the source of all these conflicts we keep getting that seem like they should be trivial to resolve. From the perspective of git, two identical changes or very similar changes were made independently, in two or more points in the graph whose common parent may be hundreds of non-relevant commits earlier.

If actual history had been maintained, git would know what the source of a change was and would be able to resolve most of these situations automatically.

But take the branch I have to remove rect...

If I re-base it on v2_develop after the squash merge, the rebase has to go through everything that happened between the point I branched it and now, on v2_develop, which are different commits than the ones that actually made some of those changes on my very own branches that still exist, even though the resulting file contents may be identical. It has no way to know that that's what happened, so its only course of action is to replay it all and consider each case of same-line changes to be conflicts and ask you what to do about it. That's also why a few of my PRs have had commits in their history going back to work that wasn't even part of my branch - a rebase was done at some point in my work, and it had to do exactly that.

For that same branch, if I re-base it on my updated and reverse-merged/resolved branch that it was originally branched from, git freaks out and claims there's no rebase in progress. And that makes some sense, since both are identical, even though they're different commits. It actually doesn't allow the rebase to be completed. And a merge from this branch to that one creates a diamond inheritance in the graph that also results in the same merge conflicts PLUS all the changes I made being considered conflicts, which is obviously not right.

And the more of these squash merges are done, the problem compounds, especially for work that goes on while other squash merges are performed and then is attempted to be merged at a later point.

[dodexahedron]

Yep. You're about to see it when I submit the PR for the next step.

There are only 6 commits in the branch, but the PR has a LOT more than that, because it has to go all the way back to the last common ancestor it can find.

[tig]

Thanks for explaining all this. Totally my bad for not fully understanding the impact of squash!

[BDisp]

This explains why it's always a nightmare to rebase a branch from another branch. They always required confirmation of each commit from the beginning, as if it were a new commit. For this reason I stopped using it and started just using merge. But even then he lost track of some commits.

[dodexahedron]

Yeah I honestly wish they didn't include it without a rather loud warning in the online interface. Both the rebase and squash options for "merge" of a PR are non-linear and will cause issues.

Oh well. Live and learn. :)

[dodexahedron]

With the submission of #3269 and #3270, this work is now finished. :)

[tig]

Woot. Amazing work @dodexahedron!!!!

[dodexahedron]

Happy to help. :)

[dodexahedron]

Also, I saw you did regular merges for recent PRs. Awesome. And it already made life easier. My next chunk of work was branched from the end of my last PR. Rebasing it on v2_develop worked beautifully for all 53 commits in my next branch. Woot!

Also, this is always pretty when a bunch of merges are done all at once 😅 :