-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[ExternalStorage] Add widget to configure/edit and visualize external storage #44533
Conversation
src/gui/qgsfilewidget.h
Outdated
* \see setStorageUrlExpression() | ||
* \since 3.22 | ||
*/ | ||
QgsExpression *storageUrlExpression() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why a pointer return here? The ownership is unclear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has no need for QGIS application, it's just in case, somebody need somehow to interact with the defined expression through the API. It mas mostly to be consistent wtih what is done in QgsFeatureRequest.
The ownership is unclear
I would have said that when returning a pointer, the default expected behavior when nothing else is specified is that object keep ownership of the returned pointer, like it's done in Qt for instance or QGIS.
But, If you think it's necessary, I can add it.
02c63f4
to
89c2988
Compare
@3nids Did you have the time to take a look at this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some minor remarks
good job for writing detailed tests!
@@ -354,56 +341,61 @@ void QgsFileWidget::openFileDialog() | |||
return; | |||
|
|||
if ( mStorageMode != GetMultipleFiles ) | |||
fileNames << fileName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't we need this anymore?
QDir::toNativeSeparators( QDir::cleanPath( QFileInfo( fileName ).absoluteFilePath() ) );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still do. It's done 3 lines later, in the for loop for every filename in fileNames
.
In the rest of the code, I only deal with fileNames and no longer fileName.
* \see setStorageUrlExpression() | ||
* \since 3.22 | ||
*/ | ||
QgsExpression *storageUrlExpression() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following the discussion here #44533 (comment)
indeed in other places, expression are returned by pointer.
any suggestion/objection @nyalldawson
a2f2977
to
add8818
Compare
add8818
to
7d89628
Compare
@troopa81 ok to merge? |
@3nids I had to rebase, but once everything is green yes! |
@3nids everything is green. OK to merge |
This is the following of #43882
It provides UI elements to deal with external storage backends
Here is a little demo of how it works
The demo use a Netcloud server with a Webdav backend. This last will come in a separate PR.
Few optimisations/improvments will come in separate PR (don't fetch what we just store, deal correctly with folder...) because I don't want to make this one to big to review.
EDIT: Directory selection with an external storage is a complicated matter so for now, Storage mode (File or Directory) widget is not visible when external storage is selected. Same for relative path which has no meaning with external storage.
cc @Jean-Roc
cc @3nids for review
Funded by Lille Metropole