PD: hole change the task panel threaded and model thread checkboxes into a combo/dropdown (#21664)

* PD: hole change threaded and modeled to combo in taskpanel

* Gui: Property editor apply review suggestions

src/Mod/PartDesign/Gui/TaskHoleParameters.cpp

@@ -76,11 +76,12 @@ TaskHoleParameters::TaskHoleParameters(ViewProviderHole* HoleView, QWidget* pare
     auto pcHole = getObject<PartDesign::Hole>();
     bool isNone = std::string(pcHole->ThreadType.getValueAsString()) == "None";
 
-    ui->Threaded->setHidden(isNone);
+    ui->labelHoleType->setHidden(isNone);
+    ui->HoleType->setHidden(isNone);
     ui->ThreadSize->setHidden(isNone);
     ui->labelSize->setHidden(isNone);
 
-    ui->Threaded->setChecked(pcHole->Threaded.getValue());
+    updateHoleTypeCombo();
     ui->ThreadType->setCurrentIndex(pcHole->ThreadType.getValue());
 
     ui->ThreadSize->clear();
@@ -158,17 +159,15 @@ TaskHoleParameters::TaskHoleParameters(ViewProviderHole* HoleView, QWidget* pare
     ui->TaperedAngle->setValue(pcHole->TaperedAngle.getValue());
     ui->Reversed->setChecked(pcHole->Reversed.getValue());
 
-    bool isThreaded = ui->Threaded->isChecked();
+    bool isThreaded = pcHole->Threaded.getValue();
     bool isModeled = pcHole->ModelThread.getValue();
     ui->ThreadGroupBox->setVisible(isThreaded);
     ui->ClearanceWidget->setHidden(isNone || isThreaded);
-    ui->ModelThread->setChecked(isModeled);
     ui->UseCustomThreadClearance->setChecked(pcHole->UseCustomThreadClearance.getValue());
     ui->CustomThreadClearance->setValue(pcHole->CustomThreadClearance.getValue());
     ui->ThreadDepthType->setCurrentIndex(pcHole->ThreadDepthType.getValue());
     ui->ThreadDepth->setValue(pcHole->ThreadDepth.getValue());
 
-    ui->ModelThread->setEnabled(ui->Threaded->isChecked() && ui->ThreadType->currentIndex() != 0);
     ui->CustomClearanceWidget->setVisible(isThreaded && isModeled);
     ui->CustomThreadClearance->setEnabled(ui->UseCustomThreadClearance->isChecked());
     ui->UpdateView->setChecked(false);
@@ -186,8 +185,8 @@ TaskHoleParameters::TaskHoleParameters(ViewProviderHole* HoleView, QWidget* pare
     setCutDiagram();
 
     // clang-format off
-    connect(ui->Threaded, &QCheckBox::clicked,
-            this, &TaskHoleParameters::threadedChanged);
+    connect(ui->HoleType, qOverload<int>(&QComboBox::currentIndexChanged),
+            this, &TaskHoleParameters::holeTypeChanged);
     connect(ui->ThreadType, qOverload<int>(&QComboBox::currentIndexChanged),
             this, &TaskHoleParameters::threadTypeChanged);
     connect(ui->ThreadSize, qOverload<int>(&QComboBox::currentIndexChanged),
@@ -228,8 +227,6 @@ TaskHoleParameters::TaskHoleParameters(ViewProviderHole* HoleView, QWidget* pare
             this, &TaskHoleParameters::reversedChanged);
     connect(ui->TaperedAngle, qOverload<double>(&Gui::QuantitySpinBox::valueChanged),
             this, &TaskHoleParameters::taperedAngleChanged);
-    connect(ui->ModelThread, &QCheckBox::clicked,
-            this, &TaskHoleParameters::modelThreadChanged);
     connect(ui->UpdateView, &QCheckBox::toggled,
             this, &TaskHoleParameters::updateViewChanged);
     connect(ui->UseCustomThreadClearance, &QCheckBox::toggled,
@@ -266,31 +263,23 @@ TaskHoleParameters::TaskHoleParameters(ViewProviderHole* HoleView, QWidget* pare
 
 TaskHoleParameters::~TaskHoleParameters() = default;
 
-void TaskHoleParameters::threadedChanged()
+void TaskHoleParameters::holeTypeChanged(int index)
 {
-    auto pcHole = getObject<PartDesign::Hole>();
-
-    bool isChecked = ui->Threaded->isChecked();
-    pcHole->Threaded.setValue(isChecked);
-
-    ui->ThreadGroupBox->setVisible(isChecked);
-    ui->ClearanceWidget->setHidden(isChecked);
-    // run modelThreadChanged
-    // it will handle the visibility of the model options
-    modelThreadChanged();
-    recomputeFeature();
-}
-
-void TaskHoleParameters::modelThreadChanged()
-{
-    auto pcHole = getObject<PartDesign::Hole>();
-    bool isThreaded = pcHole->Threaded.getValue();
-    bool isModeled = isThreaded && ui->ModelThread->isChecked();
-    if (!isThreaded) {
-        ui->ModelThread->setChecked(false);
+    if (index < 0) {
+        return;
     }
+    auto pcHole = getObject<PartDesign::Hole>();
+    if (!pcHole) {
+        return;
+    }
+    bool isThreaded = getThreaded();
+    bool isModeled = getModelThread();
+
+    pcHole->Threaded.setValue(isThreaded);
     pcHole->ModelThread.setValue(isModeled);
 
+    ui->ThreadGroupBox->setVisible(isThreaded);
+    ui->ClearanceWidget->setHidden(isThreaded);
     // update view not active if modeling threads
     // this will also ensure that the feature is recomputed.
     ui->UpdateView->setVisible(isModeled);
@@ -298,7 +287,7 @@ void TaskHoleParameters::modelThreadChanged()
 
     // conditional enabling of thread modeling options
     ui->CustomClearanceWidget->setVisible(isModeled);
-    ui->CustomThreadClearance->setEnabled(ui->UseCustomThreadClearance->isChecked());
+    ui->CustomThreadClearance->setEnabled(pcHole->UseCustomThreadClearance.getValue());
 
     ui->ThreadDepthWidget->setVisible(isThreaded && isModeled);
     ui->ThreadDepthDimensionWidget->setVisible(
@@ -651,10 +640,11 @@ void TaskHoleParameters::threadTypeChanged(int index)
     // Threaded checkbox is meaningless if no thread profile is selected.
     bool isNone = std::string(hole->ThreadType.getValueAsString()) == "None";
     bool isThreaded = hole->Threaded.getValue();
-    ui->Threaded->setHidden(isNone);
     ui->ThreadGroupBox->setHidden(isNone || !isThreaded);
     ui->ThreadSize->setHidden(isNone);
     ui->labelSize->setHidden(isNone);
+    ui->labelHoleType->setHidden(isNone);
+    ui->HoleType->setHidden(isNone);
     ui->ClearanceWidget->setHidden(isNone || isThreaded);
 
     if (TypeClass == QByteArray("None")) {
@@ -799,9 +789,8 @@ void TaskHoleParameters::changedObject(const App::Document&, const App::Property
         widget->setDisabled(ro);
     };
 
-    if (&Prop == &hole->Threaded) {
-        ui->Threaded->setEnabled(true);
-        updateCheckable(ui->Threaded, hole->Threaded.getValue());
+    if (&Prop == &hole->Threaded || &Prop == &hole->ModelThread) {
+        updateHoleTypeCombo();
     }
     else if (&Prop == &hole->ThreadType) {
         ui->ThreadType->setEnabled(true);
@@ -890,10 +879,6 @@ void TaskHoleParameters::changedObject(const App::Document&, const App::Property
         ui->TaperedAngle->setEnabled(true);
         updateSpinBox(ui->TaperedAngle, hole->TaperedAngle.getValue());
     }
-    else if (&Prop == &hole->ModelThread) {
-        ui->ModelThread->setEnabled(true);
-        updateCheckable(ui->ModelThread, hole->ModelThread.getValue());
-    }
     else if (&Prop == &hole->UseCustomThreadClearance) {
         ui->UseCustomThreadClearance->setEnabled(true);
         updateCheckable(ui->UseCustomThreadClearance, hole->UseCustomThreadClearance.getValue());
@@ -915,6 +900,24 @@ void TaskHoleParameters::changedObject(const App::Document&, const App::Property
     }
 }
 
+void TaskHoleParameters::updateHoleTypeCombo()
+{
+    auto hole = getObject<PartDesign::Hole>();
+    if (!hole) {
+        return;
+    }
+    [[maybe_unused]] QSignalBlocker blocker(ui->HoleType);
+    if (hole->Threaded.getValue()) {
+        if (hole->ModelThread.getValue()) {
+            ui->HoleType->setCurrentIndex(ModeledThread);
+        } else {
+            ui->HoleType->setCurrentIndex(TapDrill);
+        }
+    } else {
+        ui->HoleType->setCurrentIndex(Clearance);
+    }
+}
+
 void TaskHoleParameters::onSelectionChanged(const Gui::SelectionChanges& msg)
 {
     Q_UNUSED(msg)
@@ -922,7 +925,12 @@ void TaskHoleParameters::onSelectionChanged(const Gui::SelectionChanges& msg)
 
 bool TaskHoleParameters::getThreaded() const
 {
-    return ui->Threaded->isChecked();
+    return ui->HoleType->currentIndex() != Clearance;
+}
+
+bool TaskHoleParameters::getModelThread() const
+{
+    return ui->HoleType->currentIndex() == ModeledThread;
 }
 
 long TaskHoleParameters::getThreadType() const
@@ -1047,11 +1055,6 @@ double TaskHoleParameters::getCustomThreadClearance() const
     return ui->CustomThreadClearance->value().getValue();
 }
 
-bool TaskHoleParameters::getModelThread() const
-{
-    return ui->ModelThread->isChecked();
-}
-
 long TaskHoleParameters::getThreadDepthType() const
 {
     return ui->ThreadDepthType->currentIndex();

src/Mod/PartDesign/Gui/TaskHoleParameters.h

@@ -82,7 +82,7 @@ public:
     int getBaseProfileType() const;
 
 private Q_SLOTS:
-    void threadedChanged();
+    void holeTypeChanged(int index);
     void threadTypeChanged(int index);
     void threadSizeChanged(int index);
     void threadClassChanged(int index);
@@ -103,7 +103,6 @@ private Q_SLOTS:
     void taperedChanged();
     void taperedAngleChanged(double value);
     void reversedChanged();
-    void modelThreadChanged();
     void useCustomThreadClearanceChanged();
     void customThreadClearanceChanged(double value);
     void updateViewChanged(bool isChecked);
@@ -121,6 +120,11 @@ private:
         TaskHoleParameters * owner;
         PartDesign::Hole * hole;
     };
+    enum HoleTypeIndex : int {
+        Clearance = 0,
+        TapDrill = 1,
+        ModeledThread = 2
+    };
 
 protected:
     void changeEvent(QEvent *e) override;
@@ -129,6 +133,7 @@ protected:
 private:
     void onSelectionChanged(const Gui::SelectionChanges &msg) override;
     void updateHoleCutLimits(PartDesign::Hole* hole);
+    void updateHoleTypeCombo();
 
 private:
 

src/Mod/PartDesign/Gui/TaskHoleParameters.ui

@@ -615,17 +615,7 @@ over 90: larger hole radius at the bottom</string>
      <property name="topMargin">
       <number>0</number>
      </property>
-     <item row="1" column="1">
-      <widget class="QComboBox" name="ThreadType">
-       <property name="sizePolicy">
-        <sizepolicy hsizetype="Ignored" vsizetype="Fixed">
-         <horstretch>0</horstretch>
-         <verstretch>0</verstretch>
-        </sizepolicy>
-       </property>
-      </widget>
-     </item>
-     <item row="1" column="0">
+     <item row="0" column="0">
       <widget class="Gui::ElideLabel" name="labelThreadType">
        <property name="sizePolicy">
         <sizepolicy hsizetype="Ignored" vsizetype="Preferred">
@@ -638,8 +628,8 @@ over 90: larger hole radius at the bottom</string>
        </property>
       </widget>
      </item>
-     <item row="2" column="1">
-      <widget class="QComboBox" name="ThreadSize">
+     <item row="0" column="1">
+      <widget class="QComboBox" name="ThreadType">
        <property name="sizePolicy">
         <sizepolicy hsizetype="Ignored" vsizetype="Fixed">
          <horstretch>0</horstretch>
@@ -648,7 +638,7 @@ over 90: larger hole radius at the bottom</string>
        </property>
       </widget>
      </item>
-     <item row="2" column="0">
+     <item row="1" column="0">
       <widget class="Gui::ElideLabel" name="labelSize">
        <property name="sizePolicy">
         <sizepolicy hsizetype="Ignored" vsizetype="Preferred">
@@ -661,24 +651,44 @@ over 90: larger hole radius at the bottom</string>
        </property>
       </widget>
      </item>
+     <item row="1" column="1">
+      <widget class="QComboBox" name="ThreadSize">
+       <property name="sizePolicy">
+        <sizepolicy hsizetype="Ignored" vsizetype="Fixed">
+         <horstretch>0</horstretch>
+         <verstretch>0</verstretch>
+        </sizepolicy>
+       </property>
+      </widget>
+     </item>
+     <item row="2" column="1">
+      <widget class="QComboBox" name="HoleType">
+       <item>
+        <property name="text">
+         <string>Clearance / Passthrough</string>
+        </property>
+       </item>
+       <item>
+        <property name="text">
+         <string>Tap Drill (To Be Threaded)</string>
+        </property>
+       </item>
+       <item>
+        <property name="text">
+         <string>Modeled Thread</string>
+        </property>
+       </item>
+      </widget>
+     </item>
+     <item row="2" column="0">
+      <widget class="Gui::ElideLabel" name="labelHoleType">
+       <property name="text">
+        <string>Hole Type</string>
+       </property>
+      </widget>
+     </item>
     </layout>
    </item>
-   <item>
-    <widget class="QCheckBox" name="Threaded">
-     <property name="sizePolicy">
-      <sizepolicy hsizetype="Expanding" vsizetype="Fixed">
-       <horstretch>0</horstretch>
-       <verstretch>0</verstretch>
-      </sizepolicy>
-     </property>
-     <property name="toolTip">
-      <string>Whether the hole gets a thread</string>
-     </property>
-     <property name="text">
-      <string>Threaded</string>
-     </property>
-    </widget>
-   </item>
    <item>
     <widget class="QWidget" name="ClearanceWidget" native="true">
      <property name="sizePolicy">
@@ -778,6 +788,26 @@ Only available for holes without thread</string>
       <property name="bottomMargin">
        <number>6</number>
       </property>
+      <item>
+       <widget class="Gui::ElideCheckBox" name="UpdateView">
+        <property name="enabled">
+         <bool>true</bool>
+        </property>
+        <property name="sizePolicy">
+         <sizepolicy hsizetype="Ignored" vsizetype="Fixed">
+          <horstretch>0</horstretch>
+          <verstretch>0</verstretch>
+         </sizepolicy>
+        </property>
+        <property name="toolTip">
+         <string>Live update of changes to the thread
+Note that the calculation can take some time</string>
+        </property>
+        <property name="text">
+         <string>Update thread view</string>
+        </property>
+       </widget>
+      </item>
       <item>
        <layout class="QGridLayout" name="classLayout" columnstretch="2,5">
         <property name="topMargin">
@@ -876,52 +906,6 @@ Only available for holes without thread</string>
         </item>
        </layout>
       </item>
-      <item>
-       <layout class="QHBoxLayout" name="horizontalLayout_5">
-        <property name="leftMargin">
-         <number>0</number>
-        </property>
-        <property name="topMargin">
-         <number>0</number>
-        </property>
-        <item>
-         <widget class="Gui::ElideCheckBox" name="ModelThread">
-          <property name="sizePolicy">
-           <sizepolicy hsizetype="Ignored" vsizetype="Fixed">
-            <horstretch>0</horstretch>
-            <verstretch>0</verstretch>
-           </sizepolicy>
-          </property>
-          <property name="toolTip">
-           <string>Whether the hole gets a modelled thread</string>
-          </property>
-          <property name="text">
-           <string>Model Thread</string>
-          </property>
-         </widget>
-        </item>
-        <item>
-         <widget class="Gui::ElideCheckBox" name="UpdateView">
-          <property name="enabled">
-           <bool>true</bool>
-          </property>
-          <property name="sizePolicy">
-           <sizepolicy hsizetype="Ignored" vsizetype="Fixed">
-            <horstretch>0</horstretch>
-            <verstretch>0</verstretch>
-           </sizepolicy>
-          </property>
-          <property name="toolTip">
-           <string>Live update of changes to the thread
-Note that the calculation can take some time</string>
-          </property>
-          <property name="text">
-           <string>Update thread view</string>
-          </property>
-         </widget>
-        </item>
-       </layout>
-      </item>
       <item>
        <widget class="QWidget" name="ThreadDepthWidget" native="true">
         <property name="sizePolicy">
@@ -944,7 +928,7 @@ Note that the calculation can take some time</string>
           <number>0</number>
          </property>
          <item>
-          <layout class="QHBoxLayout" name="horizontalLayout_11" stretch="2,5">
+          <layout class="QHBoxLayout" name="depthTypeLayout">
            <property name="spacing">
             <number>6</number>
            </property>
@@ -993,7 +977,7 @@ Note that the calculation can take some time</string>
          </item>
          <item>
           <widget class="QWidget" name="ThreadDepthDimensionWidget" native="true">
-           <layout class="QHBoxLayout" name="horizontalLayout_3" stretch="2,5">
+           <layout class="QHBoxLayout" name="horizontalLayout_3">
             <property name="leftMargin">
              <number>0</number>
             </property>
@@ -1037,7 +1021,7 @@ Note that the calculation can take some time</string>
       </item>
       <item>
        <widget class="QWidget" name="CustomClearanceWidget" native="true">
-        <layout class="QHBoxLayout" name="horizontalLayout_13" stretch="2,5">
+        <layout class="QHBoxLayout" name="horizontalLayout_13">
          <property name="spacing">
           <number>6</number>
          </property>