diff --git a/modules/juce_core/containers/juce_Array.h b/modules/juce_core/containers/juce_Array.h index 010cbbc302..05f9579974 100644 --- a/modules/juce_core/containers/juce_Array.h +++ b/modules/juce_core/containers/juce_Array.h @@ -829,9 +829,10 @@ public: If the item isn't found, no action is taken. @param valueToRemove the object to try to remove + @returns the index of the removed item, or -1 if the item isn't found @see remove, removeRange, removeIf */ - void removeFirstMatchingValue (ParameterType valueToRemove) + int removeFirstMatchingValue (ParameterType valueToRemove) { const ScopedLockType lock (getLock()); auto* e = values.begin(); @@ -841,9 +842,11 @@ public: if (valueToRemove == e[i]) { removeInternal (i); - break; + return i; } } + + return -1; } /** Removes items from the array. diff --git a/modules/juce_core/containers/juce_ListenerList.cpp b/modules/juce_core/containers/juce_ListenerList.cpp new file mode 100644 index 0000000000..0dabe7ff7b --- /dev/null +++ b/modules/juce_core/containers/juce_ListenerList.cpp @@ -0,0 +1,317 @@ +/* + ============================================================================== + + This file is part of the JUCE 7 technical preview. + Copyright (c) 2022 - Raw Material Software Limited + + You may use this code under the terms of the GPL v3 + (see www.gnu.org/licenses). + + For the technical preview this file cannot be licensed commercially. + + JUCE IS PROVIDED "AS IS" WITHOUT ANY WARRANTY, AND ALL WARRANTIES, WHETHER + EXPRESSED OR IMPLIED, INCLUDING MERCHANTABILITY AND FITNESS FOR PURPOSE, ARE + DISCLAIMED. + + ============================================================================== +*/ + +namespace juce +{ + +#if JUCE_UNIT_TESTS + +class ListenerListTests : public UnitTest +{ +public: + //============================================================================== + class TestListener + { + public: + explicit TestListener (std::function cb) : callback (std::move (cb)) {} + + void doCallback() + { + ++numCalls; + callback(); + } + + int getNumCalls() const { return numCalls; } + + private: + int numCalls = 0; + std::function callback; + }; + + class TestObject + { + public: + void addListener (std::function cb) + { + listeners.push_back (std::make_unique (std::move (cb))); + listenerList.add (listeners.back().get()); + } + + void removeListener (int i) { listenerList.remove (listeners[(size_t) i].get()); } + + void callListeners() + { + ++callLevel; + listenerList.call ([] (auto& l) { l.doCallback(); }); + --callLevel; + } + + int getNumListeners() const { return (int) listeners.size(); } + + auto& getListener (int i) { return *listeners[(size_t) i]; } + + int getCallLevel() const + { + return callLevel; + } + + bool wereAllNonRemovedListenersCalled (int numCalls) const + { + return std::all_of (std::begin (listeners), + std::end (listeners), + [&] (auto& listener) + { + return (! listenerList.contains (listener.get())) || listener->getNumCalls() == numCalls; + }); + } + + private: + std::vector> listeners; + ListenerList listenerList; + int callLevel = 0; + }; + + //============================================================================== + ListenerListTests() : UnitTest ("ListenerList", UnitTestCategories::containers) {} + + void runTest() override + { + // This is a test that the pre-iterator adjustment implementation should pass too + beginTest ("All non-removed listeners should be called - removing an already called listener"); + { + TestObject test; + + for (int i = 0; i < 20; ++i) + { + test.addListener ([i, &test] + { + if (i == 5) + test.removeListener (6); + }); + } + + test.callListeners(); + expect (test.wereAllNonRemovedListenersCalled (1)); + } + + // Iterator adjustment is necessary for passing this + beginTest ("All non-removed listeners should be called - removing a yet uncalled listener"); + { + TestObject test; + + for (int i = 0; i < 20; ++i) + { + test.addListener ([i, &test] + { + if (i == 5) + test.removeListener (4); + }); + } + + test.callListeners(); + expect (test.wereAllNonRemovedListenersCalled (1)); + } + + // This test case demonstrates why we have to call --it.index instead of it.next() + beginTest ("All non-removed listeners should be called - one callback removes multiple listeners"); + { + TestObject test; + + for (int i = 0; i < 20; ++i) + { + test.addListener ([i, &test] + { + if (i == 19) + { + test.removeListener (19); + test.removeListener (0); + } + }); + } + + test.callListeners(); + expect (test.wereAllNonRemovedListenersCalled (1)); + } + + beginTest ("All non-removed listeners should be called - removing listeners randomly"); + { + auto random = getRandom(); + + for (auto run = 0; run < 10; ++run) + { + const auto numListeners = random.nextInt ({ 10, 100 }); + const auto listenersThatRemoveListeners = chooseUnique (random, + numListeners, + random.nextInt ({ 0, numListeners / 2 })); + + // The listener in position [key] should remove listeners in [value] + std::map> removals; + + for (auto i : listenersThatRemoveListeners) + { + // Random::nextInt ({1, 1}); triggers an assertion + removals[i] = chooseUnique (random, + numListeners, + random.nextInt ({ 1, std::max (2, numListeners / 10) })); + } + + TestObject test; + + for (int i = 0; i < numListeners; ++i) + { + test.addListener ([i, &removals, &test] + { + const auto iter = removals.find (i); + + if (iter == removals.end()) + return; + + for (auto j : iter->second) + { + test.removeListener (j); + } + }); + } + + test.callListeners(); + expect (test.wereAllNonRemovedListenersCalled (1)); + } + } + + // Iterator adjustment is not necessary for passing this + beginTest ("All non-removed listeners should be called - add listener during iteration"); + { + TestObject test; + const auto numStartingListeners = 20; + + for (int i = 0; i < numStartingListeners; ++i) + { + test.addListener ([i, &test] + { + if (i == 5 || i == 6) + test.addListener ([] {}); + }); + } + + test.callListeners(); + + // Only the Listeners added before the test can be expected to have been called + bool success = true; + + for (int i = 0; i < numStartingListeners; ++i) + success = success && test.getListener (i).getNumCalls() == 1; + + // Listeners added during the iteration must not be called in that iteration + for (int i = numStartingListeners; i < test.getNumListeners(); ++i) + success = success && test.getListener (i).getNumCalls() == 0; + + expect (success); + } + + beginTest ("All non-removed listeners should be called - nested ListenerList::call()"); + { + TestObject test; + + for (int i = 0; i < 20; ++i) + { + test.addListener ([i, &test] + { + const auto callLevel = test.getCallLevel(); + + if (i == 6 && callLevel == 1) + { + test.callListeners(); + } + + if (i == 5) + { + if (callLevel == 1) + test.removeListener (4); + else if (callLevel == 2) + test.removeListener (6); + } + }); + } + + test.callListeners(); + expect (test.wereAllNonRemovedListenersCalled (2)); + } + + beginTest ("All non-removed listeners should be called - random ListenerList::call()"); + { + const auto numListeners = 20; + auto random = getRandom(); + + for (int run = 0; run < 10; ++run) + { + TestObject test; + auto numCalls = 0; + + auto listenersToRemove = chooseUnique (random, numListeners, numListeners / 2); + + for (int i = 0; i < numListeners; ++i) + { + // Capturing numListeners is a warning on MacOS, not capturing it is an error on Windows + test.addListener ([&] + { + const auto callLevel = test.getCallLevel(); + + if (callLevel < 4 && random.nextFloat() < 0.05f) + { + ++numCalls; + test.callListeners(); + } + + if (random.nextFloat() < 0.5f) + { + const auto listenerToRemove = random.nextInt ({ 0, numListeners }); + + if (listenersToRemove.erase (listenerToRemove) > 0) + test.removeListener (listenerToRemove); + } + }); + } + + while (listenersToRemove.size() > 0) + { + test.callListeners(); + ++numCalls; + } + + expect (test.wereAllNonRemovedListenersCalled (numCalls)); + } + } + } + +private: + static std::set chooseUnique (Random& random, int max, int numChosen) + { + std::set result; + + while ((int) result.size() < numChosen) + result.insert (random.nextInt ({ 0, max })); + + return result; + } +}; + +static ListenerListTests listenerListTests; + +#endif + +} // namespace juce diff --git a/modules/juce_core/containers/juce_ListenerList.h b/modules/juce_core/containers/juce_ListenerList.h index 665e5c08a7..595e37793d 100644 --- a/modules/juce_core/containers/juce_ListenerList.h +++ b/modules/juce_core/containers/juce_ListenerList.h @@ -47,10 +47,11 @@ namespace juce listeners.call ([] (MyListenerType& l) { l.myCallbackMethod (1234, true); }); @endcode - If you add or remove listeners from the list during one of the callbacks - i.e. while - it's in the middle of iterating the listeners, then it's guaranteed that no listeners - will be mistakenly called after they've been removed, but it may mean that some of the - listeners could be called more than once, or not at all, depending on the list's order. + It is guaranteed that every Listener is called during an iteration if it's inside the + ListenerList before the iteration starts and isn't removed until its end. This guarantee + holds even if some Listeners are removed or new ones are added during the iteration. + + Listeners added during an iteration are guaranteed to be not called in that iteration. Sometimes, there's a chance that invoking one of the callbacks might result in the list itself being deleted while it's still iterating - to survive this situation, you can @@ -73,7 +74,13 @@ public: ListenerList() = default; /** Destructor. */ - ~ListenerList() = default; + ~ListenerList() + { + WrappedIterator::forEach (activeIterators, [&] (auto& iter) + { + iter.invalidate(); + }); + } //============================================================================== /** Adds a listener to the list. @@ -95,7 +102,16 @@ public: void remove (ListenerClass* listenerToRemove) { jassert (listenerToRemove != nullptr); // Listeners can't be null pointers! - listeners.removeFirstMatchingValue (listenerToRemove); + + typename ArrayType::ScopedLockType lock (listeners.getLock()); + + const auto index = listeners.removeFirstMatchingValue (listenerToRemove); + + WrappedIterator::forEach (activeIterators, [&] (auto& iter) + { + if (0 <= index && index < iter.get().index) + --iter.get().index; + }); } /** Returns the number of registered listeners. */ @@ -120,8 +136,8 @@ public: { typename ArrayType::ScopedLockType lock (listeners.getLock()); - for (Iterator iter (*this); iter.next();) - callback (*iter.getListener()); + for (WrappedIterator iter (*this, activeIterators); iter.get().next();) + callback (*iter.get().getListener()); } /** Calls a member function with 1 parameter, on all but the specified listener in the list. @@ -132,9 +148,9 @@ public: { typename ArrayType::ScopedLockType lock (listeners.getLock()); - for (Iterator iter (*this); iter.next();) + for (WrappedIterator iter (*this, activeIterators); iter.get().next();) { - auto* l = iter.getListener(); + auto* l = iter.get().getListener(); if (l != listenerToExclude) callback (*l); @@ -149,8 +165,10 @@ public: { typename ArrayType::ScopedLockType lock (listeners.getLock()); - for (Iterator iter (*this); iter.next (bailOutChecker);) - callback (*iter.getListener()); + for (WrappedIterator iter (*this, activeIterators); iter.get().next (bailOutChecker);) + { + callback (*iter.get().getListener()); + } } /** Calls a member function, with 1 parameter, on all but the specified listener in the list @@ -164,9 +182,9 @@ public: { typename ArrayType::ScopedLockType lock (listeners.getLock()); - for (Iterator iter (*this); iter.next (bailOutChecker);) + for (WrappedIterator iter (*this, activeIterators); iter.get().next (bailOutChecker);) { - auto* l = iter.getListener(); + auto* l = iter.get().getListener(); if (l != listenerToExclude) callback (*l); @@ -187,15 +205,12 @@ public: //============================================================================== /** Iterates the listeners in a ListenerList. */ - template struct Iterator { - Iterator (const ListType& listToIterate) noexcept + explicit Iterator (const ListenerList& listToIterate) noexcept : list (listToIterate), index (listToIterate.size()) {} - ~Iterator() = default; - //============================================================================== bool next() noexcept { @@ -211,21 +226,23 @@ public: return index >= 0; } + template bool next (const BailOutCheckerType& bailOutChecker) noexcept { return (! bailOutChecker.shouldBailOut()) && next(); } - typename ListType::ListenerType* getListener() const noexcept + ListenerClass* getListener() const noexcept { return list.getListeners().getUnchecked (index); } - //============================================================================== private: - const ListType& list; + const ListenerList& list; int index; + friend ListenerList; + JUCE_DECLARE_NON_COPYABLE (Iterator) }; @@ -260,7 +277,7 @@ public: { typename ArrayType::ScopedLockType lock (listeners.getLock()); - for (Iterator iter (*this); iter.next();) + for (Iterator iter (*this); iter.next();) (iter.getListener()->*callbackFunction) (static_cast::type> (args)...); } @@ -271,7 +288,7 @@ public: { typename ArrayType::ScopedLockType lock (listeners.getLock()); - for (Iterator iter (*this); iter.next();) + for (Iterator iter (*this); iter.next();) if (iter.getListener() != listenerToExclude) (iter.getListener()->*callbackFunction) (static_cast::type> (args)...); } @@ -283,7 +300,7 @@ public: { typename ArrayType::ScopedLockType lock (listeners.getLock()); - for (Iterator iter (*this); iter.next (bailOutChecker);) + for (Iterator iter (*this); iter.next (bailOutChecker);) (iter.getListener()->*callbackFunction) (static_cast::type> (args)...); } @@ -295,15 +312,49 @@ public: { typename ArrayType::ScopedLockType lock (listeners.getLock()); - for (Iterator iter (*this); iter.next (bailOutChecker);) + for (Iterator iter (*this); iter.next (bailOutChecker);) if (iter.getListener() != listenerToExclude) (iter.getListener()->*callbackFunction) (static_cast::type> (args)...); } #endif private: + class WrappedIterator + { + public: + WrappedIterator (const ListenerList& listToIterate, WrappedIterator*& listHeadIn) + : it (listToIterate), listHead (listHeadIn), next (listHead) + { + listHead = this; + } + + ~WrappedIterator() + { + if (valid) + listHead = next; + } + + auto& get() noexcept { return it; } + + template + static void forEach (WrappedIterator* wrapped, Callback&& cb) + { + for (auto* p = wrapped; p != nullptr; p = p->next) + cb (*p); + } + + void invalidate() noexcept { valid = false; } + + private: + Iterator it; + WrappedIterator*& listHead; + WrappedIterator* next = nullptr; + bool valid = true; + }; + //============================================================================== ArrayType listeners; + WrappedIterator* activeIterators = nullptr; JUCE_DECLARE_NON_COPYABLE (ListenerList) }; diff --git a/modules/juce_core/juce_core.cpp b/modules/juce_core/juce_core.cpp index aa1f72ff62..e0d1187d3a 100644 --- a/modules/juce_core/juce_core.cpp +++ b/modules/juce_core/juce_core.cpp @@ -128,6 +128,7 @@ //============================================================================== #include "containers/juce_AbstractFifo.cpp" #include "containers/juce_ArrayBase.cpp" +#include "containers/juce_ListenerList.cpp" #include "containers/juce_NamedValueSet.cpp" #include "containers/juce_OwnedArray.cpp" #include "containers/juce_PropertySet.cpp"