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

Draft: Edit improvements to context menu and code organization #4811

Merged
merged 7 commits into from Jun 4, 2021

Conversation

carlopav
Copy link
Contributor

@carlopav carlopav commented May 22, 2021

Reference to post by @matthijskooijman: https://forum.freecadweb.org/viewtopic.php?f=23&t=58643&start=10#p505619

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, we ask you to conform to the following items. Pull requests which don't satisfy all the items below might be rejected. If you are in doubt with any of the items below, don't hesitate to ask for help in the FreeCAD forum!

  • Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
  • In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
  • Your branch is rebased on latest master git pull --rebase upstream master
  • All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
  • All commit messages are well-written ex: Fixes typo in Draft Move command text
  • Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
  • Commit messages include issue #<id> or fixes #<id> where <id> is the FreeCAD bug tracker issue number in case a particular commit solves or is related to an existing issue on the tracker. Ex: Draft: fix typos - fixes #0004805

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.20 Changelog Forum Thread.


@carlopav
Copy link
Contributor Author

@matthijskooijman thanks for the code example :)
how does it looks like to you?

@carlopav carlopav changed the title Draft: edit improvements to context menu and code organization Draft: Edit improvements to context menu and code organization May 22, 2021
Copy link
Contributor

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up! I really like where this is going.

Overall, the code looks great as-is. I left a bunch of inline comments on where I think things could be made a bit better.

I also left some comments about splitting this PR into more smaller commits, which would greatly help the reviewability of it. One additional commit that you could maybe split off is a commit where the contents of addPoints is spread around the various GuiTools base classes, but without all the menu generalization yet. Instead, as an interim approach, you could just have evaluate_menu_action call obj_gui_tools.add_point() instead of self.addPoint(). By itself, that's not very pretty, but as a temporary situations to be improved in a following commit, it would be fine. Combined with the other commit-splitting suggestions I made, I think that the main "generalize actions" commit would then be significantly smaller, making it a lot easier to see what's going on.

src/Mod/Draft/draftguitools/gui_edit.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit_base_object.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit_draft_objects.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit_draft_objects.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit_draft_objects.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit_draft_objects.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit_draft_objects.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit_base_object.py Outdated Show resolved Hide resolved
@matthijskooijman
Copy link
Contributor

I agree that separated commits would have been better, but can I keep the suggestion for the next contribution? :-)

Well, ultimately, that's not up to me, but I would think that splitting/restructuring the commits to improve reviewability would be helpful to reduce load on maintainers and make it easier to merge this. Especially now that you're adding commits that amend to original thing, it becomes more difficult to review this commit-by-commit, leaving the only option of reviewing the complete PR as one, which I would not be happy about as a maintainer.

If you're intimidated by this restructuring because git can be a challenge to work with (I'm quite proficient with it and love its power, but it's certainly not easy if you're less experienced) but you a're open to investing the extra time needed to improve the commit structure, I'll be happy to share some tips on how I'd approach this using some git commands to reduce the amount of work needed.

ok, let's do it (i feel a bit guilty cause you are loosing a lot of time). I'm using GitKraken because the command line interface is quite scaring to me as a Windows user...

Ok, here goes. I can't really comment on gitkraken (I always use the commandline since there it is, to me, clearest what happens under the hood and you can always use all features, rather than the subset exposed by the GUI), but I'll try to also explain the concept so you can find the same thing in gitkraken.

First off: I think it would be good to first further update the code to look like you want it to look (e.g. generalizing the getObjectInfo stuff), since you'd otherwise need to make those changes twice later.

