kbgg / qgis-stac-browser

Plugin for QGIS to browse and download assets from STAC APIs
Apache License 2.0
26 stars 9 forks source link

Various fixes and some added features #58

Closed suricactus closed 5 years ago

suricactus commented 5 years ago

I really enjoy the existence of this plugin! Finally some convenience in this world. I've fixed a number of bugs that I've encountered, and also added some nice to have features. I hope the commit messages are clear enough and the code good enough to be merged.

kbgg commented 5 years ago

Thank you for your PR! These look like some awesome additions and I'll be happy to merge them. Just a few things need to be changed:

Thank you!

suricactus commented 5 years ago

I think I now comply with the three bullets you listed above. However, there is a conflict with your fix to disable the button, I think mine solution fixes more cases.

IMHO Changing the linter to accept lines that accept at least 120 characters would improve the overall readability of the code, as we are dealing with QT and QGIS interface, which tend to be quite verbose. For example: https://github.com/suricactus/qgis-stac-browser/blob/fixes/controllers/query_dialog.py#L26-L28 .

I will put a few more "controvesial" suggestions in the issue list, I'll be happy to implement them if you agree they are needed. Adding filters is already on my TODO list.

kbgg commented 5 years ago

However, there is a conflict with your fix to disable the button, I think mine solution fixes more cases.

Merged in your fix

IMHO Changing the linter to accept lines that accept at least 120 characters would improve the overall readability of the code, as we are dealing with QT and QGIS interface, which tend to be quite verbose. For example: https://github.com/suricactus/qgis-stac-browser/blob/fixes/controllers/query_dialog.py#L26-L28 .

Good point, I'll create a ticket for updating the linter to accept lines to 120 characters.

I will put a few more "controvesial" suggestions in the issue list, I'll be happy to implement them if you agree they are needed. Adding filters is already on my TODO list.

Keep the suggestions coming! The best way to have a great plugin is to work collaboratively

suricactus commented 5 years ago

Wohoo, happy to see my PR merged. One more thing I forgot to mention is to check if "View in external image viewer" button works on Windows/Mac. I've checked it on Linux and seems to be fine. According to the documentation, this is the official way to invoke it, but for such API some empyrical confirmation is better.

kbgg commented 5 years ago

I checked it on Mac and it works there. We'll have to update some processes for testing on different platforms before we cut a release.