mbrlabs / Lorien

Infinite canvas drawing/whiteboarding app for Windows, Linux and macOS. Made with Godot.
MIT License
5.55k stars 241 forks source link

Keybindings persistence #172

Closed MrApplejuice closed 2 years ago

MrApplejuice commented 2 years ago

Requires #169 and #159 to be merged first.

https://user-images.githubusercontent.com/542105/176624429-7285d72c-5e66-4af7-9298-e34102bfcd68.mp4

Changes:

mbrlabs commented 2 years ago

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 :sweat_smile:. 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I can merge https://github.com/mbrlabs/Lorien/pull/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... :shrug: no idea what the better choice is wrt this :laughing:

mbrlabs commented 2 years ago

Alright i merged https://github.com/mbrlabs/Lorien/pull/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 commented 2 years ago

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 :-/

MrApplejuice commented 2 years ago

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

bennyschirm commented 2 years ago

@MrApplejuice awesome, thank you so much. This is exactly what i wanted :1st_place_medal: