Improve the internals of map layer style manager

- style manager is always enabled (client code does not need to distinguish between enabled/disabled state) - layers currently active style is stored only in QgsMapLayer - in style manager it has entry with no data (avoids duplication of data) This also solves issues with visibility presets and styles when some presets do not have stored style
qgis · Jan 9, 2015 · 8b5587f · 8b5587f
1 parent 7c4f3f9
commit 8b5587f
Show file tree

Hide file tree

Showing 9 changed files with 49 additions and 94 deletions.
diff --git a/python/core/qgsmaplayer.sip b/python/core/qgsmaplayer.sip
@@ -370,17 +370,8 @@ class QgsMapLayer : QObject
      */
     QgsMapLayerLegend* legend() const;
 
-    /**
-     * Enable or disable layer's style manager. When disabled (default), the styleManager() will return null pointer.
-     * By enabling the style manager will be created with one default style (same as the layer's active style).
-     * By disabling the style manager all associated styles will be lost (only the layer's active style will stay).
-     * @note added in 2.8
-     */
-    void enableStyleManager( bool enable = true );
-
     /**
      * Get access to the layer's style manager. Style manager allows switching between multiple styles.
-     * If the style manager is not enabled, null pointer will be returned.
      * @note added in 2.8
      */
     QgsMapLayerStyleManager* styleManager() const;

diff --git a/python/core/qgsmaplayerstylemanager.sip b/python/core/qgsmaplayerstylemanager.sip
@@ -11,6 +11,9 @@ class QgsMapLayerStyle
     //! Tell whether the style is valid (i.e. there is something stored in it)
     bool isValid() const;
 
+    //! Remove any stored style data (will get invalid)
+    void clear();
+
     //! Return information about the style - for debugging purposes only
     QString dump() const;
 
@@ -35,6 +38,9 @@ class QgsMapLayerStyleManager
     //! Construct a style manager associated with a map layer (must not be null)
     QgsMapLayerStyleManager( QgsMapLayer* layer );
 
+    //! Reset the style manager to a basic state - with one default style which is set as current
+    void reset();
+
     //! Read configuration (for project loading)
     void readXml( const QDomElement& mgrElement );
     //! Write configuration (for project saving)

diff --git a/src/app/qgsmaplayerstyleguiutils.cpp b/src/app/qgsmaplayerstyleguiutils.cpp
@@ -33,9 +33,6 @@ QMenu* QgsMapLayerStyleGuiUtils::createStyleManagerMenu( QgsMapLayer* layer )
 
   QgsMapLayerStyleManager* mgr = layer->styleManager();
 
-  if ( !mgr )
-    return m;
-
   QMenu* mRemove = m->addMenu( tr( "Remove" ) );
   m->addSeparator();
 
@@ -78,8 +75,6 @@ void QgsMapLayerStyleGuiUtils::addStyle()
   if ( !ok || text.isEmpty() )
     return;
 
-  layer->enableStyleManager(); // make sure it exists
-
   bool res = layer->styleManager()->addStyleFromLayer( text );
 
   if ( res ) // make it active!
@@ -116,11 +111,7 @@ void QgsMapLayerStyleGuiUtils::removeStyle()
     return;
 
   if ( layer->styleManager()->styles().count() == 1 )
-  {
-    // let's get rid of the style manager altogether
-    layer->enableStyleManager( false );
     return;
-  }
 
   QString name = a->text();
   if ( name == defaultStyleName() )

diff --git a/src/app/qgsvisibilitypresets.cpp b/src/app/qgsvisibilitypresets.cpp
@@ -115,8 +115,7 @@ void QgsVisibilityPresets::addPerLayerCurrentStyle( QgsVisibilityPresets::Preset
     if ( !nodeLayer )
       continue;
 
-    if ( nodeLayer->layer()->styleManager() )
-      rec.mPerLayerCurrentStyle[layerID] = nodeLayer->layer()->styleManager()->currentStyle();
+    rec.mPerLayerCurrentStyle[layerID] = nodeLayer->layer()->styleManager()->currentStyle();
   }
 }
 
