planetlabs / qgis-planet-plugin

Browse, filter, preview and download Planet Inc imagery in QGIS.
https://developers.planet.com/docs/integrations/qgis/
GNU General Public License v2.0
45 stars 12 forks source link

Add STAC as a checkout option #67

Closed Samweli closed 1 year ago

Samweli commented 1 year ago

These changes enabled using STAC metadata as an option when making orders.

The current behaviour is user enable the STAC metadata option by clicking STAC button and can disable it by re-clicking the same button. This behaviour is per delivered from the below mockup. image

Although this achieves the intended outcome, I'm advising to use a checkbox instead of a button and will make the change if it is confirmed.

Screenshot stac_checkout_option

john-dupuy commented 1 year ago

@Samweli can you add a test for this new feature? It can go in https://github.com/planetlabs/qgis-planet-plugin/blob/master/planet_explorer/tests/test_orders.py

john-dupuy commented 1 year ago

@Samweli looks like ERROR planet_explorer/tests/test_orders.py::test_order_scene is hitting an error on QGIS 3.26 - can you investigate that? I seem to recall this version of QGIS giving me a bunch of trouble when initially writing the order test.

Samweli commented 1 year ago

@Samweli looks like ERROR planet_explorer/tests/test_orders.py::test_order_scene is hitting an error on QGIS 3.26 - can you investigate that

Yes, @john-dupuy I suspect that this is an error that is related to the changes introduced by this PR but still I haven't found on why it only occurs on 3.26 version. I ll continue to investigate and get back to you, thanks.

astrong19 commented 1 year ago

Hey! I had some UI design feedback on the STAC metadata tool.

1.) Could we use the same font and style as the Harmonization and Clip tools... for example, can we Bold METADATA

2.) Could we move STAC above the Review Items section so that it is under Harmonization and Clip

3.) Can we hyperlink the word "STAC" in the description to https://stacspec.org

4.) Can we make the description under metadata have more space so that it is more similar to the descriptions for Harmonization and Clip?

All of these suggestions are basically the same as saying please copy the example form the ArcGIS Pro Add-In that is shown above

STAC feedback
Samweli commented 1 year ago

@astrong19 The latest commit contain UI updates you have requested on the past comment, see below the new look of the Order dialog

image

john-dupuy commented 1 year ago

OK so I tested this locally and can confirm it's working - I just want to confirm the UI. On Linux:

Ordering STAC metadata Screenshot from 2023-01-31 15-34-48

Not Ordering STAC metadata Screenshot from 2023-01-31 15-35-45

IMO they should be flipped, i.e. the filled button should be for when you are getting STAC metadata.

john-dupuy commented 1 year ago

OK so I tested this locally and can confirm it's working - I just want to confirm the UI. On Linux:

IMO they should be flipped, i.e. the filled button should be for when you are getting STAC metadata.

Looking at the code and I see that this is the intention - however, I think there is a bug where the order dialog "remembers" what the previous selected option was. So if you go through and order an item with STAC enabled once, it will enabled the next time you order that item, regardless of the button's styling.

We should reset this button style when the order dialog is opened. FYI @astrong19 @Samweli

Samweli commented 1 year ago

We should reset this button style when the order dialog is opened. FYI @astrong19 @Samweli

however, I think there is a bug where the order dialog "remembers"

@john-dupuy It is not bug, that how I designed it to work that once user select/un-select the STAC Metadata option then the plugin should always remember that selection so they don't have to click the option each time they open the Order dialog.

We can change it if you and @astrong19 agree that we should change the workflow, but in my opinion it is usually better to save user defined options so they won't have to reselect them each time.

john-dupuy commented 1 year ago

We should reset this button style when the order dialog is opened. FYI @astrong19 @Samweli

however, I think there is a bug where the order dialog "remembers"

@john-dupuy It is not bug, that how I designed it to work that once user select/un-select the STAC Metadata option then the plugin should always remember that selection so they don't have to click the option each time they open the Order dialog.

We can change it if you and @astrong19 agree that we should change the workflow, but in my opinion it is usually better to save user defined options so they won't have to reselect them each time.

@Samweli The issue is not really the "remembering" but rather the fact that the disabled/enabled styling for the button can become flipped. So the user will press the button and think they've selected STAC metadata to be delivered, but the order will not contain any STAC metadata.

Samweli commented 1 year ago

@Samweli The issue is not really the "remembering" but rather the fact that the disabled/enabled styling for the button can become flipped

Right @john-dupuy, I ll remove the "remembering" logic that should resolve the issue. Do you think we should use a normal checkbox like in the other options, I do realize that this can be against the shared UI for this feature but all the other options just use a checkbox.

john-dupuy commented 1 year ago

@Samweli The issue is not really the "remembering" but rather the fact that the disabled/enabled styling for the button can become flipped

Right @john-dupuy, I ll remove the "remembering" logic that should resolve the issue. Do you think we should use a normal checkbox like in the other options, I do realize that this can be against the shared UI for this feature but all the other options just use a checkbox.

@Samweli afaik @astrong19 would like to keep the button as is - I say we remove the remembering logic and revisit making it a check box after the fact.

astrong19 commented 1 year ago

Talking to the team a bit today I think we should actually change the UI for the STAC metadata checkout to a checkbox so that it's more consistent and less confusing. Also we can add an option to the plugin settings to enable STAC by default. The text can be "Enable STAC metadata by default"

settings
Samweli commented 1 year ago

Talking to the team a bit today I think we should actually change the UI for the STAC metadata checkout to a checkbox so that it's more consistent and less confusing. Also we can add an option to the plugin settings to enable STAC by default. The text can be "Enable STAC metadata by default"

Right @astrong19, working on adding this to the current PR.

Samweli commented 1 year ago

Thanks @Samweli - tested locally and the checkbox is working.

Alright @john-dupuy thanks, I have also updated the settings dialog to include the STAC metadata tool option, see below Workspace 1_005

astrong19 commented 1 year ago

Looks good to me, thanks!