From 87a89e71e88988d4ebfd2992e295e69d7fa20219 Mon Sep 17 00:00:00 2001 From: ed Date: Wed, 24 Oct 2018 10:40:43 +0100 Subject: [PATCH] Fixed a crash that could occur due to ValueWithDefault::onChange calling a deleted PropertyComponent object --- .../juce_ChoicePropertyComponent.cpp | 38 +++++++------ .../properties/juce_ChoicePropertyComponent.h | 22 +++++--- .../juce_MultiChoicePropertyComponent.cpp | 55 +++++++++++-------- .../juce_MultiChoicePropertyComponent.h | 14 ++++- .../properties/juce_TextPropertyComponent.cpp | 22 +++++--- .../properties/juce_TextPropertyComponent.h | 17 ++++-- 6 files changed, 104 insertions(+), 64 deletions(-) diff --git a/modules/juce_gui_basics/properties/juce_ChoicePropertyComponent.cpp b/modules/juce_gui_basics/properties/juce_ChoicePropertyComponent.cpp index 197451488a..3b952745a4 100644 --- a/modules/juce_gui_basics/properties/juce_ChoicePropertyComponent.cpp +++ b/modules/juce_gui_basics/properties/juce_ChoicePropertyComponent.cpp @@ -73,9 +73,9 @@ class ChoicePropertyComponent::RemapperValueSourceWithDefault : public Value: private Value::Listener { public: - RemapperValueSourceWithDefault (ValueWithDefault& vwd, const Array& map) + RemapperValueSourceWithDefault (ValueWithDefault* vwd, const Array& map) : valueWithDefault (vwd), - sourceValue (valueWithDefault.getPropertyAsValue()), + sourceValue (valueWithDefault->getPropertyAsValue()), mappings (map) { sourceValue.addListener (this); @@ -83,7 +83,7 @@ public: var getValue() const override { - if (valueWithDefault.isUsingDefault()) + if (valueWithDefault->isUsingDefault()) return -1; auto targetValue = sourceValue.getValue(); @@ -101,24 +101,24 @@ public: if (newValueInt == -1) { - valueWithDefault.resetToDefault(); + valueWithDefault->resetToDefault(); } else { auto remappedVal = mappings [newValueInt - 1]; if (! remappedVal.equalsWithSameType (sourceValue)) - valueWithDefault = remappedVal; + *valueWithDefault = remappedVal; } } private: - ValueWithDefault& valueWithDefault; + void valueChanged (Value&) override { sendChangeMessage (true); } + + ValueWithDefault* valueWithDefault; Value sourceValue; Array mappings; - void valueChanged (Value&) override { sendChangeMessage (true); } - //============================================================================== JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (RemapperValueSourceWithDefault) }; @@ -161,17 +161,19 @@ ChoicePropertyComponent::ChoicePropertyComponent (ValueWithDefault& valueToContr const Array& correspondingValues) : ChoicePropertyComponent (name, choiceList, correspondingValues) { - createComboBoxWithDefault (choiceList [correspondingValues.indexOf (valueToControl.getDefault())]); + valueWithDefault = &valueToControl; - comboBox.getSelectedIdAsValue().referTo (Value (new RemapperValueSourceWithDefault (valueToControl, + createComboBoxWithDefault (choiceList [correspondingValues.indexOf (valueWithDefault->getDefault())]); + + comboBox.getSelectedIdAsValue().referTo (Value (new RemapperValueSourceWithDefault (valueWithDefault, correspondingValues))); - valueToControl.onDefaultChange = [this, &valueToControl, choiceList, correspondingValues] + valueWithDefault->onDefaultChange = [this, choiceList, correspondingValues] { auto selectedId = comboBox.getSelectedId(); comboBox.clear(); - createComboBoxWithDefault (choiceList [correspondingValues.indexOf (valueToControl.getDefault())]); + createComboBoxWithDefault (choiceList [correspondingValues.indexOf (valueWithDefault->getDefault())]); comboBox.setSelectedId (selectedId); }; @@ -182,17 +184,19 @@ ChoicePropertyComponent::ChoicePropertyComponent (ValueWithDefault& valueToContr : PropertyComponent (name), choices ({ "Enabled", "Disabled" }) { - createComboBoxWithDefault (valueToControl.getDefault() ? "Enabled" : "Disabled"); + valueWithDefault = &valueToControl; - comboBox.getSelectedIdAsValue().referTo (Value (new RemapperValueSourceWithDefault (valueToControl, + createComboBoxWithDefault (valueWithDefault->getDefault() ? "Enabled" : "Disabled"); + + comboBox.getSelectedIdAsValue().referTo (Value (new RemapperValueSourceWithDefault (valueWithDefault, { true, false }))); - valueToControl.onDefaultChange = [this, &valueToControl] + valueWithDefault->onDefaultChange = [this] { auto selectedId = comboBox.getSelectedId(); comboBox.clear(); - createComboBoxWithDefault (valueToControl.getDefault() ? "Enabled" : "Disabled"); + createComboBoxWithDefault (valueWithDefault->getDefault() ? "Enabled" : "Disabled"); comboBox.setSelectedId (selectedId); }; @@ -200,6 +204,8 @@ ChoicePropertyComponent::ChoicePropertyComponent (ValueWithDefault& valueToContr ChoicePropertyComponent::~ChoicePropertyComponent() { + if (valueWithDefault != nullptr) + valueWithDefault->onDefaultChange = nullptr; } //============================================================================== diff --git a/modules/juce_gui_basics/properties/juce_ChoicePropertyComponent.h b/modules/juce_gui_basics/properties/juce_ChoicePropertyComponent.h index eacbc2cbfc..8f0406aa72 100644 --- a/modules/juce_gui_basics/properties/juce_ChoicePropertyComponent.h +++ b/modules/juce_gui_basics/properties/juce_ChoicePropertyComponent.h @@ -50,6 +50,10 @@ namespace juce */ class JUCE_API ChoicePropertyComponent : public PropertyComponent { +private: + /** Delegating constructor. */ + ChoicePropertyComponent (const String&, const StringArray&, const Array&); + protected: /** Creates the component. Your subclass's constructor must add a list of options to the choices member variable. @@ -79,7 +83,8 @@ public: /** Creates the component using a ValueWithDefault object. This will add an item to the ComboBox for the default value with an ID of -1. - @param valueToControl the ValueWithDefault object that contains the Value object that the combo box will read and control + @param valueToControl the ValueWithDefault object that contains the Value object that the combo box will read and control. + NB: this object must outlive the ChoicePropertyComponent. @param propertyName the name of the property @param choices the list of possible values that the drop-down list will contain @param correspondingValues a list of values corresponding to each item in the 'choices' StringArray. @@ -135,20 +140,23 @@ protected: StringArray choices; private: - /** Delegating constructor. */ - ChoicePropertyComponent (const String&, const StringArray&, const Array&); - - ComboBox comboBox; - bool isCustomClass = false; - + //============================================================================== class RemapperValueSource; class RemapperValueSourceWithDefault; + //============================================================================== void createComboBox(); void createComboBoxWithDefault (const String&); void changeIndex(); + //============================================================================== + ComboBox comboBox; + bool isCustomClass = false; + + ValueWithDefault* valueWithDefault = nullptr; + + //============================================================================== JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (ChoicePropertyComponent) }; diff --git a/modules/juce_gui_basics/properties/juce_MultiChoicePropertyComponent.cpp b/modules/juce_gui_basics/properties/juce_MultiChoicePropertyComponent.cpp index d9c314bda0..549ac4d482 100644 --- a/modules/juce_gui_basics/properties/juce_MultiChoicePropertyComponent.cpp +++ b/modules/juce_gui_basics/properties/juce_MultiChoicePropertyComponent.cpp @@ -106,10 +106,10 @@ class MultiChoicePropertyComponent::MultiChoiceRemapperSourceWithDefault : pu private Value::Listener { public: - MultiChoiceRemapperSourceWithDefault (ValueWithDefault& vwd, var v, int c, ToggleButton* b) + MultiChoiceRemapperSourceWithDefault (ValueWithDefault* vwd, var v, int c, ToggleButton* b) : valueWithDefault (vwd), varToControl (v), - sourceValue (valueWithDefault.getPropertyAsValue()), + sourceValue (valueWithDefault->getPropertyAsValue()), maxChoices (c), buttonToControl (b) { @@ -118,7 +118,7 @@ public: var getValue() const override { - auto v = valueWithDefault.get(); + auto v = valueWithDefault->get(); if (auto* arr = v.getArray()) { @@ -134,11 +134,11 @@ public: void setValue (const var& newValue) override { - auto v = valueWithDefault.get(); + auto v = valueWithDefault->get(); OptionalScopedPointer> arrayToControl; - if (valueWithDefault.isUsingDefault()) + if (valueWithDefault->isUsingDefault()) arrayToControl.set (new Array(), true); // use an empty array so the default values are overwritten else arrayToControl.set (v.getArray(), false); @@ -149,7 +149,7 @@ public: bool newState = newValue; - if (valueWithDefault.isUsingDefault()) + if (valueWithDefault->isUsingDefault()) { if (auto* defaultArray = v.getArray()) { @@ -171,15 +171,27 @@ public: StringComparator c; temp.sort (c); - valueWithDefault = temp; + *valueWithDefault = temp; if (temp.size() == 0) - valueWithDefault.resetToDefault(); + valueWithDefault->resetToDefault(); } } private: - ValueWithDefault& valueWithDefault; + //============================================================================== + void valueChanged (Value&) override { sendChangeMessage (true); } + + void updateButtonTickColour() const noexcept + { + auto alpha = valueWithDefault->isUsingDefault() ? 0.4f : 1.0f; + auto baseColour = buttonToControl->findColour (ToggleButton::tickColourId); + + buttonToControl->setColour (ToggleButton::tickColourId, baseColour.withAlpha (alpha)); + } + + //============================================================================== + ValueWithDefault* valueWithDefault; var varToControl; Value sourceValue; @@ -187,17 +199,6 @@ private: ToggleButton* buttonToControl; - //============================================================================== - void valueChanged (Value&) override { sendChangeMessage (true); } - - void updateButtonTickColour() const noexcept - { - auto alpha = valueWithDefault.isUsingDefault() ? 0.4f : 1.0f; - auto baseColour = buttonToControl->findColour (ToggleButton::tickColourId); - - buttonToControl->setColour (ToggleButton::tickColourId, baseColour.withAlpha (alpha)); - } - //============================================================================== JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (MultiChoiceRemapperSourceWithDefault) }; @@ -254,16 +255,24 @@ MultiChoicePropertyComponent::MultiChoicePropertyComponent (ValueWithDefault& va int maxChoices) : MultiChoicePropertyComponent (propertyName, choices, correspondingValues) { + valueWithDefault = &valueToControl; + // The value to control must be an array! - jassert (valueToControl.get().isArray()); + jassert (valueWithDefault->get().isArray()); for (int i = 0; i < choiceButtons.size(); ++i) - choiceButtons[i]->getToggleStateValue().referTo (Value (new MultiChoiceRemapperSourceWithDefault (valueToControl, + choiceButtons[i]->getToggleStateValue().referTo (Value (new MultiChoiceRemapperSourceWithDefault (valueWithDefault, correspondingValues[i], maxChoices, choiceButtons[i]))); - valueToControl.onDefaultChange = [this] { repaint(); }; + valueWithDefault->onDefaultChange = [this] { repaint(); }; +} + +MultiChoicePropertyComponent::~MultiChoicePropertyComponent() +{ + if (valueWithDefault != nullptr) + valueWithDefault->onDefaultChange = nullptr; } void MultiChoicePropertyComponent::paint (Graphics& g) diff --git a/modules/juce_gui_basics/properties/juce_MultiChoicePropertyComponent.h b/modules/juce_gui_basics/properties/juce_MultiChoicePropertyComponent.h index 2a4b3fb415..1c0c126a2a 100644 --- a/modules/juce_gui_basics/properties/juce_MultiChoicePropertyComponent.h +++ b/modules/juce_gui_basics/properties/juce_MultiChoicePropertyComponent.h @@ -41,6 +41,10 @@ namespace juce */ class MultiChoicePropertyComponent : public PropertyComponent { +private: + /** Delegating constructor. */ + MultiChoicePropertyComponent (const String&, const StringArray&, const Array&); + public: /** Creates the component. Note that the underlying var object that the Value refers to must be an array. @@ -62,7 +66,8 @@ public: /** Creates the component using a ValueWithDefault object. This will select the default options. - @param valueToControl the ValueWithDefault object that contains the Value object that the ToggleButtons will read and control + @param valueToControl the ValueWithDefault object that contains the Value object that the ToggleButtons will read and control. + NB: This object must outlive the MultiChoicePropertyComponent. @param propertyName the name of the property @param choices the list of possible values that will be represented @param correspondingValues a list of values corresponding to each item in the 'choices' StringArray. @@ -78,6 +83,8 @@ public: const Array& correspondingValues, int maxChoices = -1); + ~MultiChoicePropertyComponent(); + //============================================================================== /** Returns true if the list of options is expanded. */ bool isExpanded() const noexcept { return expanded; } @@ -104,8 +111,7 @@ public: void refresh() override {} private: - MultiChoicePropertyComponent (const String&, const StringArray&, const Array&); - + //============================================================================== class MultiChoiceRemapperSource; class MultiChoiceRemapperSourceWithDefault; @@ -113,6 +119,8 @@ private: void lookAndFeelChanged() override; //============================================================================== + ValueWithDefault* valueWithDefault; + int maxHeight = 0; int numHidden = 0; bool expanded = false; diff --git a/modules/juce_gui_basics/properties/juce_TextPropertyComponent.cpp b/modules/juce_gui_basics/properties/juce_TextPropertyComponent.cpp index 09166336b9..2238ad07c3 100644 --- a/modules/juce_gui_basics/properties/juce_TextPropertyComponent.cpp +++ b/modules/juce_gui_basics/properties/juce_TextPropertyComponent.cpp @@ -124,26 +124,26 @@ private: class TextPropertyComponent::RemapperValueSourceWithDefault : public Value::ValueSource { public: - RemapperValueSourceWithDefault (const ValueWithDefault& vwd) + RemapperValueSourceWithDefault (ValueWithDefault* vwd) : valueWithDefault (vwd) { } var getValue() const override { - return valueWithDefault.isUsingDefault() ? var() : valueWithDefault.get(); + return valueWithDefault->isUsingDefault() ? var() : valueWithDefault->get(); } void setValue (const var& newValue) override { if (newValue.toString().isEmpty()) - valueWithDefault.resetToDefault(); + valueWithDefault->resetToDefault(); else - valueWithDefault = newValue; + *valueWithDefault = newValue; } private: - ValueWithDefault valueWithDefault; + ValueWithDefault* valueWithDefault; //============================================================================== JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (RemapperValueSourceWithDefault) @@ -171,18 +171,22 @@ TextPropertyComponent::TextPropertyComponent (ValueWithDefault& valueToControl, int maxNumChars, bool multiLine, bool isEditable) : TextPropertyComponent (name, maxNumChars, multiLine, isEditable) { - textEditor->getTextValue().referTo (Value (new RemapperValueSourceWithDefault (valueToControl))); - textEditor->setTextToDisplayWhenEmpty (valueToControl.getDefault(), 0.5f); + valueWithDefault = &valueToControl; - valueToControl.onDefaultChange = [this, &valueToControl] + textEditor->getTextValue().referTo (Value (new RemapperValueSourceWithDefault (valueWithDefault))); + textEditor->setTextToDisplayWhenEmpty (valueWithDefault->getDefault(), 0.5f); + + valueWithDefault->onDefaultChange = [this] { - textEditor->setTextToDisplayWhenEmpty (valueToControl.getDefault(), 0.5f); + textEditor->setTextToDisplayWhenEmpty (valueWithDefault->getDefault(), 0.5f); repaint(); }; } TextPropertyComponent::~TextPropertyComponent() { + if (valueWithDefault != nullptr) + valueWithDefault->onDefaultChange = nullptr; } void TextPropertyComponent::setText (const String& newText) diff --git a/modules/juce_gui_basics/properties/juce_TextPropertyComponent.h b/modules/juce_gui_basics/properties/juce_TextPropertyComponent.h index e8ee51b6cf..fb67ef5dfc 100644 --- a/modules/juce_gui_basics/properties/juce_TextPropertyComponent.h +++ b/modules/juce_gui_basics/properties/juce_TextPropertyComponent.h @@ -74,7 +74,8 @@ public: /** Creates a text property component with a default value. - @param valueToControl The ValueWithDefault that is controlled by the TextPropertyComponent + @param valueToControl The ValueWithDefault that is controlled by the TextPropertyComponent. + NB: this object must outlive the TextPropertyComponent. @param propertyName The name of the property @param maxNumChars If not zero, then this specifies the maximum allowable length of the string. If zero, then the string will have no length limit. @@ -168,19 +169,23 @@ public: virtual void textWasEdited(); private: - bool isMultiLine; - class RemapperValueSourceWithDefault; - class LabelComp; friend class LabelComp; + //============================================================================== + void callListeners(); + void createEditor (int maxNumChars, bool isEditable); + + //============================================================================== + bool isMultiLine; + std::unique_ptr textEditor; ListenerList listenerList; - void callListeners(); - void createEditor (int maxNumChars, bool isEditable); + ValueWithDefault* valueWithDefault = nullptr; + //============================================================================== JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (TextPropertyComponent) };