@@ -209,8 +208,7 @@ void QgsVisibilityPresets::applyPresetCheckedLegendNodesToLayer( const QString&
   if ( rec.mPerLayerCurrentStyle.contains( layerID ) )
   {
     // apply desired style first
-    if ( layer->styleManager() )
-      layer->styleManager()->setCurrentStyle( rec.mPerLayerCurrentStyle[layerID] );
+    layer->styleManager()->setCurrentStyle( rec.mPerLayerCurrentStyle[layerID] );
   }
 
   QgsVectorLayer* vlayer = qobject_cast<QgsVectorLayer*>( layer );
@@ -275,8 +273,7 @@ void QgsVisibilityPresets::applyStateToLayerTreeGroup( QgsLayerTreeGroup* parent
         if ( rec.mPerLayerCurrentStyle.contains( nodeLayer->layerId() ) )
         {
           // apply desired style first
-          if ( nodeLayer->layer()->styleManager() )
-            nodeLayer->layer()->styleManager()->setCurrentStyle( rec.mPerLayerCurrentStyle[nodeLayer->layerId()] );
+          nodeLayer->layer()->styleManager()->setCurrentStyle( rec.mPerLayerCurrentStyle[nodeLayer->layerId()] );
         }
 
         QgsLayerTreeModel* model = QgisApp::instance()->layerTreeView()->layerTreeModel();
@@ -359,9 +356,7 @@ void QgsVisibilityPresets::applyState( const QString& presetName )
       continue;
 
     QString name = rec.mPerLayerCurrentStyle[layerID];
-    if ( !ml->styleManager() )
-      rec.mPerLayerCurrentStyle.remove( layerID );
-    else if ( !ml->styleManager()->styles().contains( name ) )
+    if ( !ml->styleManager()->styles().contains( name ) )
       rec.mPerLayerCurrentStyle[layerID] = ml->styleManager()->currentStyle();
   }
 }