Intended commit history
Then, to plan what we want the commit history to look at eventually, I think something like below. Note that I'm just talking about what the resulting commits should look like, not yet about how to get there from the current history. So the starting point of the below list is the master commit you started from.

  1. Remove all the empty pass functions
  2. Remove the event parameter from startEditing (you added this commit last, but since evaluate_context_menu_action already has an obj and node_idx parameter, I think you can do this change earlier, preventing the intermediate breakage by removing references to edit_command.event before removing the variable itself).
  3. Remove the App.DraftWorkingPlane.restore() line (I do not really understand this change, but you mentioned it should be its own commit?)
  4. Introduce a getSpecificObjectInfo function and update the original addPoint function to use it (or whatever other approach you choose to extract the object-type-agnostic code from addPoint so it does not have to be duplicated).
  5. Split out addPoint to multiple add_point methods in the GuiTools subclasses (and replacing the call to addPoint in evaluate_menu_action with a call to obj_gui_tools.add_point).
  6. Rewriting evaluate_menu_action and display_tracker_menu to use the new callback structure, and rewrite all GuiTools subclasses to also adapt (you could split this into two commits for clarity, but then the first commit would break things, which is usually not a good idea, so I'd keep this as one commit, unless you want to add the backward compatibility wrapper).
  7. Add the reverse wire command

Basic approach
Given that most of these changes are pretty independent, the basic workflow I would choose here is to:

  • Reset you branch back to the original master commit you started working from. The default "mixed" flavor of the git reset command resets your branch and the index/staging area but not the working copy, so this would put you in a position where you would have been if you had done all the coding in your checkout, but not committed any of it yet. On the commandline, this would be git reset origin/master (assuming that you origin/master still points to the commit you branched off from, if not you might need to rebase first, or replace origin/master with the SHA of the branch-off point).
  • Now, you can use interactive staging to select a subset of all your changes and create a new commit for that. You can do this for each commit to be created in turn. On the commandline, you would use git add -p for this, but I suspect that in kraken you can do a similar thing in e.g. a diff view where you select lines of the diff to be staged. On the commandline you can even edit the to-be-staged diff in a text editor (useful in case you need to commit one change in a line but not another, or in adjacent lines), but if Kraken doesn't support this, and you'd need it, you can always just revert one of the changes in the file manually, commit that, ^Z in your editor and commit the other change.

Note that especially the git add -p / interactive staging is IMHO a very powerful tool, which allows me to have a much improved workflow where I can (in a lot of cases) just focus on coding and making things work first, and then still be able to split the changes in proper commits at a later stage (sometimes at the end, somethings I already commit things halfway when I'm confident that a particular change is somewhat complete and will be included in the final history).

Additional work for commit 4 and 5
This approach will not work for all commits above. In particular, number 4 and 5 need to make changes to code in gui_edit.py that is already removed in the current PR (so with the above approach, that code will be gone from your working copy already). For this, I would suggest first making a backup copy of gui_edit.py and then use git checkout to throw away all uncommitted changes from gui_edit.py (or alternatively, only throw away a selection of changes, git checkout has an interactive -p option too). After this, you should again have the original addPoint and evaluate_menu_action code, so you can make changes to those and commit that (this is the part where you'll need to do some manual code editing, and will be writing/copying back some code that will be thrown away again in the next commit, but that's ok). After you created these two commits, you can restore your gui_edit.py backup again, and start interactively committing the rest of the commits.

Verifying commits work
Note that you might want to verify in between that your separated changes actually work. One easy way for that is to first commit them (even when you're not entirely sure that you got everything, you can always amend the commit later) and then use git stash to stash away all uncommitted changes, so you can test the code as you committed it. Then unstash and continue with the next commit.

Interactive rebase and fixup commits
One additional thing I would want to point out, in case you haven't used it before, is the interactive rebase in git. Though rebase is intended to be used to "rewind" a number of commits you made and "replay" them onto another starting point (e.g. a more recent version of the master branch), you can also rebase on top of an earlier commit in your existing history (instead of some newer master version), so you end up rewinding to that earlier commit, and then simply replaying the same commits on top of the same earlier base commit again (i.e. nothing changes). But if you then use rebase in interactive mode, you can select which commits should be replayed in which order. This allows you to easily reorder your commits or remove some of them.

Even more, you can tell git to squash some commits together in the process, which means it becomes very easy to fix mistakes later: If you already committed something, but spot a mistake in it, you just make a new commit with just the fix, and then use an interactive rebase to move that "fixup" commit backwards in history to be right after the original commit, and then ask git to squash them together into one commit, as if the mistake never happened.

Git even automates a part of this process using git commit --fixup, which marks such "fixup" commits in such a a way that the commit message indicates what previous commit they are fixing, so an interactive rebase (with --autosquash) can automatically reorder and squash them. I use this workflow often where I put fixes for earlier mistakes into such fixup commits, but do not do the interactive rebase right away, but wait until some later point (so I can rebase/squash a few of them at the same time to save time, or so I can push out the fixup commits to a PR so reviewers that already saw the mistake can see the fixup more clearly before I make both disappear again). Since git remembers for me which fixup is meant to fix which mistake, I can easily postpone the actual rebasing/squashing since I do not have to remember :-) Of course if multiple commits touch the same code and you reverse their order (i.e. a fixup commit that fixes a mistake in code that was modified again after the mistake commit), such a rebase is likely to cause conflicts, so be prepared to fix some conflicts along the way (but knowing how to fix conflicts is useful in any case, of course).

Force pushing
One word of caution, though: All this rebase/fixup (and also the reset + recommit stuff) rewrites history, which is typically not appropriate once commits have been merged to master, or some other branch that is intended to have some stability. If you rewrite history on a branch that you already pushed somewhere, you'll have to force push to overwrite the previous history, which means that pulling from such a branch might cause problems (since as far as git is concerned, the original and rewritten commit histories look like separately developed commits which happen to touch all the same code, leading to conflicts). Force pushing is not acceptable for master usually (though this is a matter of convention, git doesn't care), but for pull request branches, this is usually ok (people pulling from PR branches should pull carefully, though again, project-specific convention might differ).

Commit messages
One additional advantage of splitting code into different commits, is that you can use each commit's message to more specifically motivate the changes in that commit. In this particular case, some of the changes may seem meaningless when the commit is viewed in isolation (i.e. commit 4 would split off some code into its own function which is then called only once), but makes sense in the whole. What I usually do for such commits is explicitly note this in the commit message (e.g. something like "This commit prepares for splitting out the addPoint code into multiple versions in a future commit, reducing the amount of code to duplicate when that happens").

Final words
I hope this will add some tools to your toolbox, and allow you to restructure this PR in a easier-to-review manner. If things are still unclear or you're stuck somewhere, just shout and I'll try to help a bit more :-)

