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

[quick] Brand new elevation profile canvas item #51347

Merged
merged 1 commit into from Jan 30, 2023

Conversation

nirvn
Copy link
Contributor

@nirvn nirvn commented Jan 1, 2023

Description

This PR adds a brand new elevation profile canvas item to QGIS' quick toolbox. Here's how it looks in action:
image

The new item has a couple of properties to set things up including a project and a crs. When those properties are set, an invokable function, populateLayersFromProject(), is used to populate the item with layers from the project property. Then, the elevation profile canvas can be rendered by setting the profileCurve property (a QgsGeometry object). Et voila, you got a nice elevation profile using the provided project's symbology settings.

It's been tested for a couple of months in QField and has proven to be stable. Happy new year! :)

@github-actions github-actions bot added this to the 3.30.0 milestone Jan 1, 2023
@github-actions github-actions bot added the Quick Quick GUI widgets label Jan 1, 2023
@nirvn nirvn added the Feature label Jan 1, 2023
@nirvn nirvn force-pushed the quick_profiling branch 3 times, most recently from 36ec1e4 to 9f4d101 Compare January 2, 2023 02:24
@nyalldawson
Copy link
Collaborator

@nirvn
any chance we can hold off on this one for a couple of days? I'm working now on the elevation profile chart item for layouts, and I strongly suspect I'll be adding some helper methods/classes as part of that which will simplify your new classes!

@nirvn
Copy link
Contributor Author

nirvn commented Jan 13, 2023

@nyalldawson , sounds good.

@github-actions
Copy link

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular

    • link to any issues which this pull request fixes

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in a week.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 28, 2023
@nirvn
Copy link
Contributor Author

nirvn commented Jan 28, 2023

Just courteously waiting ;)

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 28, 2023
@nirvn
Copy link
Contributor Author

nirvn commented Jan 30, 2023

@wonder-sk , @PeterPetrik , I'll merge this later this afternoon if you guys don't plan to review it.

@wonder-sk
Copy link
Member

@nirvn please feel free to merge...

(By the way, ~2 months ago we have dropped quickgui in Mergin Maps Input as a part of transition to CMake, as it was getting complicated to keep the machinery working to stay in sync with quickgui for the few files in the lib, and slowing down the development when some quickgui changes were necessary...)

@nirvn
Copy link
Contributor Author

nirvn commented Jan 30, 2023

@wonder-sk , ok, good to know. Are you guys still trying to maintain that nice qgsquick overlap in the Venn diagram of QField & Input?

@nirvn nirvn merged commit 58b1e26 into qgis:master Jan 30, 2023
@wonder-sk
Copy link
Member

Sorry the qgsquick files got blended into the rest of the Input app now...

@nirvn
Copy link
Contributor Author

nirvn commented Jan 31, 2023

@wonder-sk , ouch; are you guys planning to upstream code from now on?

@nirvn
Copy link
Contributor Author

nirvn commented Jan 31, 2023

@wonder-sk , beyond the question on upstreaming effort, I have to say this is a rather sad development. It feels like it was just yesterday that we had all agreed to commit to continue maintaining a reduced set of core classes across apps. Is there no way to reconsider this decision on Lutra’s part? The QgsQuickMapCanvas class for e.g. isn’t one that is bound to specific UI/UX, these foundational stones were never going to diverge from one project to another.

I also take note of the complete absence of acknowledgment/mention of the origin of the source code in the renamed classes (e.g. QgsQuickMapCanvas to InputMapCanvasMap), which isn’t a very polite move to do. While you guys can certainly fork away, it would be nice to maintain a form of acknowledgment of the QGIS community as the foundation of the classes being forked.

@wonder-sk
Copy link
Member

@nirvn

are you guys planning to upstream code from now on?

I don't think there are going to be any upstream updates to quickgui from Lutra... unfortunately.

I have to say this is a rather sad development. It feels like it was just yesterday that we had all agreed to commit to continue maintaining a reduced set of core classes across apps.

It is sad for me too. I have done a lot of effort on trying to make QGIS Quick happen, to become another common QGIS library that other apps could use - and maybe one day eventually QGIS desktop app itself. But I was probably too naive - desktop QGIS does not seem anywhere close to adopting QML/Quick for its GUI (and in hindsight, that's probably good, as QML can be quite a mess). At Lutra I was always pushing to build this common library, even though that meant more hassle in the development. After the switch to the "slim" QGIS Quick library back in May 2021, it didn't really improve development workflow of our team - we still had to track changes to it in master, keep a more complicated build system and upstream our changes - all that for 9 moderately sized files. The team did not like to have this extra complexity for the little benefit, so when switching to CMake back in November, it was decided to drop this dependency and to make the life easier for our team...

I do not see much future for the QGIS Quick library anymore unfortunately - it turned out it was not working great for QField nor Input, and it never got any traction from any other party - so I am not sure if it is worth keeping it alive with further efforts. I admit it wasn't the best idea to start it in the first place...

And I am sorry we have not communicated that earlier, we should have, to avoid any surprises...

I also take note of the complete absence of acknowledgment/mention of the origin of the source code in the renamed classes (e.g. QgsQuickMapCanvas to InputMapCanvasMap), which isn’t a very polite move to do. While you guys can certainly fork away, it would be nice to maintain a form of acknowledgment of the QGIS community as the foundation of the classes being forked.

Apologies - this was definitely not meant as a bad intent, it only happened while making the headers use the same template as the other files without much extra thought. Peter has now brought the mentions back in MerginMaps/mobile#2540 - we definitely want to acknowledge QGIS community for the foundations!

@PeterPetrik
Copy link
Contributor

Thank you Martin for expressing nicely what I feel. And it was definitely not meant with bad intent, and sorry if I offended someone by (lack) of communication.

@zacharlie zacharlie added Changelog Items that are queued to appear in the visual changelog - remove after harvesting ChangelogHarvested This PR description has been harvested in the Changelog already. and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ChangelogHarvested This PR description has been harvested in the Changelog already. Feature Quick Quick GUI widgets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants