Diffmode

[ymoisan]

We are ready to integrate the code of our new comparison mode functionality on history revisions.

Changes in versioning_base.py :

• schemas are not used anymore to create revision views; add_revision_view(...) is deprecated but still in the code even though it it not used in either the tests or versioning.py; this eliminates Error when revision schema already exists #35
• two new functions were created to generate the SQL string necessary to display revisions, either in "full mode" (all features at revision X, which is the plugin's previous behaviour) or "comparison mode" aka "diffmode"

Tests were :

• modified to work without schemas for the full mode
• added to versioning_base_test.py for the new diffmode

Changes in the UI :

• the view dialog now has a "Compare selected revisions" checkbox in the top left; the checkbox is disabled and unchecked upon dialog opening; it only becomes checkable (enabled) when exactly two revisions are selected; unselecting one of the two revisions or selecting a third one disables and unchecks the "Compare selected revisions" checkbox
• when two revisions on different branches are selected, clicking "OK" will create two new layer groups (one for each revision selected; full mode); however, clicking on the "Compare selected revisions" checkbox will trigger a warning message box that revisions need to be on the same branch to be compared; the checkbox is then unchecked and the branch names that triggered the message are highlighted (cell background color set to yellow)
• in full mode, vector layers are of type 'postgres' (as before), except now layer properties do not point on a schema.view but rather on the table with a filter in the sql= argument
• for diffmode, layers are of type 'memory' and a rule-based symbology set was added to color the different "diff_status" values of each feature :
  • 'u' for updated
  • 'a' for added
  • 'd' for deleted
  • 'i' for intermediate (e.g. a feature that was edited multiple times between revisions being compared)

Small change of behaviour : the plugin operates the same way it has in the past, but we now retrieve all historization fields irrespective of branch in the attributes table; the reason is that a user might be interested to know that a feature on branch X shown also has a history in another branch.

If all is fine for you I will rebase before you proceed.

Thank you

[vmora]

delete commented out lines (makes for easier review)

[vmora]

+1 for merge, ping me when you have rebased.

[ymoisan]

I guess I introduced conflicts with the rebase. Sorry.

delete commented out lines (makes for easier review)

I removed some of the prints but I left the commented out print results in the tests I've added just so that asserts are clearer.

select_statement, where_clause = versioning_base.rev_view_str( pg_conn_info, 'epanet', 'junctions','mybranch', 2)

Good point; I used to be a Scheme programmer, so I like lists ;-)

Adding: rev_begin, rev_end_ = str(rev_begin), str(rev_end)_ would IMO increase readability of the following code in this function

Good point. Done.

I think I recall a case where the "CASE WHEN" was a lot slower then the equivalent "UNION".

For now we'll stick with that. If there really are issues then we'll fix them later, if you don't mind.

I don't see the interest of returning the select_str, I think it would be clearer to rename the function where_clause and just return the where_str

Could be. In fact all of this could have been done right in versioning.py. That's how I did it until a few days ago. I decided to keep a function in versioning_base for tests, but really all of that is QGIS specific IMO : select_str goes in the 'table=' parameter and where_str goes into 'sql='.

I could understand the need to test when a schema + view was created, but now just filtering a PG layer doesn't warrant much testing IMO. What you are suggesting amounts to building the select_str in versioning and have versioning_base provide the where_str. I don't think you'll have strong feelings about my not changing anything for now.

I added a bit of code in versioning.checkout_pg to check for valid schema names. Supplying names with illegal chars either popped a "ProgammingError" or made QGIS crash (reported by our users). Please note that in the case of letters I only allow lowercase. I know the PG naming rules allow for uppercase letters too, but I'm not sure quoting is handled properly. If this is OK with you, I've rebased and the code is ready to integrate.

Next step : I'd like to have your (and Oslandia's) feelings about the docs branch, because that will be our next contribution to the plugin. As you may have seen, I have started a reST documentation that is built dynamically on ReadTheDocs (http://eha-qgis-versioning.readthedocs.org/en/docs/). For now, the doc is built on eHA's RTD out of our fork's 'docs' branch. What I think should be done is to build the master branch of the main GH code repository. How do you feel about this ? Are you OK with a RTD site based on reST files ? The final product would be a 'help' button in the plugin menu bar (already there) pointing to the documentation site.

[vmora]

I think I recall a case where the "CASE WHEN" was a lot slower then the equivalent "UNION".

For now we'll stick with that. If there really are issues then we'll fix them later, if you don't mind.

No problem.

I could understand the need to test when a schema + view was created, but now just filtering a PG layer doesn't warrant much testing IMO.

I strongly disagree on that point.

I don't think you'll have strong feelings about my not changing anything for now.

No problem.

I only allow lowercase

Lowercase + underscore for schema, tables and attributes names is indeed what you want to have in your DB.

Are you OK with a RTD site based on reST files?

Yes.

[vmora]

The branch has conflicts, so not ready to integrate.