Bug report #6222

implement QgsStyleV2::save()

Added by Jürgen Fischer almost 8 years ago. Updated over 1 year ago.

Status:Closed
Priority:Normal
Assignee:-
Category:Symbology
Affected QGIS version:master Regression?:No
Operating System: Easy fix?:No
Pull Request or Patch supplied:No Resolution:end of life
Crashes QGIS or corrupts data:No Copied to github as #:15533

Description

QgsStyleV2::save() is disabled (see qgsstylev2.cpp:338) and needs to be implemented.

TestStyleV2::testSaveLoad() is failing because of that (see testqgsstylev2.cpp:191)

See also ![qgis-developer] tests FAILED: 19 - qgis_stylev2test


Related issues

Related to QGIS Application - Bug report #6229: changing existing symbol or color ramp does not work in n... Closed 2012-08-20

History

#1 Updated by Etienne Tourigny almost 8 years ago

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

#2 Updated by Jürgen Fischer almost 8 years ago

  • Assignee changed from Martin Dobias to Arunmozhi P

#3 Updated by Etienne Tourigny almost 8 years ago

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;
 }

#4 Updated by Etienne Tourigny almost 8 years ago

  • Category set to 83

#5 Updated by Arunmozhi P almost 8 years ago

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.

#6 Updated by Etienne Tourigny almost 8 years ago

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?

#7 Updated by Etienne Tourigny almost 8 years ago

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

#8 Updated by Etienne Tourigny almost 8 years ago

  • % Done changed from 0 to 10

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.

#9 Updated by Paolo Cavallini almost 8 years ago

  • Target version set to Version 2.0.0

#10 Updated by Arunmozhi P almost 8 years ago

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.

#11 Updated by Jürgen Fischer about 6 years ago

  • Target version changed from Version 2.0.0 to Future Release - Lower Priority

#12 Updated by Jürgen Fischer about 4 years ago

  • Assignee deleted (Arunmozhi P)

#13 Updated by Giovanni Manghi over 3 years ago

  • Easy fix? set to No
  • Regression? set to No

#14 Updated by Giovanni Manghi over 1 year ago

  • Status changed from Open to Closed
  • Resolution set to end of life

Also available in: Atom PDF