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

[FEATURE] QML chart and drawings widget #7801

Merged
merged 43 commits into from Sep 14, 2018
Merged

[FEATURE] QML chart and drawings widget #7801

merged 43 commits into from Sep 14, 2018

Conversation

@signedav
Copy link
Contributor

signedav commented Sep 5, 2018

New type of widget to show graphically appealing and interactive items like charts or technical drawings on the attribute form.

The configuration is reduced to the basics. Including example templates and adding expressions. This could be improved with wizards etc.

Looks like this:
qml2

@signedav signedav force-pushed the signedav:qml-widget branch from 1341674 to ae1333d Sep 5, 2018
};


class GUI_EXPORT QmlExpression : public QObject

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 5, 2018

Member

can this be put into the cpp file?
I would keep this as an implementation detail (and not public api)

{
QString name = elem.attribute( QStringLiteral( "name" ) );
int idx = d->mFields.lookupField( name );
newElement = new QgsAttributeEditorField( name, idx, parent );
}
else if ( elem.tagName() == QLatin1String( "attributeEditorRelation" ) )
else if ( elem.tagName() == QStringLiteral( "attributeEditorRelation" ) )

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 5, 2018

Member

I think QLatin1String is still recommended for comparisons like here

@nyalldawson nyalldawson added the Feature label Sep 5, 2018
@nyalldawson nyalldawson added this to the 3.4 milestone Sep 5, 2018
Removed QmlExpression from api
On activation of template box, we overwrite the text in the GUI
The preview is loaded including expression values on start
Takes expression instead of currentText from ExpressionWidget - means it delivers fields in quotes
signedav added 3 commits Sep 10, 2018
and changed to string of formMode in the attributeformcontext

void contextChanged();
%Docstring
Signal when QgsAttributeEditorContext mContext changed after the widget wrapper has already been intialized

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 11, 2018

Member

Is sending of this signal linked to the condition of widget wrappers being created? Or was it just the reason to introduce this signal?

This comment has been minimized.

Copy link
@signedav

signedav Sep 11, 2018

Author Contributor

Not sure if I understand your question. Ìt can be connected on creation of widget wrappers, in case they need to make an action. Like QgsQmlWidgetWrapper...

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 11, 2018

Member

I was mostly wondering why the part "after the widget wrapper has already been initialized" is in the doc string.

This comment has been minimized.

Copy link
@signedav

signedav Sep 11, 2018

Author Contributor

Ah, because the issue was, that the context has been set on the initialization, and in case it changed after, the widget has values containing the "deprecated" context still.

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 11, 2018

Member

So it's mostly a historical consideration?
In this case I don't think it's useful for future readers of this docs.

This comment has been minimized.

Copy link
@signedav

signedav Sep 11, 2018

Author Contributor

No not historical. E.g. the QgsQmlWidgetWrapper used the context to set values. On change of the context, these values has to be changed as well. That's why the signal is implemented, to trigger these changes, when the context changes after the Widget is initialized. If it changes before, then the inexistent Widged doesn't mind anyway. But probably it's too clear, so it only confuses...

@@ -55,12 +55,14 @@ class GUI_EXPORT QgsQmlWidgetWrapper : public QgsWidgetWrapper

public slots:

//! passes the \a feature into the context property of the widget
void setQmlContext();

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 11, 2018

Member

private?

@@ -38,6 +38,18 @@ class GUI_EXPORT QgsAttributeEditorContext
{
public:

//! Form modes
enum Mode

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 11, 2018

Member

This is copied from somewhere, correct? Would it be possible to replace the other one with this one in the long run and deprecate it for QGIS 4?

This comment has been minimized.

Copy link
@signedav

signedav Sep 11, 2018

Author Contributor

This should be removed again. I forgot after changing back saving the Mode as QString here. Another easy to change possibility would be:

  • keep this enum here, work here with enum instead of string for mAttributeFormMode
  • use in the QgsAttributeForm the enums from here
  • implement function modeString function here instead of QgsAttributeForm and remove there.

What should be prefered?

instead of QgsAttributeForm - because it's used on places not including QgsAttributeForm
now it's able to use @form_mode in expressions of containers and QML widgets
* Returns given \a attributeFormMode as string
* \since QGIS 3.4
*/
QString attributeFormModeString( const Mode &attributeFormMode )

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 11, 2018

Member

I think this should either be static, or (preferred) no parameter and work on mAttributeFormMode instead.

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 11, 2018

Member

Oh, and const

@@ -46,18 +46,6 @@ class GUI_EXPORT QgsAttributeForm : public QWidget

public:

//! Form modes

This comment has been minimized.

Copy link
@m-kuhn

m-kuhn Sep 11, 2018

Member

To avoid an API break, this needs to stay around until QGIS 4 (but add \deprecated use ``QgsAttributeEditorContext::Mode`` instead to the docs)

@m-kuhn
m-kuhn approved these changes Sep 12, 2018
Copy link
Member

m-kuhn left a comment

Any objections to hitting the merge button?

@wonder-sk

This comment has been minimized.

Copy link
Member

wonder-sk commented Sep 12, 2018

No objection, just a pending question regarding QtCharts library - is it now a required dependency? If it is, we should declare it in CMakeLists too, right?

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Sep 12, 2018

IMO it should, I think QtCharts got only added in 5.7, so do we need a WITH_QTCHARTS directive?

@wonder-sk

This comment has been minimized.

Copy link
Member

wonder-sk commented Sep 12, 2018

I think you are right - before Qt 5.7 the Qt Charts library was versioned and delivered separately:
https://blog.qt.io/blog/2016/01/18/qt-charts-2-1-0-release/

Looks like we still have Qt 5.4 as the minimum version, so if we stick to it, QtCharts should be optional for < 5.7. But given that 5.7 is already more than two years old and even Debian stable ships it, maybe it would be better to raise minimal Qt version to 5.7 for QGIS 3.4?

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Sep 12, 2018

I think there were some discussions about the minimum version recently, but I cannot recall the details right now. Let's just add it with a conditional version check (if qt version >= 5.7 require qtcharts) and lead the discussion separately.

signedav added 3 commits Sep 13, 2018
@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Sep 13, 2018

Have a look here .docker/qgis3-build-deps.dockerfile

signedav added 2 commits Sep 13, 2018
signedav added 2 commits Sep 14, 2018
because it's not needed there. should be used in runtime.
so it included qt5charts in installs
@signedav

This comment has been minimized.

Copy link
Contributor Author

signedav commented Sep 14, 2018

I removed charts stuff from cmakelist because it's not used for build but during run time instead. So added to the package control files.

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Sep 14, 2018

And off we go!

@m-kuhn m-kuhn merged commit b75f9f3 into qgis:master Sep 14, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@anitagraser anitagraser mentioned this pull request Jan 4, 2020
6 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.