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

[External Storage] Define API and implement SimpleCopy backlend #43882

Merged
merged 20 commits into from
Jul 30, 2021

Conversation

troopa81
Copy link
Contributor

@troopa81 troopa81 commented Jun 23, 2021

This is the first PR of the proposed QEP qgis/QGIS-Enhancement-Proposals#196

It implements:

  • External storage API
  • Registry that contains all external storage backends
  • SimpleCopy external storage that stores the selected external resource on a specific location on disk

Widget modifications and WebDAV backend will come in separated PRs

I had to change the SIP_CONCAT_PARTS to 16 (I have errors when building pycore with 13, pyanalysis with 14 and 15).

cc @Jean-Roc
Funded by Lille Metropole https://www.lillemetropole.fr/

@troopa81 troopa81 requested a review from 3nids June 23, 2021 16:35
@github-actions github-actions bot added this to the 3.22.0 milestone Jun 23, 2021
@troopa81 troopa81 added Feature API API improvement only, no visible user interface changes labels Jun 23, 2021
@troopa81 troopa81 force-pushed the feat_dms_connexion_part1 branch 2 times, most recently from cef8f23 to 9cc02ef Compare June 24, 2021 11:10
@pblottiere
Copy link
Member

Hello @troopa81 ✋,

I took a quick look on code style. Let me know if I missed some things :).

@zacharlie zacharlie added the Changelog Items that are queued to appear in the visual changelog - remove after harvesting label Jun 25, 2021
@troopa81
Copy link
Contributor Author

Hi @pblottiere, and thanks for your review, I'm on my way to take your comments into consideration.

Regarding using unique_ptr in QgsExternalStorageRegistry, it brings a little bit of complexity:

  • I need to take a unique_ptr in registerExternalStorage and that doesn't fit well with SIP (though I could expose 2 functions, one for SIP, an another with unique_ptr)
  • the projectStorages() method would need to iterate over the hash table to build an other list which returns pointers and not unique_ptr

I'm wondering if these issues are worth using a unique_ptr. As it is, I can't see a way that could lead to memory leak.

@troopa81 troopa81 force-pushed the feat_dms_connexion_part1 branch 2 times, most recently from 9495368 to 3f09210 Compare June 30, 2021 12:20
Copy link
Member

@3nids 3nids left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed out, I am wondering if we should not create an object which could be started rather than starting automatically and have to check for different statuses? What do you think?

python/core/auto_generated/qgis.sip.in Outdated Show resolved Hide resolved
src/core/externalstorage/qgsexternalstorage.h Outdated Show resolved Hide resolved
src/core/externalstorage/qgsexternalstorageregistry.h Outdated Show resolved Hide resolved
@troopa81
Copy link
Contributor Author

Unrelated HANA test failure

@3nids I take all your comment into consideration, is it OK for merge?

@mkemeter
Copy link

Sorry for that. The unrelated test failure should be gone now.

Copy link
Member

@3nids 3nids left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to merge, thanks for the follow-ups!

@3nids 3nids merged commit af9a72e into qgis:master Jul 30, 2021
@troopa81
Copy link
Contributor Author

thanks @3nids

@zacharlie zacharlie added ChangelogHarvested This PR description has been harvested in the Changelog already. and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels Sep 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API improvement only, no visible user interface changes ChangelogHarvested This PR description has been harvested in the Changelog already. Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants