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

OGR: add a download option to the protocol data source #8985

Merged
merged 6 commits into from Jan 28, 2019

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Jan 25, 2019

I'm not sure whether this is is a feature, looks more like a bugfix to me, btw needs docs

screenshot_20190125_154753

Fixes #21091

The issue was due to the /vsicurl/ not
being able to open a streaming endpoint.

“Things are not always what they seem; the first appearance
deceives many; [...]”

― Phaedrus

Fixes qgis#21091

The issue was due to the /vsicurl/ not
being able to open a streaming endpoint.

“Things are not always what they seem; the first appearance
deceives many; [...]”

― Phaedrus
@elpaso elpaso added the Bugfix label Jan 25, 2019
@elpaso elpaso added this to the 3.6.0 milestone Jan 25, 2019
@nirvn
Copy link
Contributor

nirvn commented Jan 25, 2019

Hmm I'm uncertain about this approach. In terms of UI implementation, seems like it'd deserve some visual progress report here, does qgsfiledownload do that?

More fundamentally, this essentially downloads a dataset into a temp location, adds it to the project, and the data will be lost when relaunching QGIS assuming temp location is regularly wiped.

Even if we'd be able to find a way to make these downloaded datasets permanent, it seems to defeat the overall feature here, that is having a layer with a remotely accessed dataset (which can be updated across sessions etc.)

What about just popping up a "this dataset can't be opened remotely, use a download manager to copy it locally"?

@elpaso
Copy link
Contributor Author

elpaso commented Jan 25, 2019

Hmm I'm uncertain about this approach. In terms of UI implementation, seems like it'd deserve some visual progress report here, does qgsfiledownload do that?

Yes, it can be added easily (with abort button too).

More fundamentally, this essentially downloads a dataset into a temp location, adds it to the project, and the data will be lost when relaunching QGIS assuming temp location is regularly wiped.

Even if we'd be able to find a way to make these downloaded datasets permanent, it seems to defeat the overall feature here, that is having a layer with a remotely accessed dataset (which can be updated across sessions etc.)

I see your point, what about using a file selector dialog to choose the download location instead of a temp dir?

What about just popping up a "this dataset can't be opened remotely, use a download manager to copy it locally"?

Well, that would have been much faster to implement, just change the error message :)

@nirvn
Copy link
Contributor

nirvn commented Jan 25, 2019

:) IMHO, I'd implement an error message for 3.6. If you are interested in investing more time to come up with a way to essentially creating a [x] download and open a local copy of the dataset option, that could be useful for users you want to skip opening their browsers or download manager. I'd make sure the wording clearly explains the resulting layer won't connect remotely.

@elpaso elpaso added the Requires Changes! Waiting on the submitter to make requested changes label Jan 25, 2019
@elpaso
Copy link
Contributor Author

elpaso commented Jan 26, 2019

Some more thoughts here:

  1. from user perspective the download should be an available option (just press "Add" and the layer is added to the canvas), much better than fail with a message
  2. the message option is not easy to implement: the failure happens when the control has been already passed from the data source manager dialog to the app and from the app to the provider, the message is displayed in the message bar which is greyed out in the background (and times out too!).
  3. the download approach will not work with a directory (not sure about what vsicurl does with this regards though)
  4. I fully agree that the download option is substantially different than adding an online data source because the data source is changed from an online origin to a locally stored file, but this behavior will originate from a user choice so it essentially becomes a matter of choosing the right wording and GUI to communicate clearly.

That said: I don't know what to do here: the simpler solution of changing the message will actually change nothing because of the limited visibility of the message itself.

@nirvn
Copy link
Contributor

nirvn commented Jan 26, 2019

@elpaso , some thoughts over your thoughts 😉 :

  • This new option should be renamed to something along the lines of "download and use a local copy of the dataset", which makes it clear that this would effectively be a shortcut to downloading the dataset via other means;
  • IMHO, it can't be a download put in the temporary folder, otherwise we'd have to have the option say "download and use a temporary local copy of the dataset", which I don't see much use for UX-wise, meaning the implementation would need to ask users for a folder to save the dataset into;
  • On the UX front, I still wonder how this improves the error message. Since this option would have to be off by default, users would still try to stream the dataset, notice it's failing, then (I assume) give it a try again with this new option... we'd still be back to needing to improve the error reporting to users to begin with;
  • On that error reporting front, could we do a simple streaming check before opening the actual layer via vsicurl? That way, the user could be informed that the remote server isn't streaming-friendly;
  • In that "remote server doesn't support streaming" error dialog, you could have a checkbox that allows for the user to download the dataset and open it locally.

... and forward all app's message bar messages to
the dialog's message bar in case it is modal and
visible.

Also adds a question dialog when a /vsicurl/
add layer operation failed and offers a chance
to try the "normal" opening mode.
@elpaso elpaso removed the Requires Changes! Waiting on the submitter to make requested changes label Jan 26, 2019
@elpaso
Copy link
Contributor Author

elpaso commented Jan 26, 2019

@nirvn I've found the solution: the failed URL opens without issues when the URL is pasted in the "File" source instead of the "Protocol" one. So, I added a question dialog in case the protocol URL opening failed, the question dialog asks the user if she wants to try the "normal" way, if the user answers "Yes" the normal way is triggered (and it will probably work).

Note that the "normal wayl" does not download and create a local datasource like my first implementation, there is no confusion anymore.

I've also added a messageBar inside the data source manager dialog and if and only if the dialog is modal AND is visible, the messages from the app will be forwarded to the dialog's messageBar so that the user can actually inspect the messages and stop the timeout.

@@ -1853,6 +1854,21 @@ bool QgisApp::event( QEvent *event )
return done;
}


QgsMessageBar *QgisApp::visibleMessageBar()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I quite like this. I think there's scope to use more in future -- e.g. vector/raster layer properties dialogs currently trigger message bar warnings in the main canvas, so they are a potential use case for this in future.

@nirvn
Copy link
Contributor

nirvn commented Jan 28, 2019

@elpaso , thanks for investigating further and coming up with an alternative, much better. +1 from me.

@elpaso
Copy link
Contributor Author

elpaso commented Jan 28, 2019

@nirvn thank you for your inputs!

@elpaso elpaso merged commit 1f8708f into qgis:master Jan 28, 2019
@elpaso elpaso deleted the bugfix-21091-ogr-protocol-download branch January 28, 2019 07:11
@elpaso elpaso changed the title OGR: add a download option to the protocol data source [needs-docs] OGR: add a download option to the protocol data source Jan 28, 2019
elpaso added a commit to elpaso/QGIS that referenced this pull request Feb 6, 2019
…wnload

OGR: add a download option to the protocol data source [needs-docs]

Cherry-picked from master 1f8708f
nyalldawson pushed a commit that referenced this pull request Feb 6, 2019
OGR: add a download option to the protocol data source [needs-docs]

Cherry-picked from master 1f8708f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants