[composer] Fix pages are added but never removed with multiframe resi…

…zing
qgis · Sep 29, 2014 · 8c502c1 · 8c502c1
1 parent 40465ca
commit 8c502c1
Show file tree

Hide file tree

Showing 10 changed files with 153 additions and 8 deletions.
diff --git a/python/core/composer/qgscomposeritem.sip b/python/core/composer/qgscomposeritem.sip
@@ -209,7 +209,7 @@ class QgsComposerItem : QgsComposerObject, QGraphicsRectItem
     virtual void zoomContent( const double factor, const QPointF point, const ZoomMode mode = QgsComposerItem::Zoom );
 
     /**Gets the page the item is currently on.
-     * @returns page number for item
+     * @returns page number for item, beginning on page 1
      * @see pagePos
      * @see updatePagePos
      * @note this method was added in version 2.4

diff --git a/python/core/composer/qgscomposermultiframe.sip b/python/core/composer/qgscomposermultiframe.sip
@@ -95,10 +95,11 @@ class QgsComposerMultiFrame: QgsComposerObject
     /**Removes a frame from the multiframe. This method automatically removes the frame from the
      * composition.
      * @param i index of frame to remove
+     * @param removeEmptyPages set to true to remove pages which are empty after the frame is removed
      * @see addFrame
      * @see deleteFrames
     */
-    void removeFrame( int i );
+    void removeFrame( int i, const bool removeEmptyPages = false );
 
     /**Removes and deletes all child frames.
      * @see removeFrame

diff --git a/python/core/composer/qgscomposition.sip b/python/core/composer/qgscomposition.sip
@@ -95,12 +95,24 @@ class QgsComposition : QGraphicsScene
      * @see setNumPages
     */
     int numPages() const;
+
+    /**Returns whether a page is empty, ie, it contains no items except for the background
+     * paper item.
+     * @param page page number, starting with 1
+     * @returns true if page is empty
+     * @note added in QGIS 2.5
+     * @see numPages
+     * @see setNumPages
+     * @see shouldExportPage
+    */
+    bool pageIsEmpty( const int page ) const;
 
     /**Returns whether a specified page number should be included in exports of the composition.
      * @param page page number, starting with 1
      * @returns true if page should be exported
      * @note added in QGIS 2.5
      * @see numPages
+     * @see pageIsEmpty
     */
     bool shouldExportPage( const int page ) const;
 

diff --git a/src/core/composer/qgscomposeritem.h b/src/core/composer/qgscomposeritem.h
@@ -167,7 +167,7 @@ class CORE_EXPORT QgsComposerItem: public QgsComposerObject, public QGraphicsRec
     virtual void zoomContent( const double factor, const QPointF point, const ZoomMode mode = QgsComposerItem::Zoom ) { Q_UNUSED( factor ); Q_UNUSED( point ); Q_UNUSED( mode ); }
 
     /**Gets the page the item is currently on.
-     * @returns page number for item
+     * @returns page number for item, beginning on page 1
      * @see pagePos
      * @see updatePagePos
      * @note this method was added in version 2.4

diff --git a/src/core/composer/qgscomposermultiframe.cpp b/src/core/composer/qgscomposermultiframe.cpp
@@ -91,9 +91,13 @@ void QgsComposerMultiFrame::recalculateFrameSizes()
     {
       if ( mResizeMode == RepeatUntilFinished || mResizeMode == ExtendToNextPage ) //remove unneeded frames in extent mode
       {
+        bool removingPages = true;
         for ( int j = mFrameItems.size(); j > i; --j )
         {
-          removeFrame( j - 1 );
+          int numPagesBefore = mComposition->numPages();
+          removeFrame( j - 1, removingPages );
+          //if removing the frame didn't also remove the page, then stop removing pages
+          removingPages = removingPages && ( mComposition->numPages() < numPagesBefore );
         }
         return;
       }
@@ -243,8 +247,8 @@ void QgsComposerMultiFrame::handleFrameRemoval( QgsComposerItem* item )
       //otherwise the frame may not actually be removed, leading to confusing ui behaviour
       mResizeMode = QgsComposerMultiFrame::UseExistingFrames;
       emit changed();
+      recalculateFrameSizes();
     }
-    recalculateFrameSizes();
   }
 }
 
@@ -289,7 +293,7 @@ void QgsComposerMultiFrame::handlePageChange()
   update();
 }
 
-void QgsComposerMultiFrame::removeFrame( int i )
+void QgsComposerMultiFrame::removeFrame( int i, const bool removeEmptyPages )
 {
   if ( i >= mFrameItems.count() )
   {
@@ -300,7 +304,13 @@ void QgsComposerMultiFrame::removeFrame( int i )
   if ( mComposition )
   {
     mIsRecalculatingSize = true;
+    int pageNumber = frameItem->page();
     mComposition->removeComposerItem( frameItem );
+    //if frame was the only item on the page, remove the page
+    if ( removeEmptyPages && mComposition->pageIsEmpty( pageNumber ) )
+    {
+      mComposition->setNumPages( mComposition->numPages() - 1 );
+    }
     mIsRecalculatingSize = false;
   }
   mFrameItems.removeAt( i );

diff --git a/src/core/composer/qgscomposermultiframe.h b/src/core/composer/qgscomposermultiframe.h
@@ -123,10 +123,11 @@ class CORE_EXPORT QgsComposerMultiFrame: public QgsComposerObject
     /**Removes a frame from the multiframe. This method automatically removes the frame from the
      * composition.
      * @param i index of frame to remove
+     * @param removeEmptyPages set to true to remove pages which are empty after the frame is removed
      * @see addFrame
      * @see deleteFrames
     */
-    void removeFrame( int i );
+    void removeFrame( int i, const bool removeEmptyPages = false );
 
     /**Removes and deletes all child frames.
      * @see removeFrame
@@ -295,7 +296,6 @@ class CORE_EXPORT QgsComposerMultiFrame: public QgsComposerObject
     QgsComposerMultiFrame(); //forbidden
 
     bool mIsRecalculatingSize;
-
 };
 
 #endif // QGSCOMPOSERMULTIFRAME_H
diff --git a/src/core/composer/qgscomposition.cpp b/src/core/composer/qgscomposition.cpp
@@ -416,6 +416,29 @@ int QgsComposition::numPages() const
   return mPages.size();
 }
 
+bool QgsComposition::pageIsEmpty( const int page ) const
+{
+  //get all items on page
+  QList<QgsComposerItem*> items;
+  //composerItemsOnPage uses 0-based page numbering
+  composerItemsOnPage( items, page - 1 );
+
+  //loop through and check for non-paper items
+  QList<QgsComposerItem*>::const_iterator itemIt = items.constBegin();
+  for ( ; itemIt != items.constEnd(); ++itemIt )
+  {
+    //is item a paper item?
+    QgsPaperItem* paper = dynamic_cast<QgsPaperItem*>( *itemIt );
+    if ( !paper )
+    {
+      //item is not a paper item, so we have other items on the page
+      return false;
+    }
+  }
+  //no non-paper items
+  return true;
+}
+
 bool QgsComposition::shouldExportPage( const int page ) const
 {
   if ( page > numPages() || page < 1 )

diff --git a/src/core/composer/qgscomposition.h b/src/core/composer/qgscomposition.h
@@ -158,11 +158,23 @@ class CORE_EXPORT QgsComposition : public QGraphicsScene
     */
     int numPages() const;
 
+    /**Returns whether a page is empty, ie, it contains no items except for the background
+     * paper item.
+     * @param page page number, starting with 1
+     * @returns true if page is empty
+     * @note added in QGIS 2.5
+     * @see numPages
+     * @see setNumPages
+     * @see shouldExportPage
+    */
+    bool pageIsEmpty( const int page ) const;
+
     /**Returns whether a specified page number should be included in exports of the composition.
      * @param page page number, starting with 1
      * @returns true if page should be exported
      * @note added in QGIS 2.5
      * @see numPages
+     * @see pageIsEmpty
     */
     bool shouldExportPage( const int page ) const;
 

diff --git a/tests/src/core/testqgscomposermultiframe.cpp b/tests/src/core/testqgscomposermultiframe.cpp
@@ -17,6 +17,7 @@
 
 #include "qgscomposerhtml.h"
 #include "qgscomposerframe.h"
+#include "qgscomposerlabel.h"
 #include "qgscomposition.h"
 #include "qgscompositionchecker.h"
 #include <QObject>
@@ -31,6 +32,7 @@ class TestQgsComposerMultiFrame: public QObject
     void init();// will be called before each testfunction is executed.
     void cleanup();// will be called after every testfunction.
     void frameIsEmpty(); //test if frame is empty works
+    void addRemovePage(); //test if page is added and removed for RepeatUntilFinished mode
 
   private:
     QgsComposition* mComposition;
@@ -103,5 +105,60 @@ void TestQgsComposerMultiFrame::frameIsEmpty()
   delete htmlItem;
 }
 
