Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Put GeoPackage at first place in the menu #5385

Merged
merged 2 commits into from Oct 21, 2017
Merged

Put GeoPackage at first place in the menu #5385

merged 2 commits into from Oct 21, 2017

Conversation

@jachym
Copy link
Contributor

jachym commented Oct 16, 2017

Description

This PR puts OGC GeoPackage at first place in the "Create new vector layer" menu entry.

geopackagefirst

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Oct 16, 2017

Can you add a "[needs-docs]" tag to the commit message? The change in shortcut behavior will need to be updated in the qgis documentation

@DelazJ

This comment has been minimized.

Copy link
Contributor

DelazJ commented Oct 17, 2017

Question: does this also fix the toolbar?

@luipir

This comment has been minimized.

Copy link
Contributor

luipir commented Oct 17, 2017

@DelazJ should the tool bar buttons be substituted by the single add layer button ? IMHO the buttons remain just for a transition or nostalgic phase

@DelazJ

This comment has been minimized.

Copy link
Contributor

DelazJ commented Oct 17, 2017

These are buttons to create a new layer not to add a layer...
Do you mean that layer creation buttons are also meant to move to the unified add layer dialog?
My comment was to provide same shortcut/sort using the menu Layer -> Create layer than using the "group-menu" button from layer toolbar

@luipir

This comment has been minimized.

Copy link
Contributor

luipir commented Oct 17, 2017

@DelazJ about creation with single add button at least gpkg and splite, I don't remember if it's possibile also for memory and shape.
@elpaso any opinion about menu layer?

@DelazJ

This comment has been minimized.

Copy link
Contributor

DelazJ commented Oct 17, 2017

Not sure I follow you. In case, I am/was referring to these buttons (reorder the sequence and link shortcut to GPKG)
image

@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 17, 2017

Oh, I was not aware of the toolbar button - ok, will fix it also for the toolbar too.

Otherwise no objections?

@luipir

This comment has been minimized.

Copy link
Contributor

luipir commented Oct 17, 2017

@jachym go ahead :)

@jachym jachym force-pushed the jachym:geopackage branch from ab1a460 to bf102b4 Oct 18, 2017
@jachym jachym force-pushed the jachym:geopackage branch from bf102b4 to 516540a Oct 18, 2017
@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 18, 2017

@luipir Order of "new file" buttons in the toolbar changed as well
@nyalldawson Tags add

Anything else?

image

@rduivenvoorde

This comment has been minimized.

Copy link
Contributor

rduivenvoorde commented Oct 18, 2017

@jachym well, while you are on it ;-)
What about the Save Vector Layers as dialog Format dropdown?
(from layermanager, right click menu item Save As):

selection_131

I think that is a good candidate too?

@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 18, 2017

@rduivenvoorde and @luipir I've add another commit with some other dialog changes eb964ef can you guys review?

image

@luipir

This comment has been minimized.

Copy link
Contributor

luipir commented Oct 18, 2017

I can't ckeck for all conditions, but seems it's the correct way to get rid of shapefile :)... sometimes so little PRs can have a great effect. Great :)

@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 18, 2017

@luipir or great side effect - any hint, what to do with the travis check? apparently, it's complainig about api change

SIP file is not up to date
core/qgsvectorfilewriter.sip
@jachym jachym force-pushed the jachym:geopackage branch from eb964ef to 89608e8 Oct 18, 2017
@Gustry

This comment has been minimized.

Copy link
Contributor

Gustry commented Oct 18, 2017

@jachym Did you run the scripts/prepare-commit.sh before the commit?
It will run "sipify" for you, or you can run it manually: https://docs.qgis.org/testing/en/docs/developers_guide/codingstandards.html#id30

@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 18, 2017

@Gustry well I run scripts/sipify.pl manulally now - I was missing silversearcher-ag

ok, it should be fine by now

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Oct 18, 2017

Some tests apparently assume shapefiles as default.
Switching to gpkg here should be trivial:
https://github.com/qgis/QGIS/blob/master/tests/src/analysis/testqgsprocessing.cpp#L1212-L1214
Not sure exactly what happens here though:
https://github.com/qgis/QGIS/blob/master/tests/src/core/testqgsmaprendererjob.cpp#L179-L182

@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 18, 2017

