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

Path: Fixes #6849 - Properly cancel new Lead In/Out dressup [Hold] #7130

Closed

Conversation

Russ4262
Copy link
Contributor

@Russ4262 Russ4262 commented Jul 3, 2022

Make modifications to viewprovider and taskpanel classes to properly handle cancellation of new lead in/out dressup when Cancel button is pressed.

Forum discussion at Bug: LeadInOut dressup created on Cancel.

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 issue ID number from our Issues database in case a particular commit solves or is related to an existing issue. Ex: Draft: fix typos - fixes #4805

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.


@github-actions github-actions bot added the WB CAM Related to the CAM/Path Workbench label Jul 3, 2022
@luzpaz luzpaz requested a review from sliptonic July 4, 2022 11:51
@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7130. Pipeline 579735902 was triggered at e38945c. All CI branches and pipelines.

@sliptonic
Copy link
Member

I don't have a problem merging this as it solves the problem and works as described.
Without digging farther into it, I'm curious why the solution is needed though.
Isn't the dressup being created within a transaction? Shouldn't aborting that transaction occur if the user hits cancel?
It seems odd that we need to initiate and complete a transaction to remove the dressup when it shouldn't have been created at all.

@Russ4262
Copy link
Contributor Author

Russ4262 commented Jul 7, 2022

My thought was the same, that aborting the transaction should work. Unfortunately, there appears to be more at play. As for the solution in this PR, it is modeled after PathOpGui code usage.

There is another solution that involves a refactor. We can back burner this PR. I can convert this PR to draft.
Offending line is

FreeCAD.ActiveDocument.commitTransaction()
. But this line is not the only offending code for proper Cancel button functionality.

@Russ4262 Russ4262 marked this pull request as draft July 7, 2022 23:24
@Russ4262 Russ4262 changed the title Path: Fixes #6849 - Properly cancel new Lead In/Out dressup Path: Fixes #6849 - Properly cancel new Lead In/Out dressup [Hold] Jul 7, 2022
@Russ4262 Russ4262 closed this Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB CAM Related to the CAM/Path Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants