Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #3522. Adds IDesignable: Ability for Views to load design-time/demo data #3575

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

tig
Copy link
Collaborator

@tig tig commented Jun 29, 2024

Fixes

Proposed Changes/Todos

  • Define interface
  • Fix Slider
  • Fix RadioGroup
  • Fix ListView
  • Fix MenuBar
  • ...
  • Add unit tests
  • Update docs

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@tig
Copy link
Collaborator Author

tig commented Jun 29, 2024

I am doing rebase wrong, clearly.

I had to re-create this PR by manually copying my changes from #3568 because it was screwed up somehow.

I have now done this 3-4 times and I have no clue what I'm doing wrong.

@dodexahedron
Copy link
Collaborator

I'll take a quick look at the current state of the branch and see if I can piece together what went wrong. No promises without the original commits still existing, though, which is pretty unlikely unless you went out of your way to keep doing everything on new branches with each push.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jun 29, 2024

Taking a look at the commit graph, my first suspicion is those merges.

Dollars to donuts I'd bet those are at least a significant part of the difficulty, because they move the common parent to a place where you're probably going to get strange things like merge conflicts that aren't actually any different between the two copies or duplicate commits, or other annoying/strange junk like that.

That gets worse and worse if other merge activity occurs to the branch you branched from (so v2_develop), that you're trying to rebase your work branch on, after you've also merged from v2_develop into your working branch. It makes a diamond dependency, sorta (it's more that it makes the v2_develop branch an ancestor of itself via your branch, which looks to the graph like jumping back in time, yet moving forward at the same time), and it just tries its best to replay the deltas for you. But especially if you're using the default git diff algorithm, that gets......weird.....

Setting the diff/merge algorithm to histogram sometimes can help a little bit, at least in making the diffs make a bit more sense, but the solution is to never merge outward.

Outward meaning from a branch closer to the root to a branch farther from root.

In general, once you branch, the only merges that branch should ever be involved in are merges into an ancestor with a common base that is no older than the original branch point, or as the recipient of merges from descendent branches of itself.

To "merge" outward (so, to apply changes from an ancestor branch to your working branch) you rebase, effectively changing the branch point, which replays the diffs on the new base, giving you the opportunity to get involved if there are commits it can't auto-apply.

When doing a rebase, an interactive rebase is a good opportunity to squash, drop, or otherwise modify commits of your working branch, since you're already re-writing history for your branch.

However, if the branch you're working on has descendents, do not force push a rebase over it, because you will orphan the other branch, which can be a bitch to fix sometimes because there's no longer a common base. Push to a new branch, instead, if there's a descendent branch.

You can actually always push to a new branch, after a rebase, if you want to preserve the old history for any reason. You would just have to update the PR or create a new one if you did that.

But yeah... Anything that pulls in commits from an ancestor before you merge back into that ancestor has the potential (not a guarantee) to create conflicts and other fun. You can often get away with it, especially if you didn't touch any of the same files, but the nature of merges means that, when it does bite you, it's usually going to bite harder than your average merge conflict.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jun 29, 2024

A thought I just had that I've never tried but sounds valid in my head is that, if you have done outward merges like that... If you do an interactive rebase (if it'll let you), you could try to drop the commits for the merges from the ancestor branch into your branch. It should work, even if your code is dependent on what was merged in, because that's all already there, already, at the new ref you're rebasing on.

Then, your branch will no longer make its ancestor its own grandpa. 🤷‍♂️

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a look at it beyond what we were exploring in the associated issue, around nitpicky stuff with generics.

First thing that comes to mind, just scanning the file list: Are those all the places you wanted to have this, for now (ie no other view types)?

Looking at the actual code now.

Copy link
Collaborator

@dodexahedron dodexahedron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

A couple of super minor opportunistic suggestions, but good to go as-is if you don't want to bother.

Terminal.Gui/View/IDesignable.cs Outdated Show resolved Hide resolved
Terminal.Gui/Views/ComboBox.cs Outdated Show resolved Hide resolved
Terminal.Gui/Views/ComboBox.cs Show resolved Hide resolved
Terminal.Gui/Views/Menu/MenuBar.cs Show resolved Hide resolved
UICatalog/Scenarios/AllViewsTester.cs Show resolved Hide resolved
@tig
Copy link
Collaborator Author

