QGIS Quick library [dockerdeps] #6490
Conversation
This adds a new library for creation of applications based on Qt Quick framework. It contains reusable QML / Qt Quick components based on QGIS core library. The initial work introduces MapCanvas, FeatureForm, ScaleBar and various other components. To enable compilation of the library, use WITH_QUICK=TRUE Further documentation of the library is located in doc/qgsquick.dox For background information see the associated QEP: qgis/QGIS-Enhancement-Proposals#109 The initial implementation is largely based on the work of Matthias Kuhn and Marco Bernasocchi on QField probject - kudos to them for the great job!
|
Have you considered building this on Travis too? If you want to build it, you'll have to adapt the dependencies Dockerfile and the cmake command |
|
Phenomenal stuff! There's a lot of code here -- not sure who/how this should be reviewed. Is there areas in particular you'd like someone to look over? |
|
!!!WIN!!
…On Thu, Mar 1, 2018 at 9:13 AM, Nyall Dawson ***@***.***> wrote:
Phenomenal stuff!
There's a lot of code here -- not sure who/how this should be reviewed. Is
there areas in particular you'd like someone to look over?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#6490 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAXS3AOINycJb7EdZIqmGD97A2rq2Dceks5tZd2agaJpZM4SW1oh>
.
|
|
Nice one. I'll definitely have a look at this, I hope I find the time soon. But (at least) another pair of eyes on this would be great as well ! |
|
@PeterPetrik I added a small trick so that the Docker deps image is saved on the hub (thanks to having |
3nids
force-pushed
the
qgis-quick-library
branch
from
8ef061c
to
5b31975
Compare
|
@3nids nice! thanks |
3nids
force-pushed
the
qgis-quick-library
branch
from
5d75a55
to
438b2f7
Compare
|
Sorry for the mess, it's a bit more complicated than I thought (Travis yml file doesn't support having complex definition of variables). Will see if I can address this in a separate PR. |
|
Hi @m-kuhn, do you have any other outstanding issues with this PR? I have seen QField release-0.10.12 4 days ago and I am concerned that this work will diverge from QField. There is also PR for QField that uses all QgsQuick classes instead of their QField 1-1 equivalents and PR for osgeo4A to package QgsQuick. |
|
@PeterPetrik as already communicated by mail, the size and diversity in the code make it a non-trivial job to review this PR. I am quite happy with the infrastructure (library, cmake, plugin code) and would merge those immediately if they were in a self-contained PR. |
There was a problem hiding this comment.
First batch of additional comments.
Some of them are things that would be good to grep for in your code, I probably didn't find all their ocurrences (QDebug, usage of gps and btn) or do an internal review on your side (comprehensive usage of translatable strings)
| property bool allowRememberAttribute: false // when adding new feature, add checkbox to be able to save the same value for the next feature as default | ||
| property QgsQuick.Project project | ||
|
|
||
| property var saveBtnIcon: QgsQuick.Utils.getThemeIcon( "ic_save_white" ) |
There was a problem hiding this comment.
Either use saveIcon or saveButtonIcon
|
|
||
| property QgsQuick.AttributeFormModel model | ||
| property alias toolbarVisible: toolbar.visible | ||
| property bool allowRememberAttribute: false // when adding new feature, add checkbox to be able to save the same value for the next feature as default |
There was a problem hiding this comment.
Is there a plan to do something doxygen-alike for qml documentation? Would be great if we had a stable format for api docs right from the beginning.
There was a problem hiding this comment.
Would be great, but it would take a bit of investigation on how to do it properly, what tool to use to generate the docs etc. I would rather keep it for future enhancement
Similar topic is code-style for qml files (astyle)
There was a problem hiding this comment.
Ok. I did a quick search and I think Qt does it via .qdoc files (not within the qml files). Let's keep eyes open and postpone this.
| */ | ||
| Rectangle { | ||
| width: parent.width | ||
| height: parent.height |
There was a problem hiding this comment.
What's the reason to use width and height instead of anchors.fill?
| property var widget: EditorWidget | ||
| property var field: Field | ||
| property var constraintValid: ConstraintValid | ||
| property var homePath: form.project ? form.project.homePath : "" |
There was a problem hiding this comment.
Wouldn't it be better to use null/undefined instead of an empty string if the project is not available? I can't think of a situation, where it's desired to silently assume that the homepath is an empty string.
| property var homePath: form.project ? form.project.homePath : "" | ||
|
|
||
| active: widget !== 'Hidden' | ||
| source: 'qgsquick' + widget.toLowerCase() + '.qml' |
There was a problem hiding this comment.
Is it not required to specify the subdirectory here (editor)? How are they found?
There was a problem hiding this comment.
it ends up without subdirectory in the output, see https://github.com/PeterPetrik/QGIS/blob/qgis-quick-library/src/quickgui/plugin/CMakeLists.txt#L94
There was a problem hiding this comment.
Feels a bit like black magic to me. Especially since this is targetted at standalone apps which will probably ship their own packaging code.
| //! Feature fieds's roles | ||
| enum FeatureRoles | ||
| { | ||
| ElementType = Qt::UserRole + 1, //!< Element type |
There was a problem hiding this comment.
Those documentation are pretty much stating the obvious... In general it's preferable to add a bit more than copying the name.
|
|
||
| Q_ENUM( FeatureRoles ) | ||
|
|
||
| //! create new attribute form model |
| : QStandardItemModel( 0, 1, parent ) | ||
| , mFeatureModel( nullptr ) | ||
| , mLayer( nullptr ) | ||
| , mTemporaryContainer( nullptr ) |
|
|
||
| QgsQuickAttributeFormModelBase::~QgsQuickAttributeFormModelBase() | ||
| { | ||
| delete mTemporaryContainer; |
| Q_INVOKABLE void create(); | ||
|
|
||
| //! Return attribute value with name | ||
| Q_INVOKABLE QVariant attribute( const QString &name ); |
| QSGNode *QgsQuickFeatureHighlight::updatePaintNode( QSGNode *n, QQuickItem::UpdatePaintNodeData * ) | ||
| { | ||
| if ( !mDirty || !mMapSettings ) | ||
| return n; |
There was a problem hiding this comment.
The original code has a single return statement at the end of the method. This prevents from accidentially introducing memory leaks, especially if the code deals with new and delete as here. Plus it allows copy elison of the return value on any compiler. Is there a good reason to change the code from this schema?
There was a problem hiding this comment.
But here we return a pointer, not returning a value, so I am not sure where the copy elision would be used here...?
My reason for changing the code was that flat code is better than nested code.
There was a problem hiding this comment.
Yep, I don't think elison will be triggered in this particular case here.
Just saying, this was a consciuos design choice I did here concerning safety (memory leaks) and a style that will lead to elison with every recent compiler (in suitable scenarios). I'm not sure "better" code is better here.
There was a problem hiding this comment.
Hmm sorry I fail to understand the point about safety (+elision) ... I must be missing something.
There was a problem hiding this comment.
I spent some thoughts on this again, in-depth explanations follow further down. I'm ok with leaving that as is. But hope that such changes are not very widespread within this pull request because they make reviewing quite a bit harder in this huge PR here, which is also the reason why I reacted a bit picky.
Safety
func()
{
do_something();
// if some in the future adds a new here
while ( i )
{
if (some_condition()) return;
}
// and a matching delete which he expects to be executed
// we've got a memory leak
}
However I meanwhile tend to accept that guard conditions at the beginning of a method (as in this case) are acceptable as they are less prone for such misinterpretation.
Elision:
Just read a bit more on that. My previous statement was based on a quote I had in mind (stack overflow or so) "all recent compilers support copy elision if you have a single variable definition at the beginning of the function and a single return statement.". That's not entirely true (probably my memory is not perfect ;) ), NRVO can also work with multiple exit points, but it has to be done consistently. That was mainly a reason for the "single exit point" in general and not regarding this particular example.
|
|
||
| // TODO: this is very crude conversion! QgsQuickHighlightsNode should accept any type of geometry | ||
| QVector<QgsPoint> points; | ||
| for ( auto it = geom.vertices_begin(); it != geom.vertices_end(); ++it ) |
There was a problem hiding this comment.
Is there a const version for this?
There was a problem hiding this comment.
There is only a const version...
|
I see there are a lot of very good approaches in this pull request and I really like that there are unit tests coming right along with it 👍 Due to the current state of this code and the sheer size of this pull request, I'd like to re-raise the question if we can split this up into batches. I think it shouldn't be too hard. I must admit, I feel a bit uncomfortable with this review because of several reasons which have been outlined already. Would it be possible to switch to a different merge and review schema here? |
|
Thank you for the review @m-kuhn. In order to speed up the review process, I will create multiple PRs as proposed. But I am afraid I do not have time to modify associated QField PRs. Do you want to address the issues you raised from the second review in this PR, or in the new PRs? |
|
Thanks @PeterPetrik this sounds good 👍 I would suggest to address the review comments and then opening the new pull requests with the adapted code. No worries for QField, I don't think it makes sense to adjust code there before all the pull requests here are locked in. |
|
Addressed the review comments and opening the new pull requests with the adapted code. Closing this one |
This pull request is a subset of qgis#6490 This adds a new library for creation of applications based on Qt Quick framework. It contains reusable QML / Qt Quick components based on QGIS core library. The initial work introduces MapCanvas To enable compilation of the library, use WITH_QUICK=TRUE Further documentation of the library is located in doc/qgsquick.dox For background information see the associated QEP: qgis/QGIS-Enhancement-Proposals#109 The initial implementation is largely based on the work of Matthias Kuhn and Marco Bernasocchi on QField probject - kudos to them for the great job!
[FEATURE] Introduction of QGIS Quick library
This adds a new library for creation of applications based on Qt Quick
framework.
It contains reusable QML / Qt Quick components based on QGIS core library.
The initial work introduces MapCanvas, FeatureForm, ScaleBar and various other
components.
To enable compilation of the library, use WITH_QUICK=TRUE
Further documentation of the library is located in doc/qgsquick.dox
For background information see the associated QEP:
qgis/QGIS-Enhancement-Proposals#109
The initial implementation is largely based on the work of Matthias Kuhn
and Marco Bernasocchi on QField project - kudos to them for the great job!
Also thanks to my colleagues Martin Dobias and Viktor Sklencar.
Work co-funded by Yorkshire Dales National Park.