Merge pull request #3477 from nyalldawson/legend_col_align

Fixes to multicolumn legends

src/core/qgslegendrenderer.cpp

@@ -265,49 +265,49 @@ void QgsLegendRenderer::setColumns( QList<Atom>& atomList )
 
   // Divide atoms to columns
   double totalHeight = 0;
-  // bool first = true;
   qreal maxAtomHeight = 0;
   Q_FOREACH ( const Atom& atom, atomList )
   {
-    //if ( !first )
-    //{
     totalHeight += spaceAboveAtom( atom );
-    //}
     totalHeight += atom.size.height();
     maxAtomHeight = qMax( atom.size.height(), maxAtomHeight );
-    // first  = false;
   }
 
   // We know height of each atom and we have to split them into columns
   // minimizing max column height. It is sort of bin packing problem, NP-hard.
   // We are using simple heuristic, brute fore appeared to be to slow,
   // the number of combinations is N = n!/(k!*(n-k)!) where n = atomsCount-1
   // and k = columnsCount-1
-
-  double avgColumnHeight = totalHeight / mSettings.columnCount();
+  double maxColumnHeight = 0;
   int currentColumn = 0;
   int currentColumnAtomCount = 0; // number of atoms in current column
   double currentColumnHeight = 0;
-  double maxColumnHeight = 0;
   double closedColumnsHeight = 0;
-  // first = true; // first in column
+
   for ( int i = 0; i < atomList.size(); i++ )
   {
-    Atom atom = atomList[i];
+    // Recalc average height for remaining columns including current
+    double avgColumnHeight = ( totalHeight - closedColumnsHeight ) / ( mSettings.columnCount() - currentColumn );
+
+    Atom atom = atomList.at( i );
     double currentHeight = currentColumnHeight;
-    //if ( !first )
-    //{
-    currentHeight += spaceAboveAtom( atom );
-    //}
+    if ( currentColumnAtomCount > 0 )
+      currentHeight += spaceAboveAtom( atom );
     currentHeight += atom.size.height();
 
-    // Recalc average height for remaining columns including current
-    avgColumnHeight = ( totalHeight - closedColumnsHeight ) / ( mSettings.columnCount() - currentColumn );
-    if (( currentHeight - avgColumnHeight ) > atom.size.height() / 2 // center of current atom is over average height
-        && currentColumnAtomCount > 0 // do not leave empty column
-        && currentHeight > maxAtomHeight  // no sense to make smaller columns than max atom height
-        && currentHeight > maxColumnHeight  // no sense to make smaller columns than max column already created
-        && currentColumn < mSettings.columnCount() - 1 ) // must not exceed max number of columns
+    bool canCreateNewColumn = ( currentColumnAtomCount > 0 )  // do not leave empty column
+                              && ( currentColumn < mSettings.columnCount() - 1 ); // must not exceed max number of columns
+
+    bool shouldCreateNewColumn = ( currentHeight - avgColumnHeight ) > atom.size.height() / 2  // center of current atom is over average height
+                                 && currentColumnAtomCount > 0 // do not leave empty column
+                                 && currentHeight > maxAtomHeight  // no sense to make smaller columns than max atom height
+                                 && currentHeight > maxColumnHeight; // no sense to make smaller columns than max column already created
+
+    // also should create a new column if the number of items left < number of columns left
+    // in this case we should spread the remaining items out over the remaining columns
+    shouldCreateNewColumn |= ( atomList.size() - i < mSettings.columnCount() - currentColumn );
+
+    if ( canCreateNewColumn && shouldCreateNewColumn )
     {
       // New column
       currentColumn++;
@@ -322,11 +322,9 @@ void QgsLegendRenderer::setColumns( QList<Atom>& atomList )
     atomList[i].column = currentColumn;
     currentColumnAtomCount++;
     maxColumnHeight = qMax( currentColumnHeight, maxColumnHeight );
-
-    // first  = false;
   }
 
-  // Alling labels of symbols for each layr/column to the same labelXOffset
+  // Align labels of symbols for each layr/column to the same labelXOffset
   QMap<QString, qreal> maxSymbolWidth;
   for ( int i = 0; i < atomList.size(); i++ )
   {

tests/src/core/testqgslegendrenderer.cpp

@@ -118,6 +118,8 @@ class TestQgsLegendRenderer : public QObject
     void testThreeColumns();
     void testFilterByMap();
     void testFilterByMapSameSymbol();
+    void testColumns_data();
+    void testColumns();
     void testRasterBorder();
     void testFilterByPolygon();
     void testFilterByExpression();
@@ -131,6 +133,7 @@ class TestQgsLegendRenderer : public QObject
     QgsVectorLayer* mVL3; // point
     QgsRasterLayer* mRL;
     QString mReport;
+    bool _testLegendColumns( int itemCount, int columnCount, const QString& testName );
 };
 
 
@@ -459,6 +462,65 @@ void TestQgsLegendRenderer::testFilterByMapSameSymbol()
   QgsMapLayerRegistry::instance()->removeMapLayer( vl4 );
 }
 