tig commented Jun 30, 2024

Taking a look at it beyond what we were exploring in the associated issue, around nitpicky stuff with generics.

You are confusing two issues.

First thing that comes to mind, just scanning the file list: Are those all the places you wanted to have this, for now (ie no other view types)?

Any view that could show meaningful state could support this. There's no pressing need to hit them all now.

I imagine when we get to

, for example, we'll want to add this to it to help development/testing.

@tig
Copy link
Collaborator Author

tig commented Jun 30, 2024

Taking a look at the commit graph, my first suspicion is those merges.

Back when I simply did git merge and wasn't trying to be a good boy and rebase, I never had these issues. This is def something caused by how I'm rebasing. And it's probably just something stupid.

Here is my typical workflow.

When I want to create a new PR to address an issue I do

git checkout v2_develop
git pull upstream v2_develop
# push to my fork if needed, so it matches
git push 
# create new branch
git checkout -b v2_xxx-branch

Then I work, and eventually do

git add .  ; git commit -m "commit msg"
git push

And I use github.com to create the remote v2_xxx-branch

Then, if, as I'm working, v2_develop changes, I want those changes in v2_xxx-branch, so I

git checkout v2_develop
git pull upstream v2_develop
git push

git checkout v2_xxx-branch

#double check I've pushed my latest and have the latest of my branch
git push
git pull origin v2_xxx-branch
git rebase --onto v2_develop

# pull branch down and re-apply changes
git pull

# fix any merge issues
git mergetool

# commit
git add . ; git commit -m "rebased with v2_develop"

This never feels right, so whatever I'm doing wrong is in the stuff around git rebase --onto v2_develop.

@BDisp
Copy link
Collaborator

BDisp commented Jun 30, 2024

Then, if, as I'm working, v2_develop changes, I want those changes in v2_xxx-branch, so I

git checkout v2_develop
git pull upstream v2_develop
git push

git checkout v2_xxx-branch

#double check I've pushed my latest and have the latest of my branch
git push
git pull origin v2_xxx-branch
git rebase --onto v2_develop

# pull branch down and re-apply changes
git pull

# fix any merge issues
git mergetool

# commit
git add . ; git commit -m "rebased with v2_develop"

I think the problem is in the git pull origin v2_xxx-branch if you have already some local commit not yet pushed to the origin. So I recommend that you push all your local commits to the origin or don't pull from the origin at all. But if you end up with a force push avoid it because it would may delete any changes that you may want to preserve. I'll recommended using merge instead of rebase because when a PR is merged into the v2_develop all do smoothly. I confess that I do all that work using the VS2022 UI and only on situations more complicated I use the command line.

@BDisp
Copy link
Collaborator

BDisp commented Jun 30, 2024

Sorry, ignore the local commits not yet pushed because you already do a git push before the git pull origin v2_xxx-branch. Maybe the git pull origin v2_xxx-branch isn't really needed because the origin is already updated.

@BDisp
Copy link
Collaborator

BDisp commented Jun 30, 2024

Maybe the culprit is the git pull after the git rebase --onto v2_develop. During this you probably need to resolve any conflicts and only after they are fixed you do git add . ; git commit -m "rebased with v2_develop" and then push it. If you are pulling without the rebase is finished, probably you are corrupting your origin.

@dodexahedron
Copy link
Collaborator

Taking a look at it beyond what we were exploring in the associated issue, around nitpicky stuff with generics.

You are confusing two issues.

I suspected I might be.

First thing that comes to mind, just scanning the file list: Are those all the places you wanted to have this, for now (ie no other view types)?

Any view that could show meaningful state could support this. There's no pressing need to hit them all now.

I imagine when we get to

, for example, we'll want to add this to it to help development/testing.

Sounds like a good plan to me. 👍

@dodexahedron
Copy link
Collaborator

Taking a look at the commit graph, my first suspicion is those merges.

Back when I simply did git merge and wasn't trying to be a good boy and rebase, I never had these issues. This is def something caused by how I'm rebasing. And it's probably just something stupid.