+void TestQgsComposerMultiFrame::addRemovePage()
+{
+  QgsComposerHtml* htmlItem = new QgsComposerHtml( mComposition, false );
+  QgsComposerFrame* frame1 = new QgsComposerFrame( mComposition, htmlItem, 0, 0, 100, 200 );
+  htmlItem->addFrame( frame1 );
+  htmlItem->setContentMode( QgsComposerHtml::ManualHtml );
+  htmlItem->setResizeMode( QgsComposerMultiFrame::RepeatUntilFinished );
+
+  //short content, so should fit in one frame
+  htmlItem->setHtml( QString( "<p><i>Test manual <b>html</b></i></p>" ) );
+  htmlItem->loadHtml();
+
+  //should be one page
+  QCOMPARE( htmlItem->frameCount(), 1 );
+  QCOMPARE( mComposition->numPages(), 1 );
+
+  //long content, so we require 3 frames
+  htmlItem->setHtml( QString( "<p style=\"height: 2000px\"><i>Test manual <b>html</b></i></p>" ) );
+  htmlItem->loadHtml();
+
+  QCOMPARE( htmlItem->frameCount(), 3 );
+  QCOMPARE( mComposition->numPages(), 3 );
+
+  //..and back again..
+  htmlItem->setHtml( QString( "<p><i>Test manual <b>html</b></i></p>" ) );
+  htmlItem->loadHtml();
+
+  QCOMPARE( htmlItem->frameCount(), 1 );
+  QCOMPARE( mComposition->numPages(), 1 );
+
+
+  //get a bit more complicated - add another item to page 3
+  QgsComposerLabel* label1 = new QgsComposerLabel( mComposition );
+  mComposition->addComposerLabel( label1 );
+  label1->setItemPosition( 10, 10, 50, 50, QgsComposerItem::UpperLeft, false, 3 );
+
+  //long content, so we require 4 pages
+  htmlItem->setHtml( QString( "<p style=\"height: 3000px\"><i>Test manual <b>html</b></i></p>" ) );
+  htmlItem->loadHtml();
+
+  QCOMPARE( htmlItem->frameCount(), 4 );
+  QCOMPARE( mComposition->numPages(), 4 );
+
+  //..and back again. Since there's an item on page 3, only page 4 should be removed
+  htmlItem->setHtml( QString( "<p><i>Test manual <b>html</b></i></p>" ) );
+  htmlItem->loadHtml();
+
+  QCOMPARE( htmlItem->frameCount(), 1 );
+  QCOMPARE( mComposition->numPages(), 3 );
+
+  mComposition->removeMultiFrame( htmlItem );
+  delete htmlItem;
+}
+
+
 QTEST_MAIN( TestQgsComposerMultiFrame )
 #include "moc_testqgscomposermultiframe.cxx"
diff --git a/tests/src/core/testqgscomposition.cpp b/tests/src/core/testqgscomposition.cpp
@@ -39,6 +39,7 @@ class TestQgsComposition: public QObject
 
     void itemsOnPage(); //test fetching matching items on a set page
     void shouldExportPage(); //test the shouldExportPage method
+    void pageIsEmpty(); //test the pageIsEmpty method
 
   private:
     QgsComposition* mComposition;
@@ -211,6 +212,35 @@ void TestQgsComposition::shouldExportPage()
   delete htmlItem;
 }
 
+void TestQgsComposition::pageIsEmpty()
+{
+  //add some items to the composition
+  QgsComposerLabel* label1 = new QgsComposerLabel( mComposition );
+  mComposition->addComposerLabel( label1 );
+  label1->setItemPosition( 10, 10, 50, 50, QgsComposerItem::UpperLeft, false, 1 );
+  QgsComposerLabel* label2 = new QgsComposerLabel( mComposition );
+  mComposition->addComposerLabel( label2 );
+  label2->setItemPosition( 10, 10, 50, 50, QgsComposerItem::UpperLeft, false, 1 );
+  QgsComposerLabel* label3 = new QgsComposerLabel( mComposition );
+  mComposition->addComposerLabel( label3 );
+  label3->setItemPosition( 10, 10, 50, 50, QgsComposerItem::UpperLeft, false, 3 );
+
+  //only page 2 should be empty
+  QCOMPARE( mComposition->pageIsEmpty( 1 ), false );
+  QCOMPARE( mComposition->pageIsEmpty( 2 ), true );
+  QCOMPARE( mComposition->pageIsEmpty( 3 ), false );
+
+  //remove the items
+  mComposition->removeComposerItem( label1 );
+  mComposition->removeComposerItem( label2 );
+  mComposition->removeComposerItem( label3 );
+
+  //expect everything to be empty now
+  QCOMPARE( mComposition->pageIsEmpty( 1 ), true );
+  QCOMPARE( mComposition->pageIsEmpty( 2 ), true );
+  QCOMPARE( mComposition->pageIsEmpty( 3 ), true );
+}
+
 QTEST_MAIN( TestQgsComposition )
 #include "moc_testqgscomposition.cxx"