I have finally tracked-down an serious ListBox/Viewport bug that has been bugging me for a few months. I thought I should share my findings, and my candidate solution.
Configuration:
create a Component with an embedded ListBox, which doesn’t take-up the full screen
(I’m not sure if this is significant or not)
make sure the ListBox supports drag scrolling
drag around, it works well.
The problem:
mobile (e.g. iOS Simulator): drag around, but do quickly, and be sure to sometimes release the mouse outside of the ListBox onto another component.
desktop: flicking the mouse wheel around over the listbox (to scroll it) and combine with dragging, in quite quick succession.
if you’re unlucky, the list box no longer scrolls by dragging; you have to use the scrollbar; not good
My candidate solution:
I have modified juce_Viewport.cpp, to add the following:
I’ve put together a quick test app to try and reproduce this but I can’t get it to happen on either macOS or iOS (simulator or on a real device). Are you saying that the Viewport stops responding to mouse events completely when this happens? And is your suggestion to put that mouseExit code into the Viewport::DragToScrollListener?
Yes, that is all that was required for the solution to work.
I find it really easy to trigger the problem - as do my Beta users!
I use ListBoxes in many different contexts and “screens” in Wotja, and it can happen for any one of them, so I’m pretty confident that it isn’t an app implementation issue (and most likely a really subtle timing / touch event bug).
So, there must be some other underlying reason, if you’ve been unable to reproduce.
Question - are you testing with a Trackpad, or with a mouse? I always use my MacBook Pro’s Trackpad for development - I wonder if that might be a factor.
I really hope you apply my suggested change however, as whatever happens, I’m otherwise going to have to patch my copy of Juce!
I was using the trackpad when testing. To be sure that’s it’s not an app-specific issue, can you reproduce it in a bare-bones JUCE app? If you can, feel free to send it over and I’ll take another look.
bool sGetViewportForComponentInListIsScrollingOnDrag(juce::Component *component) {
auto theParent = component->getParentComponent();
while (theParent != nullptr) {
auto test = dynamic_cast<juce::ListBox*>(theParent);
if (test != nullptr) {
auto viewPort = test->getViewport();
if (viewPort != nullptr) {
return viewPort->isCurrentlyScrollingOnDrag();
}
return false;
}
theParent = theParent->getParentComponent();
}
return false;
}
Modified the juce::ToggleButton code such that it uses the above method (otherwise, they’re toggled whenever they’ve been used to drag a ListBox)
Modified the juce Button handlers to use the above method (so they’re not triggered when releasing while dragging is happening)
Modified my own custome control Components to use the above method
Hoping you can put this, or a variant, in the code base at some point.
It would be much easier for me (and, I suspect, most others…) if this were in the main code base; together with the other ViewPort change I supplied you a few weeks back.
In the meantime, my copy of Juce is becoming quite patched
I appreciate the fix but I still can’t reproduce the original issue on iOS or macOS. I’ve put together a really simple test app - can you try it out and see if you can hit the bug?
/*******************************************************************************
The block below describes the properties of this PIP. A PIP is a short snippet
of code that can be read by the Projucer and used to generate a JUCE project.
BEGIN_JUCE_PIP_METADATA
name: ListBoxViewportTest
dependencies: juce_core, juce_data_structures, juce_events, juce_graphics, juce_gui_basics
exporters: xcode_iphone, xcode_mac
type: Component
mainClass: MainComponent
END_JUCE_PIP_METADATA
*******************************************************************************/
#pragma once
//==============================================================================
class MainComponent : public Component
{
public:
//==============================================================================
MainComponent()
{
addAndMakeVisible (innerComp);
for (int i = 0; i < 4; ++i)
addAndMakeVisible (buttons.add (new TextButton ("Button" + String (i))));
setOpaque (true);
setSize (600, 400);
}
~MainComponent()
{
}
//==============================================================================
void paint (Graphics& g) override
{
g.fillAll (getLookAndFeel().findColour (ResizableWindow::backgroundColourId));
}
void resized() override
{
auto bounds = getLocalBounds().reduced (5);
buttons[0]->setBounds (bounds.removeFromTop (35).reduced (5));
buttons[1]->setBounds (bounds.removeFromBottom (35).reduced (5));
buttons[2]->setBounds (bounds.removeFromLeft (35).reduced (5));
buttons[3]->setBounds (bounds.removeFromRight (35).reduced (5));
innerComp.setBounds (bounds);
}
private:
//==============================================================================
struct InnerComponent : public Component,
public ListBoxModel
{
InnerComponent()
{
setOpaque (true);
list.setModel (this);
addAndMakeVisible (list);
}
void paint (Graphics& g) override
{
g.fillAll (Colours::lightgrey);
}
void resized() override
{
list.setBounds (getLocalBounds());
}
//==============================================================================
int getNumRows() override { return 5000; }
void paintListBoxItem (int rowNumber, Graphics& g, int, int, bool) override { g.fillAll (colours[rowNumber % colours.size()]); }
//==============================================================================
ListBox list;
Array<Colour> colours { Colours::darkgrey, Colours::blue, Colours::darkred, Colours::darkgreen, Colours::darkviolet };
};
InnerComponent innerComp;
OwnedArray<TextButton> buttons;
//==============================================================================
JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (MainComponent)
};
Ah ok, I think I see the problem now. In your refreshComponentForRow() method you are deleting the existing row component and re-creating it which means that it will not receive mouse exit or mouse up events when you let go.
If you are going to use refreshComponentForRow() a better method is to have an array of “holder” components that is as large as the amount of visible rows where the holders contain an inner component that is the content of the row. Then in the refresh method, you just update the component that is displayed in the holder instead of deleting the object.
This sample code should hopefully explain it a bit better:
/*******************************************************************************
The block below describes the properties of this PIP. A PIP is a short snippet
of code that can be read by the Projucer and used to generate a JUCE project.
BEGIN_JUCE_PIP_METADATA
name: ListBoxViewportTest
dependencies: juce_core, juce_data_structures, juce_events, juce_graphics, juce_gui_basics
exporters: xcode_iphone, xcode_mac
type: Component
mainClass: MainComponent
END_JUCE_PIP_METADATA
*******************************************************************************/
#pragma once
//==============================================================================
class MainComponent : public Component
{
public:
//==============================================================================
MainComponent()
{
addAndMakeVisible (innerComp);
for (int i = 0; i < 4; ++i)
addAndMakeVisible (buttons.add (new TextButton ("Button" + String (i))));
setOpaque (true);
setSize (600, 400);
Desktop::getInstance().addGlobalMouseListener (this);
}
~MainComponent()
{
}
//==============================================================================
void paint (Graphics& g) override
{
g.fillAll (getLookAndFeel().findColour (ResizableWindow::backgroundColourId));
}
void resized() override
{
auto bounds = getLocalBounds().reduced (5);
buttons[0]->setBounds (bounds.removeFromTop (35).reduced (5));
buttons[1]->setBounds (bounds.removeFromBottom (35).reduced (5));
buttons[2]->setBounds (bounds.removeFromLeft (35).reduced (5));
buttons[3]->setBounds (bounds.removeFromRight (35).reduced (5));
innerComp.setBounds (bounds);
}
void mouseExit (const MouseEvent& e) override
{
}
private:
//==============================================================================
struct InnerComponent : public Component,
public ListBoxModel
{
InnerComponent()
{
setOpaque (true);
list.setModel (this);
addAndMakeVisible (list);
}
Component* refreshComponentForRow (int rowNumber, bool isRowSelected, Component* existingComponentToUpdate) override
{
auto holderIndex = rowNumber % (list.getNumRowsOnScreen() + 5); // add a bit of a buffer to be safe
if (holderIndex >= holders.size()) // need to create a holder component
{
auto* holder = new CustomComponentHolder();
holder->setNewComponent (colours[rowNumber % colours.size()]);
holders.add (holder);
return holder;
}
if (auto* custom = dynamic_cast<CustomComponentHolder*> (existingComponentToUpdate)) // update the inner component of the holder
{
custom->setNewComponent (colours[rowNumber % colours.size()]);
return existingComponentToUpdate;
}
if (existingComponentToUpdate != nullptr)
jassertfalse;
return nullptr;
}
struct CustomComponentHolder : public Component
{
CustomComponentHolder()
{
setInterceptsMouseClicks (false, false);
}
void setNewComponent (Colour c)
{
customComponent.reset (new InnerComponent (c));
addAndMakeVisible (customComponent);
resized();
}
void resized() override
{
customComponent->setBounds (getLocalBounds());
}
struct InnerComponent : public Component
{
InnerComponent (Colour c) : colour (c) {}
void paint (Graphics& g) override { g.fillAll (colour); }
Colour colour;
};
ScopedPointer<InnerComponent> customComponent;
};
Array<CustomComponentHolder*> holders;
void paint (Graphics& g) override
{
g.fillAll (Colours::lightgrey);
}
void resized() override
{
list.setBounds (getLocalBounds());
}
//==============================================================================
int getNumRows() override { return 5000; }
void paintListBoxItem (int rowNumber, Graphics& g, int, int, bool) override { g.fillAll (Colours::hotpink); }
//==============================================================================
ListBox list;
Array<Colour> colours { Colours::darkgrey, Colours::blue, Colours::darkred, Colours::darkgreen, Colours::darkviolet };
};
InnerComponent innerComp;
OwnedArray<TextButton> buttons;
//==============================================================================
JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (MainComponent)
};
once the app has added a component to the listbox via refreshComponentForRow, the app must not directly delete it within refreshComponentForRow
the app must re-use a previously returned Component in refreshComponentForRow, if that component had been returned previously when rowNumber is re-requested?
Clearly of course, if the underlying model is re-queried, the above wouldn’t apply.
I’m not clear when the components returned by the client app in calls to refreshComponentForRow get deleted; especially if the client is supposed to maintain its own model of returned Components, form which it must “pick” a previously returned Component when the rowNumber is repeated from a previous call.
The ListBox owns the components that are returned from calls to refreshComponentForRow() and will delete them when they aren’t needed. In the “holder” method from the example that I posted then only as many Components as there are visible rows will be created, and their lifetime will be determined by the lifetime of the ListBox.
If the existingComponentToUpdate is non-null, it will be a pointer to a component previously created by this method. In this case, the method must either update it to make sure it’s correctly representing the given row (which may be different from the one that the component was created for), or it can delete this component and return a new one.
Sorry - I’m coming to this half way through - but are these instructions, from the docs, wrong?
In this case, the method must either update it to make sure it’s correctly representing the given row
That’s what the code I posted above does, it’s updating the existingComponentToUpdate to represent the given row (by doing custom->setNewComponent (colours[rowNumber % colours.size()]);) or creating one if it doesn’t exist already. This example just avoids deleting the component by re-using a fixed number of “holder” components to solve the drag-to-scroll issues that @impete was having.
Actually, just to re-query; I think that you’re saying for drag-to-scroll to work, the application must not delete previously returned components; it must either create a new one and return it; or return a previously created one which we know applied for the previous row.
However, that really is at odds with the documentation, which if I’m understanding correctly gives the wrong advice for drag-and-scroll to work properly.
I would suggest that this area is worth a re-think, as it is confusing - and if an app fails with drag-to-scroll for this reason, it takes a lot of time all round to track-down and resolve.
I don’t suppose it matters , but I think that my existing implementation is quite logical, in that it is consistent with your published documentation and it does work with drag-to-scroll provided Juce has had applied the previously supplied (very minor) patch for the mouse exit method.
But the patch that you provided doesn’t solve the issue. The mouse exit events will never be sent to the Viewport’s drag-to-scroll listener as the Component that is applies to has been deleted.