Here is my typical workflow.

When I want to create a new PR to address an issue I do

git checkout v2_develop
git pull upstream v2_develop
# push to my fork if needed, so it matches
git push 
# create new branch
git checkout -b v2_xxx-branch

Then I work, and eventually do

git add .  ; git commit -m "commit msg"
git push

And I use github.com to create the remote v2_xxx-branch

Then, if, as I'm working, v2_develop changes, I want those changes in v2_xxx-branch, so I

git checkout v2_develop
git pull upstream v2_develop
git push

git checkout v2_xxx-branch

#double check I've pushed my latest and have the latest of my branch
git push
git pull origin v2_xxx-branch
git rebase --onto v2_develop

# pull branch down and re-apply changes
git pull

# fix any merge issues
git mergetool

# commit
git add . ; git commit -m "rebased with v2_develop"

This never feels right, so whatever I'm doing wrong is in the stuff around git rebase --onto v2_develop.

Here's what I do when working on a github repo that I have a fork of, that helps avoid unexpected weirdness:

  • First, sync my fork's v2_develop, on github.
  • Fetch locally to get the updated index.
  • Branch from head of v2_develop.
  • Do my work.
  • If at any point I want to rebase on v2_develop again:
    • Sync my fork on github again and fetch to get the index again
    • Do the rebase
  • Continue working and repeat the above step whenever. Usually also once more before submitting a PR, to make it no-conflict auto-mergeable
  • Submit the PR for dodexahedron/Terminal.Gui/branchI'mworkingOn to gui-cs/Terminal.Gui/v2_develop

@dodexahedron
Copy link
Collaborator

If I drop or squash anything during the rebases above, I sometimes will push a new branch, to preserve the old one, just in case, and if I remember to do so.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jul 1, 2024

This step:

git pull origin v2_xxx-branch

Has the potential to cause a merge that could cause unexpected annoyances, since the default is to merge from your remote into your local, if they differ.

You can change that behavior default to rebase (what I have set as the default behavior) or to fast-forward only, if you want, or you can specify it at pull-time with --rebase=[base] or --ff-only. Both will avoid a merge. --ff-only will fail if fast-forward isn't possible, which is usually a good thing in this use case, since it means history diverged.

However, a git checkout already does a pull. It's a shortcut for a git switch and a git pull. git pull itself is already just a shortcut for git fetch and git merge, by default, which is where the auto-merge into local comes from.

If I do it at the command line, I usually use git switch rather than git checkout just to be careful that nothing I didn't explicitly intend happens. That merge that happens on a pull/checkout replays the remote on top of your working tree, which is probably not what you actually want, especially if you have dropped/squashed/modified any commits that were common between them, before.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jul 1, 2024

Then, if, as I'm working, v2_develop changes, I want those changes in v2_xxx-branch, so I

git checkout v2_develop
git pull upstream v2_develop
git push

git checkout v2_xxx-branch

#double check I've pushed my latest and have the latest of my branch
git push
git pull origin v2_xxx-branch
git rebase --onto v2_develop

# pull branch down and re-apply changes
git pull

# fix any merge issues
git mergetool

# commit
git add . ; git commit -m "rebased with v2_develop"

I think the problem is in the git pull origin v2_xxx-branch if you have already some local commit not yet pushed to the origin. So I recommend that you push all your local commits to the origin or don't pull from the origin at all. But if you end up with a force push avoid it because it would may delete any changes that you may want to preserve. I'll recommended using merge instead of rebase because when a PR is merged into the v2_develop all do smoothly. I confess that I do all that work using the VS2022 UI and only on situations more complicated I use the command line.

That step stood out to me, too, because of the implicit merge that happens if history differs.

Check out the steps I posted a couple comments up from here for what I do that avoids any of these issues while using the recommended rebase/branch outward, merge inward flow.

I most often use GitKraken to interact with it, but it respects your git configuration and also can show you exactly what it did to achieve an operation, which is part of how I originally learned pieces of how it all works, a while back. Pretty sure that's still available with a free license, and it's available on all platforms. I use it in windows and linux, and it's just...NICE.

@tig tig merged commit 716ac31 into gui-cs:v2_develop Jul 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants