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

[PartDesign Hole] Add reversed checkbox to dialog #3800

Closed
wants to merge 2 commits into from

Conversation

mwganson
Copy link
Contributor

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:

  • Branch rebased on latest master git pull --rebase upstream master
  • Unit tests confirmed to pass by running ./bin/FreeCAD --run-test 0
  • Commit message is well-written
  • Commit message includes issue #<id> or fixes #<id> where <id> is the associated MantisBT issue id if one exists

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.19 Changelog Forum Thread.


@mwganson
Copy link
Contributor Author

@spanner888
Copy link
Contributor

Built & tested that both the property changes in dialog & property pane and also that the hole does reverse direction.

OS: Debian GNU/Linux 10 (buster) (XFCE/xfce)
Word size of OS: 64-bit
Word size of FreeCAD: 64-bit
Version: 0.19.22257 +1 (Git)
Build type: Unknown
Branch: mwganson/PD_hole_reversed_checkbox
Hash: af58fcc
Python version: 3.7.3
Qt version: 5.11.3
Coin version: 4.0.0a
OCC version: 7.3.0
Locale: English/Australia (en_AU)

@mwganson
Copy link
Contributor Author

Thanks for testing.

Copy link
Contributor

@ezzieyguywuf ezzieyguywuf left a comment

Choose a reason for hiding this comment

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

This change is relatively straightforward - a Qt qui-element appears to have been added in the ".ui" file, a corresponding "slot" added to the "TaskHoleParameters.h" file, and finally the implementation in the cpp file.

Everything looks correct, and from other comments this change appears to work as expected.

@@ -258,7 +258,7 @@
</property>
</widget>
</item>
<item row="22" column="2">
<item row="23" column="2">
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this row number need to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye. Initially I had the Reversed checkbox below the Tapered checkbox, and I had moved the vertical spacer down to the next row, but then I decided to put the Reversed checkbox on the same row as the Tapered checkbox so users would be less likely to need to scroll down as it's already a fairly tall dialog. When I moved the Reversed checkbox up I guess I forgot to also move the spacer back up to where it was originally.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I doubt the row number actually makes a difference, but it's probably worth changing it back to avoid confusing future devs.

Copy link
Contributor

@ezzieyguywuf ezzieyguywuf left a comment

Choose a reason for hiding this comment

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

This was a minor tweak to fix a typo. Again, this overall PR looks solid, no reason I can see not to merge.

@wwmayer
Copy link
Contributor

wwmayer commented Aug 22, 2020

Merged.

One minor issue: when opening the dialog the value of the Reversed checkbox should be restored: 666682d

@wwmayer wwmayer closed this Aug 22, 2020
@mwganson mwganson deleted the hole branch September 4, 2021 17:52
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

4 participants