+bool TestQgsLegendRenderer::_testLegendColumns( int itemCount, int columnCount, const QString& testName )
+{
+  QgsFillSymbol* sym = new QgsFillSymbol();
+  sym->setColor( Qt::cyan );
+
+  QgsLayerTreeGroup* root = new QgsLayerTreeGroup();
+
+  QList< QgsVectorLayer* > layers;
+  for ( int i = 1; i <= itemCount; ++i )
+  {
+    QgsVectorLayer* vl = new QgsVectorLayer( "Polygon", QString( "Layer %1" ).arg( i ), "memory" );
+    QgsMapLayerRegistry::instance()->addMapLayer( vl );
+    vl->setRenderer( new QgsSingleSymbolRenderer( sym->clone() ) );
+    root->addLayer( vl );
+    layers << vl;
+  }
+  delete sym;
+
+  QgsLayerTreeModel legendModel( root );
+  QgsLegendSettings settings;
+  settings.setColumnCount( columnCount );
+  _setStandardTestFont( settings, "Bold" );
+  _renderLegend( testName, &legendModel, settings );
+  bool result = _verifyImage( testName, mReport );
+
+  Q_FOREACH ( QgsVectorLayer* l, layers )
+  {
+    QgsMapLayerRegistry::instance()->removeMapLayer( l );
+  }
+  return result;
+}
+
+void TestQgsLegendRenderer::testColumns_data()
+{
+  QTest::addColumn<QString>( "testName" );
+  QTest::addColumn<int>( "items" );
+  QTest::addColumn<int>( "columns" );
+
+  QTest::newRow( "2 items, 2 columns" ) << "legend_2_by_2" << 2 << 2;
+  QTest::newRow( "3 items, 2 columns" ) << "legend_3_by_2" << 3 << 2;
+  QTest::newRow( "4 items, 2 columns" ) << "legend_4_by_2" << 4 << 2;
+  QTest::newRow( "5 items, 2 columns" ) << "legend_5_by_2" << 5 << 2;
+  QTest::newRow( "3 items, 3 columns" ) << "legend_3_by_3" << 3 << 3;
+  QTest::newRow( "4 items, 3 columns" ) << "legend_4_by_3" << 4 << 3;
+  QTest::newRow( "5 items, 3 columns" ) << "legend_5_by_3" << 5 << 3;
+  QTest::newRow( "6 items, 3 columns" ) << "legend_6_by_3" << 6 << 3;
+  QTest::newRow( "7 items, 3 columns" ) << "legend_7_by_3" << 7 << 3;
+}
+
+void TestQgsLegendRenderer::testColumns()
+{
+  //test rendering legend with different combinations of columns and items
+
+  QFETCH( QString, testName );
+  QFETCH( int, items );
+  QFETCH( int, columns );
+  QVERIFY( _testLegendColumns( items, columns, testName ) );
+}
+
 void TestQgsLegendRenderer::testRasterBorder()
 {
   QString testName = "legend_raster_border";