Delete style from db provider postgis #4064

jgrocha · 2017-01-26T17:21:06Z

Hi,

As already been discussed on the mailing list there was no way to remove a saved style from the database.

This PR adds the possibility to remove a style from the database, just for the postgres provider, for now.
Each provider has a new method virtual bool isDeleteStyleFromDBSupported() which return false until we add this functionality to it. Only qgspostgresprovider returns true and implements QgsPostgresProvider::deleteStyleById.

If the provider does not support this feature, the delete button is hidden from the interface.

The dialog for load and delete styles looks almost the same. It has a new title and a new delete button. The buttons (delete and load) are only enabled when a style is selected.

After deleting the style from the database, instead of just remove the row from the interface, I am reading the style again from the database. The reason is that this kind of provider is used in environments with multiple users that might have added, deleted of updated the styles in the meanwhile.

Feedback is very welcome. I would like thank @luipir and @nyalldawson the feedback already sent on the mailing list.

The changes might have small impact on the documentation. Should I do something about it or it will be managed by the documentation team?

jgrocha · 2017-01-27T00:27:16Z

I'm still learning how to contribute. I would really appreciate if someone can guide me to solve the two issues reported by Travis, regarding documentation and sip coverage.

documentation

Unacceptable missing documentation:
Class QgsVectorDataProvider, 61/64 members documented
  deleteStyleById(const QString &uri, QString styleId, QString &errCause)
  isDeleteStyleFromDBSupported() const

Where and how to document these two new methods?

sip coverage

FAIL: new unbound members have been introduced, please add SIP bindings for these members
If these members are not suitable for the Python bindings, please add the Doxygen tag
"@note not available in Python bindings" to the MEMBER Doxygen comments
----------------------------------------------------------------------
Ran 1 test in 11.992s
FAILED (failures=1)
CTEST_FULL_OUTPUT
---------------------------------
Missing members:
  QgsVectorDataProvider.deleteStyleById
  QgsVectorDataProvider.isDeleteStyleFromDBSupported

Where do I add these two new methods?

local tests

On my local qgis sources, I can run almost all tests. But
both PyQgsDocCoverage and PyQgsSipCoverage complains with "No tests were found!!!". Other tests are working. How can I enable or run these two tests in my computer?

luipir

I didnt' test the PR, but generally speacking, I'm in favour of this PR.

luipir · 2017-01-27T09:01:19Z

src/app/qgsloadstylefromdbdialog.cpp

