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

Keybindings persistence #172

Merged
merged 2 commits into from
Jul 6, 2022
Merged

Conversation

MrApplejuice
Copy link
Contributor

Requires #169 and #159 to be merged first.

s.mp4

Changes:

  • I refactored PR primitive config with custom shortcuts #159 of @HuntClauss a bit. Simplified it and gave it some "forwards compatibility" that might otherwise be annoying to user (see code comment).
  • Added store_shortcuts() function to Settings which is called to persist keybindings

lorien/Misc/Settings.gd Outdated Show resolved Hide resolved
@MrApplejuice MrApplejuice force-pushed the keybindings-persistence branch from 0173006 to 3a72717 Compare July 1, 2022 21:36
@mbrlabs
Copy link
Owner

mbrlabs commented Jul 3, 2022

I squashed your commits from #169 because it makes the tree cleaner. Not sure how to resolve this right now...i never had to merge a PR that depended on on annother PR that got squashed 😅. Can you rebase to the current main branch? I also need to merge #159 first. Worst case you have to create a new PR for this.

@MrApplejuice
Copy link
Contributor Author

Hey! Ah yes, squashing. I remember this problem from work where we have the age-old discussion about "linear commit history" vs "safety while working with code". I had to work this one out completely when we migrated from SVN to GIT.

Not sure if you are that well-versed with git, but the short version is: (please unfold if interested)

... Not sure if you are that well-versed with git, but the short version is: Git really is not designed with linear commit histories in mind. It was built to implement workflows from kernel.org where patches fly left right and center and 3-5 different branches seem to get merged all the time and reintegrated into mainline. Since that workflow is all but linear, git does not implement linear workflows - and for a good reason: By encoding such complex merge trees it can keep track of changes in multiple branches in parallel and offer three-point merge features and other neat stuff without loosing any work, like we did in Subversion when someone would execute a faulty conflicting merge.

Rebase and squash are "non-git" workflow additions that allow for linear workflows and - well squash gives a way to make more expressive, compressed histories. Both come with the cost of loosing work and with the cost of disrupting the complex merge tree by default. After a squash and after a rebase, branches will not directly integrate with each other anymore even if they originally might have a common ancestor in the commit-tree, because that ancestor might get deleted during a merge.

At work, we decided to disable rebase&sqaush for one of our repos because of regular problems with this that were causing us to loose work... Anyway...

Because of the squash, this branch lost its ancestor that linked it to the main-tree. Now there are a lot of conflicts because the three-point-merge algorithm fails to resolve the points where I resolved the conflicts before.

I can fix that, I know how to do that, but it would be the easiest for me if I can do this once #159 is actually merged. Alternatively, we could decide now that we merge #159 through merging this PR. If you give me green-light for that solution, I will fix the commit tree doing a difficult rebase and then this is ready for merging. #159 would not be merged in that case, because it would be covered with the PR.

@mbrlabs
Copy link
Owner

mbrlabs commented Jul 4, 2022

Thanks for the explanation, very insightful. I got the squashing/rebase workflow from the Godot repo...normally i don't do this either, but it has it's advantages (cleaner tree, easier to revert changes etc).

I can merge #159 first if it's easier. I assume squasing that PR would make things even more complicated, so should i just rebase and merge the 5 commits? Or a traditional merge commit?

@MrApplejuice
Copy link
Contributor Author

I can merge #159 first if it's easier. I assume squasing that PR would make things even more complicated, so should i just rebase and merge the 5 commits? Or a traditional merge commit?

Your choice! One squash happened already, so I need to construct new patches anyway. Most likely, I will opt to rewrite the patches of this PR from scratch, which actually is quite easy because this PR mainly builds on top of the keybindings-PR (that one that is now squashed and merged) plus the config-file PR.

So does not matter anymore :-)

cleaner tree, easier to revert changes etc

Yeah, I know. From a management perspective it all becomes quite easy to follow when rebasing. And typically all works quite well as long as there is not too much parallel development like we had with the keybindings now. I will leave this open to you how you want to manage this project. The other way to manage this with rebasing is to negotiate with the contributors to a certain work-pattern that leads to compatible patches... 🤷 no idea what the better choice is wrt this 😆

@mbrlabs
Copy link
Owner

mbrlabs commented Jul 4, 2022

Alright i merged #159 ;)

no idea what the better choice is wrt this

Yeah i have to think about that. For smaller self-contained PRs squashing should be fine. For bigger changes across multiple PRs like this one the traditional workflow might be better. We'll see

@MrApplejuice MrApplejuice force-pushed the keybindings-persistence branch from 3a72717 to ab6ce2a Compare July 5, 2022 08:58
@MrApplejuice MrApplejuice marked this pull request as ready for review July 5, 2022 08:59
@MrApplejuice
Copy link
Contributor Author

MrApplejuice commented Jul 5, 2022

FYI: There only were two changesets that had to be rebased, so I did that manually using git cherry-pick. Ready-for-review otherwise.

One problem though: The code comments got unlinked. Please check then in this thread and match them up with the appropriate locations in the actual code-base :-/

Copy link
Owner

@mbrlabs mbrlabs left a comment

Choose a reason for hiding this comment

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

Great work as always, thank you!
You deserve a special mention in the next release notes ;)

@mbrlabs mbrlabs merged commit e65b641 into mbrlabs:main Jul 6, 2022
@mbrlabs mbrlabs mentioned this pull request Jul 6, 2022
@MrApplejuice MrApplejuice deleted the keybindings-persistence branch July 8, 2022 07:19
@MrApplejuice
Copy link
Contributor Author

MrApplejuice commented Jul 8, 2022

Thanks :-) Glad to help. I was planning on contributing some extra additions though, if you do not mind! :-)

@bennyschirm
Copy link
Contributor

@MrApplejuice awesome, thank you so much. This is exactly what i wanted 🥇

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