ListBox/Viewport bug (description, candidate solution)


#1

Hi!

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 :slight_smile:

My candidate solution:

  • I have modified juce_Viewport.cpp, to add the following:
  void mouseExit (const MouseEvent& e) override
  {
    if (doesMouseEventComponentBlockViewportDrag (e.eventComponent))
      isViewportDragBlocked = false;
    
    if (numTouches == 0) {
      // printf ("MPC mouseExit - numTouches already zero\n");
      return;
    }

    printf ("MPC mouseExit - WARNING!!! - numTouches=%d!\n", numTouches);
    
    if (--numTouches <= 0)
    {
      printf ("MPC mouseExit - WARNING!!! - numTouches now=%d!\n", numTouches);

      offsetX.endDrag();
      offsetY.endDrag();
      isDragging = false;
      numTouches = 0;
    }
  }

Summary:

  • can you please find a way to reproduce this, and apply a fix to the repo?
  • that will remove the requirement for me to locally patch my Juce source code :slight_smile:

Pete


#2

See also https://github.com/WeAreROLI/JUCE/issues/366


#3

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?


#4

Hi Ed,

Thanks very much for investigating so promptly.

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!

Best wishes,

Pete


#5

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.


#6

Hi Ed, makes me wonder if this is something you might have fixed in Juce, therefore - I’m using the version from 17/01/2018 … Pete


#7

Yep, always good to test out bugs with the latest tip of develop first as we may have fixed them already.


#8

Hi Ed,

My solution was as follows.

  1. Created a new method as follows:

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;
}
  1. 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)

  2. Modified the juce Button handlers to use the above method (so they’re not triggered when releasing while dragging is happening)

  3. 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 :slight_smile:

Pete


#9

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)
};

#10

Hi Ed!

Many thanks for that, and apologies for the delay in reply; took me a while as I had to get this Wotja release out the door.

Anyhow I took what you had, and it worked.

I made a few changes, and it is now easy to reproduce! :slight_smile:

The issue is, as you will see, in virtual Component* refreshComponentForRow

I suspect that reusing components causes something in their event handling model not to match the expectations of the ListBox.

/*******************************************************************************
 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
  {
    std::vector<Component*> mComponentForRow;
    
    InnerComponent()
    {
      setOpaque (true);
      
      list.setModel (this);
      addAndMakeVisible (list);
    }
    
    virtual Component* refreshComponentForRow (int rowNumber, bool isRowSelected,
                                               Component* existingComponentToUpdate) override {
      if (rowNumber < mComponentForRow.size()) {
        auto test = mComponentForRow[rowNumber];
        if (test == existingComponentToUpdate) {
          return test;
        }
      }
      
      delete existingComponentToUpdate;
      
      Component *newComponent = new Component();
      
      while (rowNumber >= mComponentForRow.size()) {
        mComponentForRow.push_back(nullptr);
      }
      mComponentForRow[rowNumber] = newComponent;
      
      return newComponent;
    }
    
    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)
};

Anyhow, I hope this’ll now make it easy for you to reproduce, and from that create a fix.

Many thanks for your patience with this!

Best wishes,

Pete


#11

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)
};

#12

Hi Ed,

OK, thanks - I suspected as much.

So, to be clear, for scrolling to work safely:

  1. once the app has added a component to the listbox via refreshComponentForRow, the app must not directly delete it within refreshComponentForRow
  2. 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.

Best wishes,

Pete


#13

Pretty much, yes. The example that I posted above should give you a good starting point for how to implement this.


#14

Hi Ed,

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.

Thanks in advance,

Pete


#15

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.


#16

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?


#17

Thanks Ed!

Jim - it certainly seems to be the case!

Pete


#18

No those are still correct.

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.


#19

Hi Ed,

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 :slight_smile: , 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.

Anyhow, just my 2 cents.

Best wishes,

Pete


#20

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.