Hi,
If a TableListBox is scaled by certain factors (using an AffineTransform::scale(…)) then it tries to use one row past the last row in the model. It doesn't seem to be the case with integer scaling factors, but for instance with scale 1.4 and 5 rows it will happen.
If the model paints this extra row with for example a red background, you will see a row of red pixels below the last row, like this:
--
Roeland
Can you give me some sample code (e.g. that I can paste into the juce demo) so I can reproduce this?
Here is some code for a simple table model:
class OurTableModel : public TableListBoxModel
{
const static int rows = 5;
public:
virtual int getNumRows()
{
return rows;
}
virtual void paintRowBackground (Graphics& g,
int rowNumber,
int width, int height,
bool rowIsSelected)
{
if (rowNumber >= rows)
{
g.fillAll(Colours::red);
}
}
virtual void paintCell (Graphics& g,
int rowNumber,
int columnId,
int width, int height,
bool rowIsSelected)
{
if (rowNumber >= rows)
{
// Argh! We don't have this many rows!
jassertfalse;
}
else if (rowIsSelected)
{
g.fillAll(LookAndFeel::getDefaultLookAndFeel().findColour(TextEditor::highlightColourId));
}
g.drawText("row " + String(rowNumber),
0, 0, width, height,
Justification::centredLeft, false);
}
};
And the main component code:
class MainContentComponent : public Component,
private Slider::Listener
{
public:
//==============================================================================
MainContentComponent();
~MainContentComponent();
void paint (Graphics&);
void resized();
virtual void sliderValueChanged (Slider* slider);
private:
//==============================================================================
JUCE_DECLARE_NON_COPYABLE_WITH_LEAK_DETECTOR (MainContentComponent)
ScopedPointer<TableListBox> listBox;
ScopedPointer<TableListBoxModel> listModel;
ScopedPointer<Slider> scaleSlider;
float scale() const;
};
MainContentComponent::MainContentComponent()
{
scaleSlider = new Slider();
scaleSlider->setRange(.5, 3., .1);
scaleSlider->setValue(1.);
scaleSlider->addListener(this);
addAndMakeVisible(scaleSlider);
listModel = new OurTableModel();
listBox = new TableListBox("", listModel);
listBox->setTransform(AffineTransform::scale(scale(), scale(), 0, 0));
TableHeaderComponent *header = new TableHeaderComponent();
header->addColumn("row", 1, 200);
listBox->setHeader(header);
addAndMakeVisible(listBox);
listBox->updateContent();
setSize (500, 400);
}
MainContentComponent::~MainContentComponent()
{
listBox->setModel(nullptr);
}
void MainContentComponent::paint (Graphics& g)
{
}
void MainContentComponent::resized()
{
Rectangle<int> bounds = getLocalBounds();
scaleSlider->setBounds(bounds.removeFromTop(24));
listBox->setBounds(bounds.transformedBy(AffineTransform::scale(1.f / scale())));
}
void MainContentComponent::sliderValueChanged (Slider* slider)
{
listBox->setTransform(AffineTransform::scale(scale(), scale(), 0, 0));
resized();
}
float MainContentComponent::scale() const
{
return (float) scaleSlider->getValue();
}
I have tested this with regular ListBoxes as well, but I didn't see this bug there.
--
Roeland
So I just saw this issue in a regular ListBox as well while resizing it (increasing the vertical size). That was on Windows 7, without any scaling.
And note that if you have DPI scaling enabled, every table list box will be scaled by default, and you will get paint calls for a row number 1 past the last valid row index.
So the question is: should the JUCE code ensure that the paintCell() method is only called for valid indices (0 to getNumRows() - 1)? Or are you just supposed to start any paintCell() method with "if (rowNumber >= getNumRows()) { return; }"?
--
Roeland
Some people might need to paint the background of other rows, e.g. they might want the whole table to be drawn with horizontal lines on the empty rows as well as the active ones. If it's a problem for your app then yes, you should probably just avoid drawing rows that are out-of-range.
Hmm then I would expect the entire area below the rows to be red. This looks more like a rounding issue to me.
This is what I see happening:
You definitely can't use the paintRowBackground method to paint anything on empty rows, since that area will be outside the parent component of the row components. It looks like those extra row components are not supposed to be painted.
--
Roeland
Yes, you're right about what's going wrong - seems that the software renderer just has a rounding error that's causing the sub-pixel repaint to overlap a bit.
Looking into it, I don't think I should change anything though - I could indeed stop the paintCell methods being called for row numbers that are beyond getNumRows, but that'd mean checking getNumRows many times during a paint, and user-code could be doing anything in that function, so it could get expensive. Since the code was always written with the intention that the paint methods must be able to handle out-of-range values, I think it's best if you just check it in your class. I'll add a bit of extra explanation in the comments for that method to make this clear.
I just ran into this same issue, and it still exists in the latest release. Our list box code was assuming it would only ever be called with valid row indices, and was crashing horribly in this problem case.
And note that if you have DPI scaling enabled, every table list box will be scaled by default, and you will get paint calls for a row number 1 past the last valid row index.
That is exactly the case where the problem was showing up in our case. With high DPI displays becoming more and more prevalent, I would think this issue will become more common.
Note that if you don't have a high DPI display, you can also simulate this w/o changing the overall system scaling by manually tweaking the JUCE native Win function getGlobalDPI() to return a hardcoded value (for example 120.0 for 125% scaling). That function lives here:
modules\juce_gui_basics\native\juce_win32_Windowing.cpp
Looking into it, I don't think I should change anything though - I could indeed stop the paintCell methods being called for row numbers that are beyond getNumRows, but that'd mean checking getNumRows many times during a paint, and user-code could be doing anything in that function, so it could get expensive. Since the code was always written with the intention that the paint methods must be able to handle out-of-range values, I think it's best if you just check it in your class. I'll add a bit of extra explanation in the comments for that method to make this clear.
Did those comments ever happen? Because I sure didn't see anything to warn me to expect this. Perhaps that and even an assert in the debug case would prevent some unnecessary head-scratching for someone else in the future.
I added a comment that says:
Note that the rowNumber value may be greater than the number of rows in your
list, so be careful that you don't assume it's less than getNumRows().
Isn't that what you mean?
Crap. For our production code we're using a slightly older version of JUCE 3 (not the tip), and that comment isn't in that version. I thought the version we were using was more recent than the last comment but I guess not. I see it there in the tip.
Still think that it would be useful to have something beyond comments... maybe the debug build could do the check and print out a log message in the case where it's calling paint for a row out of range. I understand why you don't want to call getNumRows() all the time, but really this just "feels" like a straight-up bug. I mean header comment or not, why should you be getting called to paint a non-existent row?
