Quick and dirty patch to DB-Manager after PR 8831 #9086

elpaso · 2019-02-04T17:32:58Z

The "comments" PR #8831 added support for postgres only
(and broke all the others backends).

I'd be in favor of a revert of the whole original PR but
this patch restores functionality and could be an acceptable
temporary fix until the comments PR is reworked in a more
maintainable and elegant way.

Fixes #21151

The "comments" PR 8831 added support for postgres only (and broke all the others backends). I'd be in favor of a revert of the whole original PR but this patch restores functionality and could be an acceptable temporary fix until the comments PR is reworked in a more maintainable and elegant way. Fixes qgis#21151 btw

luipir

IMHO a better and wider solution would be in charge of the patched PR 8831.
Is it necessary to add need-docs because of QMessageBoxes @elpaso ?

m-kuhn · 2019-02-05T09:08:24Z

I'd be in favor of a revert of the whole original PR but
this patch restores functionality and could be an acceptable
temporary fix until the comments PR is reworked in a more
maintainable and elegant way.

Are there more things that you would like to have fixed on the original PR?

elpaso · 2019-02-05T10:05:18Z

@m-kuhn the architecture is plain wrong: the back-end specific part should go into the plugins.py of the various back-ends instead of the parent class, we now have a bunch of PG specific SQL in the parent class which is not specific to PG.

The GUI also needs some refactoring: it makes no sense to have post-mortem messages when after the user tried to set a comment on a back-end that do not supports (or worse do not implement) them, the GUI should hide the comments part completely if not supported/implemented.

Btw, I think that with this PR at least we don't get tracebacks anymore ---> ( if I manage to fix the pep8 thing, that my commit hooks insist to rewrite in a way that travis do not like: do we need a particular version of autopep8?).

m-kuhn · 2019-02-05T10:13:17Z

@m-kuhn the architecture is plain wrong: the back-end specific part should go into the plugins.py of the various back-ends instead of the parent class, we now have a bunch of PG specific SQL in the parent class which is not specific to PG.

agreed, is there more than the thing you put into the try-block?

Btw, I think that with this PR at least we don't get tracebacks anymore ---> ( if I manage to fix the pep8 thing, that my commit hooks insist to rewrite in a way that travis do not like: do we need a particular version of autopep8?).

Hmmm... don't know, I'm sorry.

elpaso · 2019-02-05T10:16:44Z

@m-kuhn the try-block works but it's just a "patch", I don't like it, but in the paid-bugfixing context I don't feel it's fair to spend QGIS money on a deeper refactoring (but I might be wrong on this).

m-kuhn · 2019-02-05T10:23:35Z

I was just trying to see the scope.