diff --git a/src/core/qgsmaplayer.cpp b/src/core/qgsmaplayer.cpp
@@ -56,7 +56,7 @@ QgsMapLayer::QgsMapLayer( QgsMapLayer::LayerType type,
     mLayerType( type ),
     mBlendMode( QPainter::CompositionMode_SourceOver ) // Default to normal blending
     , mLegend( 0 )
-    , mStyleManager( 0 )
+    , mStyleManager( new QgsMapLayerStyleManager( this ) )
 {
   mCRS = new QgsCoordinateReferenceSystem();
 
@@ -86,6 +86,7 @@ QgsMapLayer::~QgsMapLayer()
 {
   delete mCRS;
   delete mLegend;
+  delete mStyleManager;
 }
 
 QgsMapLayer::LayerType QgsMapLayer::type() const
@@ -708,12 +709,9 @@ void QgsMapLayer::readStyleManager( const QDomNode& layerNode )
 {
   QDomElement styleMgrElem = layerNode.firstChildElement( "map-layer-style-manager" );
   if ( !styleMgrElem.isNull() )
-  {
-    enableStyleManager();
-    styleManager()->readXml( styleMgrElem );
-  }
+    mStyleManager->readXml( styleMgrElem );
   else
-    enableStyleManager( false );
+    mStyleManager->reset();
 }
 
 void QgsMapLayer::writeStyleManager( QDomNode& layerNode, QDomDocument& doc ) const
@@ -1448,22 +1446,6 @@ QgsMapLayerLegend*QgsMapLayer::legend() const
   return mLegend;
 }
 
-void QgsMapLayer::enableStyleManager( bool enable )
-{
-  if (( enable && mStyleManager ) || ( !enable && !mStyleManager ) )
-    return;
-
-  if ( enable )
-  {
-    mStyleManager = new QgsMapLayerStyleManager( this );
-  }
-  else
-  {
-    delete mStyleManager;
-    mStyleManager = 0;
-  }
-}
-
 QgsMapLayerStyleManager* QgsMapLayer::styleManager() const
 {
   return mStyleManager;

diff --git a/src/core/qgsmaplayer.h b/src/core/qgsmaplayer.h
@@ -388,17 +388,8 @@ class CORE_EXPORT QgsMapLayer : public QObject
      */
     QgsMapLayerLegend* legend() const;
 
-    /**
-     * Enable or disable layer's style manager. When disabled (default), the styleManager() will return null pointer.
-     * By enabling the style manager will be created with one default style (same as the layer's active style).
-     * By disabling the style manager all associated styles will be lost (only the layer's active style will stay).
-     * @note added in 2.8
-     */
-    void enableStyleManager( bool enable = true );
-
     /**
      * Get access to the layer's style manager. Style manager allows switching between multiple styles.
-     * If the style manager is not enabled, null pointer will be returned.
      * @note added in 2.8
      */
     QgsMapLayerStyleManager* styleManager() const;

diff --git a/src/core/qgsmaplayerstylemanager.cpp b/src/core/qgsmaplayerstylemanager.cpp
@@ -25,9 +25,13 @@
 QgsMapLayerStyleManager::QgsMapLayerStyleManager( QgsMapLayer* layer )
     : mLayer( layer )
 {
-  QgsMapLayerStyle defaultStyle;
-  defaultStyle.readFromLayer( mLayer );
-  mStyles.insert( QString(), defaultStyle );
+  reset();
+}
+
+void QgsMapLayerStyleManager::reset()
+{
+  mStyles.insert( QString(), QgsMapLayerStyle() ); // insert entry for the default current style
+  mCurrentStyle.clear();
 }
 
 void QgsMapLayerStyleManager::readXml( const QDomElement& mgrElement )
@@ -49,8 +53,6 @@ void QgsMapLayerStyleManager::readXml( const QDomElement& mgrElement )
 
 void QgsMapLayerStyleManager::writeXml( QDomElement& mgrElement ) const
 {
-  const_cast<QgsMapLayerStyleManager*>( this )->syncCurrentStyle();
-
   QDomDocument doc = mgrElement.ownerDocument();
   mgrElement.setAttribute( "current", mCurrentStyle );
 
@@ -70,8 +72,13 @@ QStringList QgsMapLayerStyleManager::styles() const
 
 QgsMapLayerStyle QgsMapLayerStyleManager::style( const QString& name ) const
 {
-  if ( name == mCurrentStyle ) // make sure it is sync'ed
-    const_cast<QgsMapLayerStyleManager*>( this )->syncCurrentStyle();
+  if ( name == mCurrentStyle )
+  {
+    // current style's entry is always kept invalid - get the style data from layer's properties
+    QgsMapLayerStyle curr;
+    curr.readFromLayer( mLayer );
+    return curr;
+  }
 
   return mStyles.value( name );
 }
@@ -128,17 +135,14 @@ bool QgsMapLayerStyleManager::setCurrentStyle( const QString& name )
   if ( mCurrentStyle == name )
     return true; // nothing to do
 
-  syncCurrentStyle(); // sync before unloading it
+  mStyles[mCurrentStyle].readFromLayer( mLayer ); // sync before unloading it
   mCurrentStyle = name;
   mStyles[mCurrentStyle].writeToLayer( mLayer );
+  mStyles[mCurrentStyle].clear(); // current style does not keep any stored data
   mLayer->triggerRepaint();
   return true;
 }
 
-void QgsMapLayerStyleManager::syncCurrentStyle()
-{
-  mStyles[mCurrentStyle].readFromLayer( mLayer );
-}
 
 // -----
 
@@ -151,6 +155,11 @@ bool QgsMapLayerStyle::isValid() const
   return !mXmlData.isEmpty();
 }
 
+void QgsMapLayerStyle::clear()
+{
+  mXmlData.clear();
+}
+
 QString QgsMapLayerStyle::dump() const
 {
   return mXmlData;
@@ -195,6 +204,10 @@ void QgsMapLayerStyle::readXml( const QDomElement& styleElement )
 
 void QgsMapLayerStyle::writeXml( QDomElement& styleElement ) const
 {
+  // the currently selected style has no content stored here (layer has all the information inside)
+  if ( !isValid() )
+    return;
+
   QDomDocument docX;
   docX.setContent( mXmlData );
   styleElement.appendChild( docX.documentElement() );

diff --git a/src/core/qgsmaplayerstylemanager.h b/src/core/qgsmaplayerstylemanager.h
@@ -42,6 +42,9 @@ class CORE_EXPORT QgsMapLayerStyle
     //! Tell whether the style is valid (i.e. there is something stored in it)
     bool isValid() const;
 
+    //! Remove any stored style data (will get invalid)
+    void clear();
+
     //! Return information about the style - for debugging purposes only
     QString dump() const;
 
@@ -70,10 +73,11 @@ class CORE_EXPORT QgsMapLayerStyle
  * record them in the currently active style without any extra effort required.
  *
  * When an instance is created, it creates "default" style (with empty name) recorded from the associated map layer
- * and it is set as the current style.
+ * and it is set as the current style. The instance must always contain at least one style (which is the current).
  *
- * The instance must always contain at least one style. If no extra styles are wanted, the style manager should get
- * disabled in QgsMapLayer instance.
+ * The style which is marked as current has no style data stored in its entry. This is to avoid duplication
+ * of data as all data is stored in map layer and can be retrieved easily. Also when saving, the entry for
+ * the current style will be saved with no data because everything is already saved by the map layer itself.
  *
  * @note added in 2.8
  */
@@ -83,6 +87,9 @@ class CORE_EXPORT QgsMapLayerStyleManager
     //! Construct a style manager associated with a map layer (must not be null)
     QgsMapLayerStyleManager( QgsMapLayer* layer );
 
+    //! Reset the style manager to a basic state - with one default style which is set as current
+    void reset();
+
     //! Read configuration (for project loading)
     void readXml( const QDomElement& mgrElement );
     //! Write configuration (for project saving)
@@ -109,10 +116,6 @@ class CORE_EXPORT QgsMapLayerStyleManager
     //! @return true on success
     bool setCurrentStyle( const QString& name );
 
-  private:
-    void syncCurrentStyle();
-    void ensureCurrentInSync() const;
-
   private:
     QgsMapLayer* mLayer;
     QMap<QString, QgsMapLayerStyle> mStyles;

diff --git a/tests/src/core/testqgsmaplayerstylemanager.cpp b/tests/src/core/testqgsmaplayerstylemanager.cpp
@@ -18,7 +18,6 @@ class TestQgsMapLayerStyleManager : public QObject
     void init();// will be called before each testfunction is executed.
     void cleanup();// will be called after every testfunction.
 
-    void testEnabled();
     void testDefault();
     void testStyle();
     void testReadWrite();
@@ -50,23 +49,8 @@ void TestQgsMapLayerStyleManager::cleanup()
   QgsMapLayerRegistry::instance()->removeAllMapLayers();
 }
 
-void TestQgsMapLayerStyleManager::testEnabled()
-{
-  QVERIFY( !mVL->styleManager() );
-
-  mVL->enableStyleManager( false );
-  QVERIFY( !mVL->styleManager() );
-
-  mVL->enableStyleManager();
-  QVERIFY( mVL->styleManager() );
-
-  mVL->enableStyleManager( false );
-  QVERIFY( !mVL->styleManager() );
-}
-
 void TestQgsMapLayerStyleManager::testDefault()
 {
-  mVL->enableStyleManager();
   QgsMapLayerStyleManager* mgr = mVL->styleManager();
   QVERIFY( mgr );
 
@@ -168,7 +152,6 @@ void TestQgsMapLayerStyleManager::testSwitchingStyles()
 {
   _setVLColor( mVL, Qt::red );
 
-  mVL->enableStyleManager();
   mVL->styleManager()->addStyleFromLayer( "s1" );
   mVL->styleManager()->setCurrentStyle( "s1" );