Dev add to map btn

[vermeulendivan]

This mostly focusses on a new feature which allows the user to add a downloaded layer to the QGIS canvas. Other changes includes the following:

• Buttons has been moved to be at the bottom of each element in the orders list, as apposed to on the right;
• Buttons are next to each other, and no longer stacked on each other;
• The layer will be added as BGR (true colour) and not RGB; and
• Error checking. For instance, if the manifest.json file is missing, or if the image it tries to load is missing, the user will be presented with an error message. This can happen if the downloading unexpectedly stopped, or if the user deleted/moved the downloaded data.

[vermeulendivan]

@john-dupuy Not sure why the download test needs to be added (or why it needs to be part if this PR). The section of code that does the downloading has not been implemented by me, and the 'Add to map' button does not change that in any way and does none of the downloading. The manifest file is downloaded, its not something I created. It contains info on the downloaded images, including directories and file names. It might be best for you to show me the other sources at our next meeting.

[Samweli]

@vermeulendivan I was expecting to also see the changes made to .pre-commit-config.yaml in this PR, but I can't find them.

[john-dupuy]

@john-dupuy Not sure why the download test needs to be added (or why it needs to be part if this PR). The section of code that does the downloading has not been implemented by me, and the 'Add to map' button does not change that in any way and does none of the downloading. The manifest file is downloaded, its not something I created. It contains info on the downloaded images, including directories and file names. It might be best for you to show me the other sources at our next meeting.

Apologies if I was unclear. I wasn't suggesting that you should create a download test, that test already exists. I was suggesting that you could add a check for the "Add to map" button to the order download test since most of the test setup will be the exact same. You could also just create a new test for it.

E.g. after this line you could add a small check that clicks the button and verifies that it has been added to the map.

[vermeulendivan]

@john-dupuy Sorry, I misunderstood you. Yes, that makes sense to add the test after the suggested line. Will add it to my tasks. On the issue with adding the data from other sources, is it possible that you can send me links with all cases which I can download? I just want to be 100% sure that I don't miss something and I think this is the most robust way going at it. Just a heads-up, I'm on leave next week so will likely get to it the following week.

[john-dupuy]

@john-dupuy Sorry, I misunderstood you. Yes, that makes sense to add the test after the suggested line. Will add it to my tasks.

Sounds good!

On the issue with adding the data from other sources, is it possible that you can send me links with all cases which I can download?

It's not so much the sources that matter but more so the name of the order. For the orders API, name is a free string field so we shouldn't assume it corresponds to some pattern. Basically, we need to figure out a more general way to determine the manifest_dir.

I see we're using the download_folder function - https://github.com/planetlabs/qgis-planet-plugin/blob/master/planet_explorer/gui/pe_orders_monitor_dockwidget.py#L174. I think it's safe to assume that there's always only a single folder in that directory. So we can just grab that directory name and then look for the manifest file in there. If there's more than a single folder in the directory (I'm pretty sure this won't happen unless a user manually places multiple orders in the directory), we can raise a warning and say that we weren't able to find the manifest file (as you do now).

[vermeulendivan]

@john-dupuy Thanks for the feedback! And sorry for taking so long to get back to you, were on leave. Your suggested change with using the single folder makes it much easier, did not know it will always be only one folder.

[vermeulendivan]

@john-dupuy I've added a test function about a week ago, but would prefer @Samweli to have a look once he gets back from leave (next week). Also had a chat with Asa a few weeks ago about disabling the 'Add to map' button when the image has been added, and he agreed with my reasoning to rather not disable it. Basically, there can be several cases where a user wants to add a layer more than once (e.g. create several group layers, use the image for a basemap and an inset map, etc.).

[john-dupuy]

Also had a chat with Asa a few weeks ago about disabling the 'Add to map' button when the image has been added, and he agreed with my reasoning to rather not disable it. Basically, there can be several cases where a user wants to add a layer more than once (e.g. create several group layers, use the image for a basemap and an inset map, etc.).

Totally makes sense - thanks for the info!

[john-dupuy]

@vermeulendivan testing this out locally and I noticed that for orders that have more than a single asset - it looks like only the first asset is able to be added to the map.

I think we should loop through all assets in the order and add each as a separate layer.

I'd be ok merging this in and doing that (and fixing the tests) as a follow up.

[vermeulendivan]

@john-dupuy Are you referring to the single folder as discussed here: https://github.com/planetlabs/qgis-planet-plugin/pull/77#issuecomment-1327632330? Otherwise I'm not sure what you are referring to. I tested a bunch of data that I got from the search results, but all of them has been structured in the same manner as I've put it together. It might be that I don't have access with the credentials that I have, and then it only shows specific data to me.

[vermeulendivan]

@john-dupuy Just popped into my mind, are you maybe referring to those "udm" and "udm2" rasters?

[john-dupuy]

@john-dupuy Just popped into my mind, are you maybe referring to those "udm" and "udm2" rasters?

Hey @vermeulendivan - nope, I'm not referring to the UDM. It's possible to submit an order for up to ~1000 different scenes/assets. The number of assets in the order is shown in the Orders Monitor with the "Asset count" value.

Example is in the gif below, the "Add to map" button will only add one of the SkySatScenes to the map (0039) whereas I think it should add both (0038 and 0039) in separate layers. Gif of what I'm talking about:

Kazam__00004.webm

[vermeulendivan]

@john-dupuy Thanks for the feedback and visual example, really helps a lot. All of the cases I've downloaded are single asset, got one with 5 assets and set the order. Will download as soon as its available - need an example to work with. Should be an easy fix.

[vermeulendivan]

@john-dupuy Fixed the bug with the multiple assets. All assets will now be loaded correctly into QGIS:

[john-dupuy]

Awesome work Divan - tested locally and it's working for me!

[astrong19]

Thanks John for catching!

[Samweli]

LGTM - @Samweli do you want to take a second look pre-merge?

@john-dupuy I have tested it, looks good! thanks, @vermeulendivan can you make changes for the review comments I made above then we will be good for a merge.

[john-dupuy]

@vermeulendivan can you fix the linting? Can be done via pre-commit run --all-files in your local branch.

[vermeulendivan]

@john-dupuy Not sure why its failing. Did try and run the pre-commit earlier. Everything always passes: Also ran black separately, same.

I did message @Samweli about it earlier, because I don't understand why its fine locally but in the PR it keeps failing - haven't received a response yet.

Also made no changes to the files being referred to:

[john-dupuy]

@john-dupuy Not sure why its failing. Did try and run the pre-commit earlier.

Huh, yeah I checked out your PR locally and I'm also unable to reproduce the pre-commit failure :confused:

[john-dupuy]

@john-dupuy Not sure why its failing. Did try and run the pre-commit earlier.

Huh, yeah I checked out your PR locally and I'm also unable to reproduce the pre-commit failure confused

@vermeulendivan Ah, it was because this branch was out of date with master - rebasing and rerunning pre-commit fixed it. I just pushed up the commit.

[vermeulendivan]

@john-dupuy Great! Thanks John.. Will keep in mind for such cases that can be the cause.