@Ailurupoda could you have a look at moving all the postgres specific sql (i.e. https://github.com/qgis/QGIS/pull/8831/files#diff-95efee568b4d3a74493ad92c3e99b065R1098 and https://github.com/qgis/QGIS/pull/8831/files#diff-1e7be63b28c39937475e92fce50a24aeR59 into the postgres plugin?

Ailurupoda · 2019-02-05T11:10:28Z

Oh sorry, i can see the mistake :/
I will take a look to see how that can be change.
For the getComment function it won't be a problem to move, but for what is inside setField i'm not sure on how that can be moved.
I'll try to watch for that

m-kuhn · 2019-02-05T11:21:37Z

Is there no access to plugin.py via self.db?

Ailurupoda · 2019-02-05T11:27:05Z

Is there no access to plugin.py via self.db?

I don't know. But i'm not sure to understand how this would help.

The setField() method is on the dlg_field_properties.py file. Not sure on how to make this function inherit on the side of postgis.

m-kuhn · 2019-02-05T12:06:54Z

The setField() method is on the dlg_field_properties.py file. Not sure on how to make this function inherit on the side of postgis.

Via self.db.connector it should be possible to call any method in db_manager/db_plugins/postgis/connector.py (and of the inherited db_manager/db_plugins/connector.py) from setFields().

m-kuhn · 2019-02-05T15:10:52Z

@Ailurupoda do you have an idea when you'll have time to work on this? A rough idea will be good to decide on the next steps on this PR and your original PR.

Ailurupoda · 2019-02-05T15:13:37Z

@Ailurupoda do you have an idea when you'll have time to work on this? A rough idea will be good to decide on the next steps on this PR and your original PR.

I'm working on it, i think i managed to fix this. I've moved to connector and plugin files and it works using postgis. I didn't test with spatialite or oracle, i've no base to test.

I'm working on the commit. By the way, where should i modify and commit ?

m-kuhn · 2019-02-05T15:16:26Z

Nice!
Geopackage and Spatialite should be very easy to test since they are file based formats. Just test any of the gpkg files in here https://github.com/qgis/QGIS/tree/master/tests/testdata

elpaso · 2019-02-05T15:22:38Z

@Ailurupoda thanks! Please don't forget the GUI changes to the two dialogs where comments are set/deleted and listed.

Ailurupoda · 2019-02-05T15:51:05Z

@Ailurupoda thanks! Please don't forget the GUI changes to the two dialogs where comments are set/deleted and listed.

Sorry, i don't understand ? :/
What changes should i not forget ?

elpaso · 2019-02-05T15:54:00Z

@Ailurupoda if a backend does not support comments you must hide all comments related elements from the dialogs.

Ailurupoda · 2019-02-05T15:58:11Z

So it must have multiple dialogs running where some get comment some does not ?
Or is there a way to hide somehow some field ?

For the moment i have dialogs where comment are editable, but for exemple for the GPKG, there is no error, but the comment won't be added

elpaso · 2019-02-05T16:03:45Z

So it must have multiple dialogs running where some get comment some does not ?
Or is there a way to hide somehow some field ?

That's up to you (if implement a single dialog and hide fields or implement multiple dialogs)

And yes, there are ways to hide fields and columns (or better: to not add them at all)

For the moment i have dialogs where comment are editable, but for exemple for the GPKG, there is no error, but the comment won't be added

This is IMO not a good design: if comments are not supported they must not be shown.

Ailurupoda · 2019-02-05T16:08:53Z

This is IMO not a good design: if comments are not supported they must not be shown.

I agree with you, but the fact is i don't know how to not show them .. I'll try on this !

m-kuhn · 2019-02-05T16:11:45Z

That's often the easiest

widget.setVisible(False)

elpaso · 2019-02-06T06:46:22Z

I'm merging this small patch while we are waiting for the real solution

m-kuhn · 2019-02-06T10:22:56Z

python/plugins/db_manager/dlg_table_properties.py

        try:
            #Using the db connector, executing de SQL query Comment on table
            self.db.connector._execute(None, 'COMMENT ON TABLE "{0}"."{1}" IS E\'{2}\';'.format(self.table.schema().name, self.table.name, self.viewComment.text()))
        except DbError as e:
            DlgDbError.showError(e, self)
            return
+        except Exception as e:


@elpaso is there any chance to avoid the catch all exception? I think even for quick and dirty patches this should be avoided.

elpaso added the Bugfix label Feb 4, 2019

elpaso added this to the 3.6.0 milestone Feb 4, 2019

luipir approved these changes Feb 4, 2019

View reviewed changes

Code style (I had autopep8 installed btw)

de5171a

Astyle again

33b8bbd

elpaso merged commit e4df72e into qgis:master Feb 6, 2019

elpaso deleted the bugfix-21151-db-manager-comments-armageddon branch February 6, 2019 06:46

m-kuhn reviewed Feb 6, 2019

View reviewed changes

qgib mentioned this pull request May 25, 2019

spatialite, edit table in dbmanager is broken in master Fault: no such table: pg_description #28969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quick and dirty patch to DB-Manager after PR 8831 #9086

Quick and dirty patch to DB-Manager after PR 8831 #9086

elpaso commented Feb 4, 2019 •

edited by m-kuhn

luipir left a comment

m-kuhn commented Feb 5, 2019

elpaso commented Feb 5, 2019

m-kuhn commented Feb 5, 2019 •

edited

elpaso commented Feb 5, 2019

m-kuhn commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

m-kuhn commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

m-kuhn commented Feb 5, 2019 •

edited

m-kuhn commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019 •

edited

m-kuhn commented Feb 5, 2019

elpaso commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

elpaso commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

elpaso commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

m-kuhn commented Feb 5, 2019

elpaso commented Feb 6, 2019

m-kuhn Feb 6, 2019

Quick and dirty patch to DB-Manager after PR 8831 #9086

Quick and dirty patch to DB-Manager after PR 8831 #9086

Conversation

elpaso commented Feb 4, 2019 • edited by m-kuhn

luipir left a comment

Choose a reason for hiding this comment

m-kuhn commented Feb 5, 2019

elpaso commented Feb 5, 2019

m-kuhn commented Feb 5, 2019 • edited

elpaso commented Feb 5, 2019

m-kuhn commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

m-kuhn commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

m-kuhn commented Feb 5, 2019 • edited

m-kuhn commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019 • edited

m-kuhn commented Feb 5, 2019

elpaso commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

elpaso commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

elpaso commented Feb 5, 2019

Ailurupoda commented Feb 5, 2019

m-kuhn commented Feb 5, 2019

elpaso commented Feb 6, 2019

m-kuhn Feb 6, 2019

Choose a reason for hiding this comment

elpaso commented Feb 4, 2019 •

edited by m-kuhn

m-kuhn commented Feb 5, 2019 •

edited

m-kuhn commented Feb 5, 2019 •

edited

Ailurupoda commented Feb 5, 2019 •

edited