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

[FEATURE] New zip format #4845

Merged
merged 30 commits into from Aug 4, 2017

Conversation

Projects
None yet
8 participants
@pblottiere
Member

pblottiere commented Jul 12, 2017

Description

In the context of the QEP qgis/QGIS-Enhancement-Proposals#27 in which the goal is to provide an auxiliary storage, a new format .qgz is proposed here.

In fact, it's for now just a zip of the usual .qgs project file. But in a second phase (others PR will follow), the auxiliary storage among other things may be embedded within the zip project.

To do that, a new dependency has been introduced : libzip.

This PR is of course a proposition so comments/ideas/criticisms are obviously welcome!

Some tests have been added to check the current behavior.

Checklist

  • 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
@NathanW2

This comment has been minimized.

Show comment
Hide comment
@NathanW2

NathanW2 Jul 12, 2017

Member
Member

NathanW2 commented Jul 12, 2017

Show outdated Hide outdated src/core/qgsproject.h
Show outdated Hide outdated src/app/qgisapp.cpp
Show outdated Hide outdated src/app/qgisapp.cpp
Show outdated Hide outdated src/app/qgisapp.cpp
/**
* Returns the current .qgs project file or an empty string if there's none
*/
QString projectFile() const;

This comment has been minimized.

@nyalldawson

nyalldawson Jul 13, 2017

Contributor

I think this should be left out of the base QgsArchive class, and QgsArchive should be left only with generic zip/unzip handling functions (nothing project specific - so reusable for other contexts). QgsProjectArchive could be a subclass with the extra project specific methods added.

@nyalldawson

nyalldawson Jul 13, 2017

Contributor

I think this should be left out of the base QgsArchive class, and QgsArchive should be left only with generic zip/unzip handling functions (nothing project specific - so reusable for other contexts). QgsProjectArchive could be a subclass with the extra project specific methods added.

This comment has been minimized.

@pblottiere

pblottiere Jul 13, 2017

Member

You're right, it's done

@pblottiere

pblottiere Jul 13, 2017

Member

You're right, it's done

Show outdated Hide outdated src/core/qgsproject.cpp
@wonder-sk

This comment has been minimized.

Show comment
Hide comment
@wonder-sk

wonder-sk Jul 13, 2017

Member

Very exciting stuff! Looking forward to the auxiliary storage!

Member

wonder-sk commented Jul 13, 2017

Very exciting stuff! Looking forward to the auxiliary storage!

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jul 13, 2017

Member

@nyalldawson @wonder-sk Thank you for this first review, it's very helping!

I let you know!

Member

pblottiere commented Jul 13, 2017

@nyalldawson @wonder-sk Thank you for this first review, it's very helping!

I let you know!

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jul 13, 2017

Member

This looks really promising!

Member

m-kuhn commented Jul 13, 2017

This looks really promising!

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jul 13, 2017

Member

You can possibly add

libzip-dev

to .travis.yml

Member

m-kuhn commented Jul 13, 2017

You can possibly add

libzip-dev

to .travis.yml

Show outdated Hide outdated .travis.yml
@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jul 18, 2017

Member

I took into account your comments. Now we have something like this as @wonder-sk suggested:

project = QgsProject()
project.read('/home/xyz/project.qgz')
project.isZipped()  # returns True

Let me know what you think.

Member

pblottiere commented Jul 18, 2017

I took into account your comments. Now we have something like this as @wonder-sk suggested:

project = QgsProject()
project.read('/home/xyz/project.qgz')
project.isZipped()  # returns True

Let me know what you think.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jul 24, 2017

Member

@nyalldawson @wonder-sk @m-kuhn @mhugo

Do you have other comments about this PR? If not, would you considere to merge this PR?

Thank you!

Member

pblottiere commented Jul 24, 2017

@nyalldawson @wonder-sk @m-kuhn @mhugo

Do you have other comments about this PR? If not, would you considere to merge this PR?

Thank you!

Show outdated Hide outdated src/core/qgsarchive.h
Show outdated Hide outdated src/core/qgsarchive.h
Show outdated Hide outdated src/core/qgsziputils.h
Show outdated Hide outdated src/core/qgsziputils.h
@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jul 24, 2017

Member

@nyalldawson I took into account your last comments. Thank's for the review! Let me know.

Member

pblottiere commented Jul 24, 2017

@nyalldawson I took into account your last comments. Thank's for the review! Let me know.

@mhugo

This comment has been minimized.

Show comment
Hide comment
@mhugo

mhugo Jul 24, 2017

Contributor

@jef-n Hi. We have a new dependency here that does not exist in osgeo4w. What is the best way to do ? I've compiled and packaged it (visual studio 2015) here.
Let us know how we can help to have a functional windows night build once this PR merged

Contributor

mhugo commented Jul 24, 2017

@jef-n Hi. We have a new dependency here that does not exist in osgeo4w. What is the best way to do ? I've compiled and packaged it (visual studio 2015) here.
Let us know how we can help to have a functional windows night build once this PR merged

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jul 28, 2017

Member

Is there other comments about the content of this PR?

Because I would like to propose another PR soon with Auxiliary Storage but I need ZIP support for that.

Let me know what can I do to improve this PR to have something finally mergeable :)

Member

pblottiere commented Jul 28, 2017

Is there other comments about the content of this PR?

Because I would like to propose another PR soon with Auxiliary Storage but I need ZIP support for that.

Let me know what can I do to improve this PR to have something finally mergeable :)

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Jul 28, 2017

Contributor

It's a +1 from me. But I'd like a second opinion for a change this significant.

Contributor

nyalldawson commented Jul 28, 2017

It's a +1 from me. But I'd like a second opinion for a change this significant.

@m-kuhn

m-kuhn approved these changes Jul 30, 2017

+1, this is a very nice improvement

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Jul 31, 2017

Member

Travis build is failing but it doesn't seem to be related to this PR.

Member

pblottiere commented Jul 31, 2017

Travis build is failing but it doesn't seem to be related to this PR.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jul 31, 2017

Member

Probably an inconsistency between this branch and master. Rebase or run the script mentioned on travis manually to synchronize.

Member

m-kuhn commented Jul 31, 2017

Probably an inconsistency between this branch and master. Rebase or run the script mentioned on travis manually to synchronize.

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Aug 1, 2017

Member

By the way, I rebased as @m-kuhn suggested and now everything seems green :)

Member

pblottiere commented Aug 1, 2017

By the way, I rebased as @m-kuhn suggested and now everything seems green :)

@NathanW2

This comment has been minimized.

Show comment
Hide comment
@NathanW2

NathanW2 Aug 1, 2017

Member
Member

NathanW2 commented Aug 1, 2017

@rldhont

rldhont approved these changes Aug 1, 2017

Great works and useful feature

@wonder-sk

This comment has been minimized.

Show comment
Hide comment
@wonder-sk

wonder-sk Aug 4, 2017

Member

Looks good to me as well... +1 to merge the PR!

Member

wonder-sk commented Aug 4, 2017

Looks good to me as well... +1 to merge the PR!

@m-kuhn m-kuhn merged commit b20f4c6 into qgis:master Aug 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Aug 4, 2017

Member

Wheee!!!

Member

m-kuhn commented Aug 4, 2017

Wheee!!!

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Aug 4, 2017

Contributor

silent cheer* (because everyone is too quiet in this room!)

Contributor

nyalldawson commented Aug 4, 2017

silent cheer* (because everyone is too quiet in this room!)

@pblottiere

This comment has been minimized.

Show comment
Hide comment
@pblottiere

pblottiere Aug 4, 2017

Member

Thank you for merging this PR :)

Member

pblottiere commented Aug 4, 2017

Thank you for merging this PR :)

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Aug 4, 2017

Contributor

@pblottiere thank you for doing all the hard work in this PR, so that @m-kuhn could steal all the credit by just hitting a single button...

Contributor

nyalldawson commented Aug 4, 2017

@pblottiere thank you for doing all the hard work in this PR, so that @m-kuhn could steal all the credit by just hitting a single button...

@3nids

This comment has been minimized.

Show comment
Hide comment
@3nids

3nids Aug 4, 2017

Member

💾 🎉

Member

3nids commented Aug 4, 2017

💾 🎉

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Aug 4, 2017

Member

👏

Member

m-kuhn commented Aug 4, 2017

👏

@NathanW2

This comment has been minimized.

Show comment
Hide comment
@NathanW2

NathanW2 Aug 4, 2017

Member
Member

NathanW2 commented Aug 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment