From 98493ffd0f6c389e6516ae688cf63ab57bbce328 Mon Sep 17 00:00:00 2001
From: Geoff Hutchison <geoff.hutchison@gmail.com>
Date: Tue, 24 Oct 2023 20:55:41 -0400
Subject: [PATCH 1/2] Fix a few UI / UX issues in the surface dialog

- Should no longer add MO / EDens items multiple times
- Should no longer add orbitals multiple times
- Cleaned up the UI grid
- Hides "frame" components unless the molecule a trajectory

Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>
---
 avogadro/qtplugins/surfaces/surfacedialog.cpp | 48 +++++++++------
 avogadro/qtplugins/surfaces/surfacedialog.ui  | 61 +++++++++----------
 2 files changed, 59 insertions(+), 50 deletions(-)

diff --git a/avogadro/qtplugins/surfaces/surfacedialog.cpp b/avogadro/qtplugins/surfaces/surfacedialog.cpp
index 560f24f0da..0e3d960ca0 100644
--- a/avogadro/qtplugins/surfaces/surfacedialog.cpp
+++ b/avogadro/qtplugins/surfaces/surfacedialog.cpp
@@ -19,7 +19,6 @@ SurfaceDialog::SurfaceDialog(QWidget* parent_, Qt::WindowFlags f)
 
   m_ui->orbitalCombo->setVisible(false);
   m_ui->spinCombo->setVisible(false);
-  m_ui->chargeCombo->setVisible(false);
   m_ui->recordButton->setVisible(false);
 
   // set the data for the default items
@@ -151,28 +150,35 @@ void SurfaceDialog::setupBasis(int numElectrons, int numMOs, bool beta)
     return;
 
   m_ui->orbitalCombo->setVisible(true);
-  m_ui->orbitalCombo->setEnabled(false);
+  auto n = m_ui->surfaceCombo->currentIndex();
+  int type = m_ui->surfaceCombo->itemData(n).toInt();
+  if (type == Surfaces::Type::MolecularOrbital ||
+      type == Surfaces::Type::FromFile)
+    m_ui->orbitalCombo->setEnabled(true);
+  else
+    m_ui->orbitalCombo->setEnabled(false);
 
-  m_ui->surfaceCombo->addItem(tr("Molecular Orbital"),
-                              Surfaces::Type::MolecularOrbital);
-  m_ui->surfaceCombo->addItem(tr("Electron Density"),
-                              Surfaces::Type::ElectronDensity);
+  if (m_ui->surfaceCombo->findData(Surfaces::Type::MolecularOrbital) == -1)
+    m_ui->surfaceCombo->addItem(tr("Molecular Orbital"),
+                                Surfaces::Type::MolecularOrbital);
+  if (m_ui->surfaceCombo->findData(Surfaces::Type::ElectronDensity) == -1)
+    m_ui->surfaceCombo->addItem(tr("Electron Density"),
+                                Surfaces::Type::ElectronDensity);
 
   if (beta) {
     m_ui->spinCombo->setVisible(true);
     m_ui->spinCombo->setEnabled(true);
+    if (m_ui->surfaceCombo->findData(Surfaces::Type::SpinDensity) == -1)
+      m_ui->surfaceCombo->addItem(tr("Spin Density"), Surfaces::Type::SpinDensity);
+  } else {
+    auto pos = m_ui->surfaceCombo->findData(Surfaces::Type::SpinDensity);
+    if (pos != -1)
+      m_ui->surfaceCombo->removeItem(pos);
   }
-  // TODO: this class doesn't know about alpha / beta spin right now
-  /*
-  if (numElectrons % 2 != 0) {
-    m_ui->surfaceCombo->addItem(tr("Spin Density"),
-  Surfaces::Type::SpinDensity); m_ui->spinCombo->setVisible(true);
-    m_ui->spinCombo->setEnabled(true);
-  }
-  */
 
   // TODO: get this from the basis information
   QString text;
