A bunch of UI/UX fixes (#5182)

* A bunch of UI/UX fixes

* Adjusted margins and size of Patch Store dialog
* Param typein now uses correct skin color for its text labels
* Miniedit now has same font as param typein, is wider, has better
  spacing and positioning of OK/Cancel buttons, and has
  focused outline color assigned to match the look of param typein
* Swapped all uses of isCtrlDown() with isCommandDown()
* Switch and MultiSwitch didn't pass key modifiers to controlModifierClicked()

src/common/SkinModel.cpp

@@ -566,7 +566,7 @@ Connector vu_meter =
 Connector mseg_editor = Connector("msegeditor.window", 0, 57, 750, 365, Components::Custom,
                                   Connector::MSEG_EDITOR_WINDOW);
 
-Connector store_patch_dialog = Connector("controls.patch.store.window", 157, 57, 390, 220,
+Connector store_patch_dialog = Connector("controls.patch.store.window", 157, 57, 390, 270,
                                          Components::Custom, Connector::STORE_PATCH_DIALOG);
 
 // modulation panel is special, so it shows up as 'CUSTOM' with no connector and is special-cased in

src/surge-xt/gui/SurgeGUIEditor.cpp

@@ -2751,7 +2751,7 @@ juce::PopupMenu SurgeGUIEditor::makePatchDefaultsMenu(const juce::Point<int> &wh
         txt[0] = 0;
         if (Surge::Storage::isValidUTF8(s))
             strxcpy(txt, s.c_str(), 256);
-        promptForMiniEdit(txt, "Enter default patch author name:", "Set Default Patch Author",
+        promptForMiniEdit(txt, "Enter a default patch author name:", "Set Default Patch Author",
                           where, [this](const std::string &s) {
                               Surge::Storage::updateUserDefaultValue(
                                   &(this->synth->storage), Surge::Storage::DefaultPatchAuthor, s);
@@ -2766,8 +2766,8 @@ juce::PopupMenu SurgeGUIEditor::makePatchDefaultsMenu(const juce::Point<int> &wh
         txt[0] = 0;
         if (Surge::Storage::isValidUTF8(s))
             strxcpy(txt, s.c_str(), 256);
-        promptForMiniEdit(txt, "Enter default patch comment text:", "Set Default Patch Comment",
-                          where, [this](const std::string &s) {
+        promptForMiniEdit(txt, "Enter a default patch comment:", "Set Default Patch Comment", where,
+                          [this](const std::string &s) {
                               Surge::Storage::updateUserDefaultValue(
                                   &(this->synth->storage), Surge::Storage::DefaultPatchComment, s);
                           });
@@ -3274,7 +3274,7 @@ juce::PopupMenu SurgeGUIEditor::makeMidiMenu(const juce::Point<int> &where)
         char msn[256];
         msn[0] = 0;
         promptForMiniEdit(
-            msn, "MIDI Mapping Name", "Save MIDI Mapping", where,
+            msn, "Enter a name for the MIDI mapping:", "Save MIDI Mapping", where,
             [this](const std::string &s) { this->synth->storage.storeMidiMappingToName(s); });
     });
 

src/surge-xt/gui/SurgeGUIEditorValueCallbacks.cpp

@@ -352,11 +352,13 @@ int32_t SurgeGUIEditor::controlModifierClicked(Surge::GUI::IComponentTagValue *c
     if (button.isMiddleButtonDown())
     {
         toggle_mod_editing();
+
         return 1;
     }
 
     juce::Rectangle<int> viewSize;
     auto bvf = control->asJuceComponent();
+
     if (bvf)
     {
         viewSize = bvf->getBounds();
@@ -369,22 +371,24 @@ int32_t SurgeGUIEditor::controlModifierClicked(Surge::GUI::IComponentTagValue *c
     }
 
     /*
-     * Alright why is this here? Well when we show the macro menu we use the modal version to
+     * Alright why is this here? Well, when we show the macro menu we use the modal version to
      * stop changes happening underneath us. It's the only non-async menu we have and probably
-     * should get fixed. But juce has a thing where RMB / RMB means the modal menus release both
-     * after the second RMB. So the end hover on rmb rmb doesn't work leading to the bug in
-     * 4874. A fix to this is to use an async menu. But I'm hnot sure why i didn't and there's an
-     * incomplete comment that it was for good reason so instead...
+     * should get fixed. But JUCE has a thing where RMB RMB means the modal menus release both
+     * after the second RMB. So the end hover on RMB RMB doesn't work, leading to the bug in
+     * #4874. A fix to this is to use an async menu. But I'm not sure why I didn't and there's an
+     * incomplete comment that it was for good reason, so instead...
      */
     for (const auto &g : gui_modsrc)
     {
         if (g && g.get() != control)
+        {
             g->endHover();
+        }
     }
 
     long tag = control->getTag();
 
-    if (button.isCtrlDown() && (tag == tag_mp_patch || tag == tag_mp_category))
+    if (button.isCommandDown() && (tag == tag_mp_patch || tag == tag_mp_category))
     {
         synth->selectRandomPatch();
         return 1;
@@ -1014,10 +1018,14 @@ int32_t SurgeGUIEditor::controlModifierClicked(Surge::GUI::IComponentTagValue *c
         }
     }
 
-    if (!(button.isRightButtonDown() || button.isCtrlDown() || isDoubleClickEvent))
+    /* ED: unsure why this is here, it prevents key modifiers from reaching the following
+       section of code? */
+    /*
+    if (!(button.isRightButtonDown() || isDoubleClickEvent))
     {
         return 0;
     }
+    */
 
     int ptag = tag - start_paramtags;
 
@@ -2211,11 +2219,15 @@ int32_t SurgeGUIEditor::controlModifierClicked(Surge::GUI::IComponentTagValue *c
                 synth->clearModulation(ptag, thisms, current_scene, modsource_index);
                 auto ctrms = dynamic_cast<Surge::Widgets::ModulatableControlInterface *>(control);
                 jassert(ctrms);
+
                 if (ctrms)
                 {
                     auto use_scene = 0;
+
                     if (this->synth->isModulatorDistinctPerScene(thisms))
+                    {
                         use_scene = current_scene;
+                    }
 
                     ctrms->setModValue(
                         synth->getModulation(p->id, thisms, use_scene, modsource_index));
@@ -2224,6 +2236,7 @@ int32_t SurgeGUIEditor::controlModifierClicked(Surge::GUI::IComponentTagValue *c
                         synth->isActiveModulation(p->id, thisms, use_scene, modsource_index));
                     ctrms->setIsModulationBipolar(synth->isBipolarModulation(thisms));
                 }
+
                 oscWaveform->repaint();
 
                 return 0;
@@ -2234,10 +2247,10 @@ int32_t SurgeGUIEditor::controlModifierClicked(Surge::GUI::IComponentTagValue *c
                 {
                 case ct_lfotype:
                     /*
-                    ** This code resets you to default if you double click on control
-                    ** but on the lfoshape UI this is undesirable; it means if you accidentally
-                    ** control click on step sequencer, say, you go back to sin and lose your
-                    ** edits. So supress
+                    ** This code resets you to default if you double-click on control,
+                    ** but on the LFO type widget this is undesirable; it means if you accidentally
+                    ** Control-click on step sequencer, say, you go back to Sine and lose your
+                    ** edits. So supress it!
                     */
                     break;
                 case ct_freq_audible_with_tunability:
@@ -2264,19 +2277,29 @@ int32_t SurgeGUIEditor::controlModifierClicked(Surge::GUI::IComponentTagValue *c
                 {
                     p->set_value_f01(p->get_default_value_f01());
                     control->setValue(p->get_value_f01());
+
                     if (oscWaveform && (p->ctrlgroup == cg_OSC))
+                    {
                         oscWaveform->repaint();
+                    }
+
                     if (lfoDisplay && (p->ctrlgroup == cg_LFO))
+                    {
                         lfoDisplay->repaint();
+                    }
+
                     if (bvf)
+                    {
                         bvf->repaint();
+                    }
+
                     return 0;
                 }
                 }
             }
         }
         // exclusive mute/solo in the mixer
-        else if (button.isCtrlDown())
+        else if (button.isCommandDown())
         {
             if (p->ctrltype == ct_bool_mute)
             {
@@ -2317,9 +2340,12 @@ int32_t SurgeGUIEditor::controlModifierClicked(Surge::GUI::IComponentTagValue *c
                 synth->refresh_editor = true;
             }
             else
+            {
                 p->bound_value();
+            }
         }
     }
+
     return 0;
 }
 
@@ -2337,6 +2363,7 @@ void SurgeGUIEditor::valueChanged(Surge::GUI::IComponentTagValue *control)
 
     juce::Rectangle<int> viewSize;
     auto bvf = control->asJuceComponent();
+
     if (bvf)
     {
         viewSize = bvf->getBounds();
@@ -2762,7 +2789,7 @@ void SurgeGUIEditor::valueChanged(Surge::GUI::IComponentTagValue *control)
                         thisms = cms->getAlternate();
 #endif
                 }
-                bool quantize_mod = juce::ModifierKeys::currentModifiers.isCtrlDown();
+                bool quantize_mod = juce::ModifierKeys::currentModifiers.isCommandDown();
                 float mv = mci->getModValue();
                 if (quantize_mod)
                 {
@@ -2857,7 +2884,7 @@ void SurgeGUIEditor::valueChanged(Surge::GUI::IComponentTagValue *control)
                     }
                 }
 
-                bool force_integer = juce::ModifierKeys::currentModifiers.isCtrlDown();
+                bool force_integer = juce::ModifierKeys::currentModifiers.isCommandDown();
                 SurgeSynthesizer::ID ptagid;
                 synth->fromSynthSideId(ptag, ptagid);
                 if (synth->setParameter01(ptagid, val, false, force_integer))

src/surge-xt/gui/overlays/LuaEditors.cpp

@@ -110,8 +110,7 @@ void CodeEditorContainerWithApply::codeDocumentTextDeleted(int startIndex, int e
 }
 bool CodeEditorContainerWithApply::keyPressed(const juce::KeyPress &key, juce::Component *o)
 {
-    if (key.getKeyCode() == juce::KeyPress::returnKey &&
-        (key.getModifiers().isCommandDown() || key.getModifiers().isCtrlDown()))
+    if (key.getKeyCode() == juce::KeyPress::returnKey && key.getModifiers().isCommandDown())
     {
         applyCode();
         return true;

src/surge-xt/gui/overlays/MSEGEditor.cpp

@@ -1651,19 +1651,21 @@ struct MSEGCanvas : public juce::Component, public Surge::GUI::SkinConsumingComp
                  * Activate temporary snap. Note this is also handled similarly in
                  * onMouseMoved so if you change ctrl/alt here change it there too
                  */
-                bool c = e.mods.isCtrlDown();
-#if MAC
-                c = e.mods.isCommandDown();
-#endif
-
+                bool c = e.mods.isCommandDown();
                 bool a = e.mods.isAltDown();
+
                 if (c || a)
                 {
                     snapGuard = std::make_shared<SnapGuard>(this);
+
                     if (c)
+                    {
                         ms->hSnap = ms->hSnapDefault;
+                    }
                     if (a)
+                    {
                         ms->vSnap = ms->vSnapDefault;
+                    }
                 }
                 break;
             }
@@ -2149,15 +2151,13 @@ struct MSEGCanvas : public juce::Component, public Surge::GUI::SkinConsumingComp
          * Activate temporary snap. Note this is also checked in onMouseDown
          * so if you change ctrl/alt here change it there too
          */
-        bool c = event.mods.isCtrlDown();
-#if MAC
-        c = event.mods.isCommandDown();
-#endif
-
+        bool c = event.mods.isCommandDown();
         bool a = event.mods.isAltDown();
+
         if ((c || a))
         {
             bool wasSnapGuard = true;
+
             if (!snapGuard)
             {
                 wasSnapGuard = false;

src/surge-xt/gui/overlays/MiniEdit.cpp

@@ -28,7 +28,7 @@ MiniEdit::MiniEdit()
 {
     typein = std::make_unique<juce::TextEditor>("minieditTypein");
     typein->setJustification(juce::Justification::centred);
-    typein->setFont(Surge::GUI::getFontManager()->getLatoAtSize(10, juce::Font::bold));
+    typein->setFont(Surge::GUI::getFontManager()->getLatoAtSize(11));
     typein->setSelectAllWhenFocused(true);
     typein->setWantsKeyboardFocus(true);
     typein->addListener(this);
@@ -49,7 +49,7 @@ MiniEdit::~MiniEdit() {}
 
 juce::Rectangle<int> MiniEdit::getDisplayRegion()
 {
-    auto fullRect = juce::Rectangle<int>(0, 0, 160, 80).withCentre(getBounds().getCentre());
+    auto fullRect = juce::Rectangle<int>(0, 0, 180, 80).withCentre(getBounds().getCentre());
     return fullRect;
 }
 
@@ -108,27 +108,32 @@ void MiniEdit::onSkinChanged()
                       skin->getColor(Colors::Dialog::Entry::Focus));
     typein->setColour(juce::TextEditor::outlineColourId,
                       skin->getColor(Colors::Dialog::Entry::Border));
+    typein->setColour(juce::TextEditor::focusedOutlineColourId,
+                      skin->getColor(Colors::Dialog::Entry::Border));
+
     repaint();
 }
 
 void MiniEdit::resized()
 {
-    auto eh = 18, mg = 2, bw = 40;
+    auto typeinHeight = 18, margin = 2, btnWidth = 40;
+
     auto fullRect = getDisplayRegion();
+    auto dialogCenter = fullRect.getWidth() / 2;
+    auto typeinBox = fullRect.translated(0, 2 * typeinHeight + margin * 2)
+                         .withHeight(typeinHeight)
+                         .withTrimmedLeft(2 * margin)
+                         .withTrimmedRight(2 * margin);
 
-    auto typeinBox = fullRect.translated(0, 2 * eh + mg * 2)
-                         .withHeight(eh)
-                         .withTrimmedLeft(2 * mg)
-                         .withTrimmedRight(2 * mg);
     typein->setBounds(typeinBox);
     typein->setIndents(4, (typein->getHeight() - typein->getTextHeight()) / 2);
 
-    auto buttonRow = fullRect.translated(0, (3 * eh) + (mg * 3) + 2)
-                         .withHeight(eh - 4)
-                         .withTrimmedLeft(mg)
-                         .withTrimmedRight(mg);
-    auto okRect = buttonRow.withTrimmedLeft(40).withWidth(40);
-    auto canRect = buttonRow.withTrimmedLeft(80).withWidth(40);
+    auto buttonRow = fullRect.translated(0, (3 * typeinHeight) + (margin * 3) + 2)
+                         .withHeight(typeinHeight - 4)
+                         .withTrimmedLeft(margin)
+                         .withTrimmedRight(margin);
+    auto okRect = buttonRow.withTrimmedLeft(dialogCenter - btnWidth - margin).withWidth(btnWidth);
+    auto canRect = buttonRow.withTrimmedLeft(dialogCenter + margin).withWidth(btnWidth);
 
     okButton->setBounds(okRect);
     cancelButton->setBounds(canRect);
@@ -145,19 +150,27 @@ void MiniEdit::buttonClicked(juce::Button *button)
     {
         callback(typein->getText().toStdString());
     }
+
     setVisible(false);
 }
 
 void MiniEdit::visibilityChanged()
 {
     if (isVisible())
+    {
         grabFocus();
+    }
+
     if (editor)
     {
         if (isVisible())
+        {
             editor->vkbForward++;
+        }
         else
+        {
             editor->vkbForward--;
+        }
     }
 }