QGIS » QGIS Application

Martin -

any idea when this could be fixed? I am sort of stuck in the color ramp improvements, because they cannot be saved not restored in current master. Thanks

The patch below solves the creation problem in the UI, but the style test still fails.

QgsStyleV2::save() does nothing, so to save a ramp you need to call QgsStyleV2::saveColorRamp(). However, if save() were implemented (calling save() on symbols and ramps), it would not work with current style manager dialog, because symbols and ramps would be saved twice (which would generate an error on the second save).

My advice would be to save new and modified symbols and ramps when dialog is closed (i.e. in save()), to replicate previous behaviour - although this might be tricky given sql storage. The idea is this should work both in UI and also outside of it, without having to call addSymbol() and saveSymbol() - addSymbol() followed by save() should work as before.

Also, changing existing symbol or color ramp does not work (they are not saved and reloaded when qgis is restarted) - so a bit or work is needed to resolve these 2 issues.

I will file another bug report on that. Thanks

diff --git a/src/gui/symbology-ng/qgsstylev2managerdialog.cpp b/src/gui/symbology-ng/qgsstylev2managerdialog.cpp
index 17d21b9..d8bc2e0 100644
--- a/src/gui/symbology-ng/qgsstylev2managerdialog.cpp
+++ b/src/gui/symbology-ng/qgsstylev2managerdialog.cpp
@@ -437,6 +437,8 @@ QString QgsStyleV2ManagerDialog::addColorRampStatic( QWidget* parent, QgsStyleV2

   // add new symbol to style and re-populate the list
   style->addColorRamp( name, ramp );
+  // TODO groups and tags
+  style->saveColorRamp( name, ramp, 0, QStringList() );
   return name;
 }

IMHO, I think we should perhaps do away with the save() altogether and embrace saveSymbol(...) and saveColorRamp(...) instead.

The save() approach was fine when everything was in a single XML file, and was re-written completely whenever the save() was called. That made sense since the entire QgsStyleV2 was written into a file. The same was used for export of symbols as well. But in a SQLite DB, I think applying the same approach is a bit of brute force. We make intelligent usage by saving only what is changed or created.

As for reading/writing in XML files, we can use the importXML(..) and the exportXML(...) functions. Hence, I recommend we re-write the test to include the saveSymbol, saveColorramp, importXML, and exportXML, and drop save() completely.

Hi, this makes sense. It would be challenging to consider all cases in save().

However, it would be nice to be able to add and save a symbol in one call, something like addSymbol(symbol,name,...,save=true) which calls saveSymbol() if save == true - or saveSymbol() which adds the symbol if it doesn't exist.

What do you think?

oh and the test does not deal with xml import/export yet, although that would be a good thing to test.

Etienne Tourigny wrote:

Partial fixes b145998078 (by me) and 447c0d1 (by Arun).

tests now working properly, but leaving this bug open because save() is still not implemented. This is needed to export style.

I am wondering what should be done by he save() function. If it has to perform the opposite of what load() does so the load+save test can be carried out, then creating the sqlite db is the way to go. IMHO, sqlite as such is not a good exchange format, hence exportXML and importXML were written. Apart from symbol exchange if there is a requirement for a sqlite file, I think I will write the code for creating a sqlite file. If there is no such requirement, IMO I think, we test load() alone and drop save() altogether. And close this bug too.