On another note: given you're still working on this PR, maybe you should mark it as a draft for now. Then once everything is cleaned up and we're happy, it can be marked as done, maybe marking a more clear point for the maintainers to have a look at the PR.

@carlopav carlopav marked this pull request as draft May 24, 2021 20:59
@yorikvanhavre
Copy link
Member

As a general note, GIT mastery is never a formal requirement here... We all try to learn and make things better, PR after PR.. In that regard, @matthijskooijman 's comments are precious (I learned a couple of things too), it's time more than well spent to help us making things better ;) But I don't think we would refuse a PR or anything because of not optimal GIT formatting (unless things are very, very bad of course :D )

Copy link
Contributor

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

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

@carlopav Nice, restructured commits look good!

I left some small remarks on the code inline.

As for commit structure: I saw you added the getSpecificObjectInfo and resetTracker-wrapper as two additional commits at the end, rather than folding them back into the history. This is of course fine (as Yorik also emphasized, no need for commit structure to be perfect), but let me at least suggest some advantages of reordering those changes:

  • If you introduce getSpecificObjectInfo in addPoint before splitting it out, then that commit becomes a lot smaller (only one place to change) and easier to review and understand the behavior changes it makes (only working on one obj, rather than all self.selected_objects and/or all hovered objects as before). Similarly, the add_point split commit becomes smaller too.
  • If you make those changes to addPoint first, then the add_point split commit really just moves around code, without changing any behavior, which is again easier to review than a commit that moves around a lot of code and also makes some subtle changes in behavior.
  • If you introduce the resetTracker-wrapper right away in the "restructure" commit, then you can leave out the resetTracker calls from the hande_foo methods, and even more, leave out the handle_foo methods entirely, making that commit again smaller and simpler. Note that I just realized you could maybe even split this further: first move all the resetTracker calls into a single call in evaluate_menu_action, and then do the "rewriting" commit after that, which can then be a little shorter.

Responding to @yorikvanhavre:

As a general note, GIT mastery is never a formal requirement here...

Thanks for confirming that. I do have a tendency to sometimes go a bit overboard with git history perfectionism (both as a contributor and a maintainer for other projects) sometimes, but as a contributor, I think it is important to do it properly because it helps making reviews easier, which allows more people to participate in the review process and takes load off the maintainers, allowing more changes to be merged. Looking at this project, I have the feeling that maintainer time and energy is indeed a more limiting factor than contributor time and energy, so IMHO it would be worth it for me, as a contributor, to spend, say, an hour extra on my commit history, if that shaves of 10 minutes of maintainer review time.

From the maintainer point of view, there is another argument to want a clean git history: It makes reading the history easier too. When figuring out how a piece of code works, the git history is often an invaluable tool for me, to see when and why a surprising bit of code is introduced. A small commit with a isolated change, ideally with a clear message that explains why a change is needed and/or links to a bug report comes a long way to understanding the origins and evolution of a bit of code and often helps to figure out which of many ways to fix a bug is the most appropriate.