+  m_ui->orbitalCombo->clear();
   for (int i = 1; i <= numMOs; ++i) {
     text = tr("MO %L1", "Molecular orbital").arg(i);
     if (i == numElectrons / 2)
@@ -204,21 +210,25 @@ void SurfaceDialog::setupCubes(QStringList cubeNames)
 void SurfaceDialog::setupSteps(int stepCount)
 {
   if (stepCount < 2) {
+    m_ui->frameLabel->hide();
     m_ui->stepValue->setEnabled(false);
+    m_ui->stepValue->hide();
     m_ui->recordButton->setEnabled(false);
-    m_ui->recordButton->setVisible(false);
+    m_ui->recordButton->hide();
     m_ui->vcrBack->setEnabled(false);
-    m_ui->vcrBack->setVisible(false);
+    m_ui->vcrBack->hide();
     m_ui->vcrPlay->setEnabled(false);
-    m_ui->vcrPlay->setVisible(false);
+    m_ui->vcrPlay->hide();
     m_ui->vcrForward->setEnabled(false);
-    m_ui->vcrForward->setVisible(false);
+    m_ui->vcrForward->hide();
   } else {
+    m_ui->frameLabel->show();
     m_ui->stepValue->setEnabled(true);
     m_ui->stepValue->setRange(1, stepCount);
     m_ui->stepValue->setSuffix(tr(" of %0").arg(stepCount));
+    m_ui->stepValue->show();
     m_ui->recordButton->setEnabled(true);
-    m_ui->recordButton->setVisible(true);
+    m_ui->recordButton->show();
     /* Disable for now, this would be nice in future.
     m_ui->vcrBack->setEnabled(true);
     m_ui->vcrBack->setVisible(true);
diff --git a/avogadro/qtplugins/surfaces/surfacedialog.ui b/avogadro/qtplugins/surfaces/surfacedialog.ui
index 13cd6eb5e1..0b06d2410d 100644
--- a/avogadro/qtplugins/surfaces/surfacedialog.ui
+++ b/avogadro/qtplugins/surfaces/surfacedialog.ui
@@ -12,8 +12,8 @@
    <rect>
     <x>0</x>
     <y>0</y>
-    <width>610</width>
-    <height>337</height>
+    <width>443</width>
+    <height>354</height>
    </rect>
   </property>
   <property name="contextMenuPolicy">
@@ -30,7 +30,7 @@
   </property>
   <layout class="QVBoxLayout" name="verticalLayout">
    <item>
-    <layout class="QGridLayout" name="gridLayout">
+    <layout class="QFormLayout" name="formLayout">
      <item row="0" column="0">
       <widget class="QLabel" name="label">
        <property name="text">
@@ -122,13 +122,6 @@
          </property>
         </widget>
        </item>
-       <item>
-        <widget class="QComboBox" name="chargeCombo">
-         <property name="enabled">
-          <bool>false</bool>
-         </property>
-        </widget>
-       </item>
        <item>
         <spacer name="horizontalSpacer_4">
          <property name="orientation">
@@ -314,6 +307,9 @@
       <layout class="QHBoxLayout" name="horizontalLayout_3">
        <item>
         <widget class="QDoubleSpinBox" name="isosurfaceDoubleSpinBox">
+         <property name="prefix">
+          <string> </string>
+         </property>
          <property name="decimals">
           <number>4</number>
          </property>
@@ -402,16 +398,16 @@
           <bool>false</bool>
          </property>
          <property name="minimum">
-          <double>0</double>
+          <number>0</number>
          </property>
          <property name="maximum">
-          <double>19</double>
+          <number>19</number>
          </property>
          <property name="singleStep">
-          <double>1</double>
+          <number>1</number>
          </property>
          <property name="value">
-          <double>5</double>
+          <number>5</number>
          </property>
         </widget>
        </item>
@@ -431,7 +427,7 @@
       </layout>
      </item>
      <item row="6" column="0">
-      <widget class="QLabel" name="label_5">
+      <widget class="QLabel" name="frameLabel">
        <property name="text">
         <string>Frame:</string>
        </property>
@@ -458,7 +454,8 @@
           <string/>
          </property>
          <property name="icon">
-          <iconset theme="media-seek-backward"/>
+          <iconset theme="media-seek-backward">
+           <normaloff>.</normaloff>.</iconset>
          </property>
         </widget>
        </item>
@@ -471,7 +468,8 @@
           <string/>
          </property>
          <property name="icon">
-           <iconset theme="media-playback-start"/>
+          <iconset theme="media-playback-start">
+           <normaloff>.</normaloff>.</iconset>
          </property>
         </widget>
        </item>
@@ -484,7 +482,8 @@
           <string/>
          </property>
          <property name="icon">
-          <iconset theme="media-seek-forward"/>
+          <iconset theme="media-seek-forward">
+           <normaloff>.</normaloff>.</iconset>
          </property>
         </widget>
        </item>
@@ -505,19 +504,6 @@
      </item>
     </layout>
    </item>
-   <item>
-    <spacer name="verticalSpacer">
-     <property name="orientation">
-      <enum>Qt::Vertical</enum>
-     </property>
-     <property name="sizeHint" stdset="0">
-      <size>
-       <width>20</width>
-       <height>40</height>
-      </size>
-     </property>
-    </spacer>
-   </item>
    <item>
     <layout class="QHBoxLayout" name="horizontalLayout_4">
      <item>
@@ -555,6 +541,19 @@
      </item>
     </layout>
    </item>
+   <item>
+    <spacer name="verticalSpacer">
+     <property name="orientation">
+      <enum>Qt::Vertical</enum>
+     </property>
+     <property name="sizeHint" stdset="0">
+      <size>
+       <width>20</width>
+       <height>40</height>
+      </size>
+     </property>
+    </spacer>
+   </item>
   </layout>
  </widget>
  <resources/>

From 7c60469104b2c37cc5648a170339767114ca1a8e Mon Sep 17 00:00:00 2001
From: Geoff Hutchison <geoff.hutchison@gmail.com>
Date: Tue, 24 Oct 2023 21:00:28 -0400
Subject: [PATCH 2/2] Fixed formatting

Signed-off-by: Geoff Hutchison <geoff.hutchison@gmail.com>
---
 avogadro/qtplugins/surfaces/surfacedialog.cpp | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/avogadro/qtplugins/surfaces/surfacedialog.cpp b/avogadro/qtplugins/surfaces/surfacedialog.cpp
index 0e3d960ca0..bb9db140e4 100644
--- a/avogadro/qtplugins/surfaces/surfacedialog.cpp
+++ b/avogadro/qtplugins/surfaces/surfacedialog.cpp
@@ -11,7 +11,7 @@ namespace Avogadro::QtPlugins {
 
 SurfaceDialog::SurfaceDialog(QWidget* parent_, Qt::WindowFlags f)
   : QDialog(parent_, f), m_ui(new Ui::SurfaceDialog),
-  m_automaticResolution(true)
+    m_automaticResolution(true)
 {
   m_ui->setupUi(this);
 
@@ -68,7 +68,7 @@ void SurfaceDialog::propertyComboChanged(int n)
     case 1: // Electrostatic Potential
       m_ui->colormapCombo->setEnabled(true);
       m_ui->modelCombo->setEnabled(true);
-      for (const auto &model: m_chargeModels)
+      for (const auto& model : m_chargeModels)
         m_ui->modelCombo->addItem(model.first.c_str(), model.second.c_str());
   }
 }
@@ -169,7 +169,8 @@ void SurfaceDialog::setupBasis(int numElectrons, int numMOs, bool beta)
     m_ui->spinCombo->setVisible(true);
     m_ui->spinCombo->setEnabled(true);
     if (m_ui->surfaceCombo->findData(Surfaces::Type::SpinDensity) == -1)
-      m_ui->surfaceCombo->addItem(tr("Spin Density"), Surfaces::Type::SpinDensity);
+      m_ui->surfaceCombo->addItem(tr("Spin Density"),
+                                  Surfaces::Type::SpinDensity);
   } else {
     auto pos = m_ui->surfaceCombo->findData(Surfaces::Type::SpinDensity);
     if (pos != -1)
@@ -241,8 +242,8 @@ void SurfaceDialog::setupSteps(int stepCount)
 }
 
 void SurfaceDialog::setupModels(
-    const std::set<std::pair<std::string, std::string>> &chargeModels
-) {
+  const std::set<std::pair<std::string, std::string>>& chargeModels)
+{
   m_chargeModels = chargeModels;
 }
 
@@ -253,7 +254,8 @@ Surfaces::Type SurfaceDialog::surfaceType()
 
 Surfaces::ColorProperty SurfaceDialog::colorProperty()
 {
-  return static_cast<Surfaces::ColorProperty>(m_ui->propertyCombo->currentIndex());
+  return static_cast<Surfaces::ColorProperty>(
+    m_ui->propertyCombo->currentIndex());
 }
 
 QString SurfaceDialog::colorModel()
@@ -330,4 +332,4 @@ void SurfaceDialog::enableRecord()
   m_ui->recordButton->setEnabled(true);
 }
 
-} // End namespace Avogadro
+} // namespace Avogadro::QtPlugins