neither do I :-(

@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 18, 2017

66 tests failing - but they do not seem to be by far related to "shp" issue :-/

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Oct 18, 2017

I'm -1 on the API changes. I think changing user facing settings is fine, but changing default API methods to use gpkg is premature and may have many unintentional side effects.

@@ -52,7 +52,7 @@
<links>
<link name="geonode:roads" type="OGC:WMS" description="my GeoNode road layer" url="http://example.org/wms"/>
<link name="geonode:roads" type="OGC:WFS" description="my GeoNode road layer" url="http://example.org/wfs"/>
<link name="roads" type="WWW:LINK" description="full dataset download" url="http://example.org/roads.tgz" format="ESRI Shapefile" mimeType="application/gzip" size="283676"/>
<link name="roads" type="WWW:LINK" description="full dataset download" url="http://example.org/roads.tgz" format="GPKG" mimeType="application/gzip" size="283676"/>

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Oct 18, 2017

Contributor

This change should be removed

This comment has been minimized.

Copy link
@jachym

jachym Oct 19, 2017

Author Contributor

you are right, changed back

@@ -29,7 +29,7 @@ def error(msg):

(minx, miny, maxx, maxy) = map(float, options.extent.split(","))

driverName = "ESRI Shapefile"
driverName = "GPKG"

This comment has been minimized.

Copy link
@nyalldawson

nyalldawson Oct 18, 2017

Contributor

Revert this change too - it's a purely test based tool, switching to gpkg has no benefit here

This comment has been minimized.

Copy link
@jachym

jachym Oct 19, 2017

Author Contributor

changed back

@jachym jachym force-pushed the jachym:geopackage branch from 89608e8 to 88df6a2 Oct 19, 2017
@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 19, 2017

Thank you @nyalldawson for review, any more comments?

Copy link
Contributor

luipir left a comment

Should be GPKG and not DPKG

@@ -306,7 +306,7 @@ void QgsAttributeActionDialog::addDefaultActions()
{
int pos = 0;
insertRow( pos++, QgsAction::Generic, tr( "Echo attribute's value" ), QStringLiteral( "echo \"[% \"MY_FIELD\" %]\"" ), QLatin1String( "" ), true, tr( "Attribute Value" ), QSet<QString>() << QStringLiteral( "Field" ), QString() );
insertRow( pos++, QgsAction::Generic, tr( "Run an application" ), QStringLiteral( "ogr2ogr -f \"ESRI Shapefile\" \"[% \"OUTPUT_PATH\" %]\" \"[% \"INPUT_FILE\" %]\"" ), QLatin1String( "" ), true, tr( "Run application" ), QSet<QString>() << QStringLiteral( "Feature" ) << QStringLiteral( "Canvas" ), QString() );
insertRow( pos++, QgsAction::Generic, tr( "Run an application" ), QStringLiteral( "ogr2ogr -f \"DPKG\" \"[% \"OUTPUT_PATH\" %]\" \"[% \"INPUT_FILE\" %]\"" ), QLatin1String( "" ), true, tr( "Run application" ), QSet<QString>() << QStringLiteral( "Feature" ) << QStringLiteral( "Canvas" ), QString() );

This comment has been minimized.

Copy link
@luipir

luipir Oct 19, 2017

Contributor

Should be GPKG and not DPKG

This comment has been minimized.

Copy link
@jachym

jachym Oct 19, 2017

Author Contributor

oh, thanks, fixed

@luipir

This comment has been minimized.

Copy link
Contributor

luipir commented Oct 19, 2017

@nyalldawson at what api change are you referring... btw I feel more confortable to this kind of api change than more new features. IMHO This will force us to fix unconsistent use fo gpkg.
Just my 2c

@jachym jachym force-pushed the jachym:geopackage branch 2 times, most recently from f22a524 to f0be0fe Oct 19, 2017
@jachym jachym force-pushed the jachym:geopackage branch 2 times, most recently from 9e28ec2 to d9e403a Oct 19, 2017
@jachym jachym force-pushed the jachym:geopackage branch from d9e403a to 28daa1a Oct 19, 2017
@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 19, 2017

Horray, travis passed

@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Oct 20, 2017

Looks good to me.

@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 20, 2017

@luipir are you ok with current pull request? can you guys push, to push this to master?

@luipir

This comment has been minimized.

Copy link
Contributor

luipir commented Oct 21, 2017

go ahead :)

@m-kuhn m-kuhn merged commit 6428fed into qgis:master Oct 21, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Oct 22, 2017

I've had to revert the changes to testqgsmaprendererjob - they've been causing hangs on Travis (see e.g. https://travis-ci.org/qgis/QGIS/jobs/290839698 )

@jachym

This comment has been minimized.

Copy link
Contributor Author

jachym commented Oct 22, 2017

OMFG, thanks a lot @m-kuhn !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.