Having said all that, there is of course a balance between a which for a clean history, and being open and having a low bar for new contributors. Git by itself can be tricky enough to scare away potential contributors, even without the fancy history editing stuff, especially when the contributors are not experience programmers, which seems to be often the case due to the specific domain that Freecad is in. So it's probably good to not be too picky about git structure, but here's my attempt to maybe raise the git-level of contributions a bit without raising the bar too much :-p

src/Mod/Draft/draftguitools/gui_edit.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit.py Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit_base_object.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit.py Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit.py Outdated Show resolved Hide resolved
src/Mod/Draft/draftguitools/gui_edit_draft_objects.py Outdated Show resolved Hide resolved
Remove unused functions. Since GuiTools objects inherit from the GuiTools class, they do not need those placeholders.
This enable to start editing also without an user click event, by just calling the method and specifying the object and the edit point index.
It is used for alternative edit mode (alt_edit_mode) by arc context menu
Now Draft_Wire, Draft_BSpline, Draft_Bezcurve have their own methods to add points.
get_edit_point_context_menu(self, edit_command, obj, node_idx)
get_edit_obj_context_menu(self, edit_command, obj, position)
that are called depending if the user is over an editpoint or over another part of the object.
Now Draft Edit can reverse the order of the points of a Draft_Wire
The wrapper allows to call resetTrackers in the main module after the callback to the GuiTools is executed.

This is the last commit, many thanks to @matthijskooijman for having menthored me :)

I think it's helpful to have @matthijskooijman explanation on this use of the wrapper:

This defines a new wrapper function, that calls the original callback and then calls resetTrackers. Note that this creates a new function for every loop iteration, so each of these wrapper functions captures potentially different callback, self and obj values so things work as expected. Note I did something weird with the callback value there: Since functions like these capture a variable, not its value at the time of function definition, and loop variables like label and callback are a single variable shared between all loop iterations, capturing callback directly ends up with all wrappers calling the last callback (i.e. they all capture the same variable and by the time the wrappers are called, that variable will contain the last of the callbacks). This is commonly solved by using a default value in the function definition, since such a default value uses the value of the (in this case) callback variable, not capturing the variable.
In new splitted addPoint methods, getObjectsInfo and the consequent checks have been removed and moved to main DraftEdit module in a new get_specific_object_info method.
This method returns the info for the selected object at a given position and the 3d Vector of the point clicked on the object.
@carlopav
Copy link
Contributor Author

carlopav commented Jun 1, 2021

@matthijskooijman thanks, what I learned from the first command line approach:

  • do not use MS Windows Command Prompt, but use Git Bash instead
  • git status and git log --oneline are really helpful (to quit form git log press q and enter!).
  • git rebase --interactive (yes, i meant --interactive, not -interactive) is just wonderful! mark the commit you want to change with edit, let the rebase arrive to that commit, do the changes and save the file, git status to check what you have changed, git add filename to stage the changed file, git rebase --continue to go to the next commit! wow!
  • GitKraken or other graphical interface is still nice to visualize the git status or to quickly check where you made some changes.

@carlopav
Copy link
Contributor Author

carlopav commented Jun 2, 2021

@matthijskooijman can we unmark the PR as Draft? what do you think?

@matthijskooijman
Copy link
Contributor

If you have no further changes you want to make, then yes :-)

@carlopav carlopav marked this pull request as ready for review June 2, 2021 13:10
@carlopav
Copy link
Contributor Author

carlopav commented Jun 3, 2021

@yorikvanhavre when you have some time the PR is ready to be reviewed/merged!
cheers!

@yorikvanhavre yorikvanhavre merged commit 3352ebf into FreeCAD:master Jun 4, 2021
@yorikvanhavre
Copy link
Member

So be it ;)

@carlopav
Copy link
Contributor Author

carlopav commented Jun 4, 2021

thanks!

@carlopav carlopav deleted the DraftEdit_Improvements branch June 4, 2021 14:58
carlopav added a commit to yorikvanhavre/BIM_Workbench that referenced this pull request Jun 6, 2021
FreeCAD/FreeCAD#4811
Updated experimental wall to work with Draft Edit after refactoring its context menu system.
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.

None yet

3 participants