@@ -25,10 +25,12 @@ QgsLoadStyleFromDBDialog::QgsLoadStyleFromDBDialog( QWidget *parent )
    , mSectionLimit( 0 )
 {
  setupUi( this );
-  setWindowTitle( QStringLiteral( "Load style from database" ) );
+  setWindowTitle( QStringLiteral( "Saved styles manager" ) );


I don't like, but would be necessary to leave reference to DB saved styles, otherwise can be confusing if style is saved on file system.
options can be:

DB Saved styles manager

Manage DB save styles

DB Saved styles

ony other

luipir · 2017-01-27T09:04:23Z

src/app/qgsloadstylefromdbdialog.cpp

+
+  if ( !msgError.isNull() )
+  {
+    QMessageBox::warning( this, infoWindowTitle, msgError );


it would be better use QgsMessageBar instead of dialog boxes

luipir · 2017-01-27T09:04:49Z

src/app/qgsloadstylefromdbdialog.cpp

+  }
+  else
+  {
+    QMessageBox::information( this, infoWindowTitle, tr( "Style deleted" ) );


as above: QgsMessageBar expecially for notification messages

luipir · 2017-01-27T09:05:12Z

src/app/qgsloadstylefromdbdialog.cpp

+    int sectionLimit = mLayer->listStylesInDatabase( ids, names, descriptions, errorMsg );
+    if ( !errorMsg.isNull() )
+    {
+      QMessageBox::warning( this, tr( "Error occurred retrieving styles from database" ), errorMsg );


luipir · 2017-01-27T09:05:53Z

src/app/qgsvectorlayerproperties.cpp

@@ -163,7 +163,7 @@ QgsVectorLayerProperties::QgsVectorLayerProperties(
    //for loading
    mLoadStyleMenu = new QMenu( this );
    mLoadStyleMenu->addAction( tr( "Load from file..." ) );
-    mLoadStyleMenu->addAction( tr( "Load from database" ) );
+    mLoadStyleMenu->addAction( tr( "Saved styles manager" ) );


as first message... lack of reference to DB

luipir · 2017-01-27T09:12:10Z

check https://dash.orfeo-toolbox.org/viewBuildError.php?type=1&buildid=259255 about Q_UNUSED build warning and more

"Saved styles manager" renamed to "Database styles manager"

luipir · 2017-01-30T09:58:55Z

would make sense to show if we are deleting the default style? this can be don adding a column.

btw, tested PR and works... IMHO can be accepted but we have to seriously think to rewritethe part of code related with style management on DB in a more organic way. But it is out of scope of this PR.

jgrocha · 2017-01-30T11:21:30Z

@luipir Thanks for your feedback. It was really helpful.

This is just a small contribution to solve the current QGIS limitation, regarding style removal from the provider.

I would like to have a much more improved panel to handle the styles from the DB, supporting load, save, delete, rename, clone, preview, change the default style for the layer, etc. I'm working on this. I'll discuss it soon as I get something to show.

luipir · 2017-01-30T11:37:13Z

in this PR, that is a new functionality, there is a lack of tests. In travis (AFAIK) there is a server to test against, but I can't find a test template to inspire writing a test.
If not available, you at least mock connection and create test in python (mock module is very powerful). btw mocked tests many times are hard to read and maintain.

luipir · 2017-01-30T12:45:37Z

Travis pg db is here:
https://github.com/qgis/QGIS/blob/master/ci/travis/linux/before_script.sh#L16

a compex example how to setup and use a pg db during test is here
https://github.com/qgis/QGIS/blob/master/tests/src/python/test_authmanager_pki_postgres.py

it make sense to have a setup sql to create db and to test against

jgrocha · 2017-01-30T18:18:36Z

@luipir I've checked out related tests. There is already one provider with tests related to saved styles by @rouault

I've followed @rouault approach and I added a new def testStyle(self): to the postgis provider tests, developed by @m-kuhn

m-kuhn · 2017-01-31T07:30:32Z

python/core/qgsvectorlayer.sip

@@ -685,6 +685,11 @@ class QgsVectorLayer : QgsMapLayer, QgsExpressionContextGenerator
    virtual QString getStyleFromDatabase( const QString& styleId, QString &msgError /Out/ );

    /**
+     * Will delete the named style corresponding to style id provided from the database
+     */
+    virtual void deleteStyleFromDatabase( const QString& styleId, QString &msgError /Out/ );


I think /Out/ parameters make mostly sense if the return value is already used for something else. Could we use the return value instead here?

+1 @m-kuhn I generally prefer an explicit bool as return success/fail value instead of hiding error in the msgError value Null or !Null, btw I don't have a stronger opinion on that.
I agree that if msgError is usdes, it can be the return value of the function. I suppose @jgrocha followed the same style as in getStyleFromDatabase where the return is the style string.

Thanks @m-kuhn and @luipir for the comments.

I've followed the saveStyleToDatabase signature, to make deleteStyleFromDatabase as similar as possible.

But I agree that it does make sense to have deleteStyleFromDatabase returning a bool value, to indicate the success of the operation.

So, I added a bool returning value and removed the /Out/ parameter from sip.

luipir · 2017-01-31T09:03:36Z

tnx @jgrocha to add this new test

m-kuhn · 2017-01-31T11:45:00Z

python/core/qgsvectorlayer.sip

@@ -685,6 +685,11 @@ class QgsVectorLayer : QgsMapLayer, QgsExpressionContextGenerator
    virtual QString getStyleFromDatabase( const QString& styleId, QString &msgError /Out/ );

    /**
+     * Will delete the named style corresponding to style id provided from the database
+     */
+    virtual bool deleteStyleFromDatabase( const QString& styleId, QString &msgError );


You still need to leave the /Out/annotation in place or the error message will not be accessible in python

Thanks @m-kuhn . I misinterpreted your previous comment.

I've put back the msgError available using SIP. I've adjust the python tests according to this signature.

Thanks @jgrocha , looks good now

nyalldawson

I've just done a quick review of the code style. I haven't tested this or made any functionality comments (@luipir has already covered these thoroughly!). Overall looks good to me, just some minor changes requested.

nyalldawson · 2017-02-04T21:57:19Z

src/app/qgsloadstylefromdbdialog.cpp

  mSelectedStyleId = QLatin1String( "" );
+  mSelectedStyleName = QLatin1String( "" );


This line (and also the one above it) isn't required. Qstrings will always be initialised to an empty string.

nyalldawson · 2017-02-04T21:58:03Z

python/core/qgsvectordataprovider.sip

+     * It returns false by default.
+     * Must be implemented by providers that support delete styles from db returning true
+     */
+    virtual bool isDeleteStyleFromDBSupported() const;


Note to self: replace all use of "DB" in API with "Db" or "database"

nyalldawson · 2017-02-04T21:59:12Z

src/app/qgsloadstylefromdbdialog.cpp

  connect( mRelatedTable, SIGNAL( doubleClicked( QModelIndex ) ), this, SLOT( accept() ) );
  connect( mOthersTable, SIGNAL( doubleClicked( QModelIndex ) ), this, SLOT( accept() ) );
  connect( mCancelButton, SIGNAL( clicked() ), this, SLOT( reject() ) );
+  connect( mDeleteButton, SIGNAL( clicked() ), this, SLOT( deleteStyleFromDB() ) );


We require use of new style qt connects in all new code - see qgis/QGIS-Enhancement-Proposals#77 for details and the motivation behind this change.

nyalldawson · 2017-02-04T22:00:40Z

src/app/qgsloadstylefromdbdialog.cpp

+  QTableWidgetItem *item;
+  QList<QTableWidgetItem *> selected = styleTable->selectedItems();
+
+  if ( selected.count() > 0 )


!selected.isEmpty()

(Using isEmpty makes the meaning of this check immediately obvious)

nyalldawson · 2017-02-04T22:01:23Z

src/app/qgsloadstylefromdbdialog.cpp

+  }
+  else
+  {
+    mSelectedStyleName = "";


mSelectedStyleName.clear()

(More efficient then construction of the empty string)

nyalldawson · 2017-02-04T22:03:07Z

src/app/qgsloadstylefromdbdialog.h


  private:
+    QgsVectorLayer *mLayer;


From a quick glance it seems this isn't initialised. Can you change this to:

QgsVectorLayer* mLayer = nullptr;

nyalldawson · 2017-02-04T22:04:51Z

src/core/qgsvectorlayer.cpp

+bool QgsVectorLayer::deleteStyleFromDatabase( const QString& styleId, QString &msgError )
+{
+  QgsProviderRegistry * pReg = QgsProviderRegistry::instance();
+  QLibrary *myLib = pReg->providerLibrary( mProviderKey );


Use

QScopedPointer here

Otherwise this function leaks.

Sorry @nyalldawson. I don't understand your comment. Where should I use QScopedPointer?

Change it to:

{ QScopedPointer<QLibrary> myLib( QgsProviderRegistry::instance()->providerLibrary( mProviderKey ) ); if ( !myLib ) { msgError = QObject::tr( "Unable to load %1 provider" ).arg( mProviderKey ); return false; } deleteStyleById_t* deleteStyleByIdMethod = reinterpret_cast< deleteStyleById_t * >( cast_to_fptr( myLib->resolve( "deleteStyleById" ) ) ); if ( !deleteStyleByIdMethod ) { msgError = QObject::tr( "Provider %1 has no %2 method" ).arg( mProviderKey, QStringLiteral( "deleteStyleById" ) ); return false; } return deleteStyleByIdMethod( mDataSource, styleId, msgError ); }

When the scoped pointer goes out of scope, the associated pointer is automatically deleted.

Thanks @nyalldawson.
The pattern I was using (without the QScopedPointer) is still used on many places. I've updated all usages of the old pattern just in src/core/qgsvectorlayer.cpp to this more safer alternative, using QScopedPointer without an explicit delete of the pointer.

nyalldawson · 2017-02-04T22:06:03Z

src/core/qgsvectorlayer.h

@@ -786,6 +786,11 @@ class CORE_EXPORT QgsVectorLayer : public QgsMapLayer, public QgsExpressionConte
    virtual QString getStyleFromDatabase( const QString& styleId, QString &msgError );

    /**
+     * Will delete the named style corresponding to style id provided from the database


Can you add a "@note added in QGIS 3.0" to all newly added public/protected members? (This helps plugin devs know what API calls they can safely use)

…ls#77 Changed isDeleteStyleFromDBSupported to isDeleteStyleFromDbSupported

nyalldawson · 2017-02-05T22:14:04Z

Thanks @jgrocha - I'll just wait for the Travis build results and then merge

nyalldawson · 2017-02-05T22:34:31Z

Great work - thank you!

jgrocha · 2017-02-05T22:38:35Z

Thanks @nyalldawson. I've learned a lot.

jgrocha added 4 commits January 26, 2017 16:31

Delete styles from postgres provider

2095b8e

Fix astyle

b7bacdd

Fix astyle

0eaf7bf

Sip coverage: first attempt

72c5cf0

luipir reviewed Jan 27, 2017

View reviewed changes

jgrocha added 2 commits January 28, 2017 19:09

QMessageBox replaced by QgsMessageBar

d60ddcc

"Saved styles manager" renamed to "Database styles manager"

Fix inconsistent-missing-override warning

bbf6ec0

Simple tests for style load, insert and remove from Postgis provider

a4637a6

make deleteStyleFromDatabase consistent with other vectorlayer's methods

6d6f288

m-kuhn reviewed Jan 31, 2017

View reviewed changes

jgrocha added 2 commits January 31, 2017 10:46

improve deleteStyleFromDatabase signature

8e989c1

improve deleteStyleFromDatabase signature

e649280

m-kuhn reviewed Jan 31, 2017

View reviewed changes

revert the SIP deleteStyleFromDatabase signature

738fbd4

This was referenced Feb 4, 2017

Delete styles from postgres provider #4105

Closed

Delete styles from postgres provider #4106

Closed

nyalldawson approved these changes Feb 4, 2017

View reviewed changes

jgrocha added 2 commits February 5, 2017 19:35

Updated Qt connections to the new style qgis/QGIS-Enhancement-Proposa…

8b36ad5

…ls#77 Changed isDeleteStyleFromDBSupported to isDeleteStyleFromDbSupported

Replaced QLibrary * with more safer QScopedPointer

20eea32

nyalldawson merged commit 4f7d9cd into qgis:master Feb 5, 2017

		mSelectedStyleId = QLatin1String( "" );
		mSelectedStyleName = QLatin1String( "" );

Delete style from db provider postgis #4064

Delete style from db provider postgis #4064

Conversation

jgrocha commented Jan 26, 2017

jgrocha commented Jan 27, 2017

documentation

sip coverage

local tests

luipir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luipir commented Jan 27, 2017

luipir commented Jan 30, 2017

jgrocha commented Jan 30, 2017

luipir commented Jan 30, 2017

luipir commented Jan 30, 2017

jgrocha commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luipir commented Jan 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyalldawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrocha Feb 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyalldawson commented Feb 5, 2017

nyalldawson commented Feb 5, 2017

jgrocha commented Feb 5, 2017

jgrocha Feb 5, 2017 •

edited

Loading