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] [feature] Feature attribute panel #7350

Merged
merged 5 commits into from
Jul 13, 2018

Conversation

PeterPetrik
Copy link
Contributor

This pull request is last missing bit from (#6490 )

Allow to show feature attribute form and edit/save the modification
of the attributes. Support most frequent QGIS edit widgets such
as text edit, value map or external resource (photo capture)

featurepanel

For background information see the associated QEP.

Allow to show feature attribute form and edit/save the modification
of the attributes. Support most frequent QGIS edit widgets such
as text edit, value map or external resource (photo capture)

void QgsQuickSubModel::onRowsAboutToBeInserted( const QModelIndex &parent, int first, int last )
{
emit beginInsertRows( mapFromSource( parent ), first, last );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a signal - do not use "emit"

Same below for onRowsInserted, beginRemoveRows, endRemoveRows

@nyalldawson nyalldawson added this to the 3.4 milestone Jul 2, 2018
virtual bool filterAcceptsRow( int source_row, const QModelIndex &source_parent ) const override;

private:
QgsQuickAttributeFormModelBase *mSourceModel; //not owned
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize to nullptr


if ( mLayer )
{
QgsAttributeEditorContainer *root;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initialize to nullptr

@wonder-sk
Copy link
Member

@m-kuhn is it okay if I merge it or would you like to review that as well?

@m-kuhn
Copy link
Member

m-kuhn commented Jul 9, 2018

Thanks for pinging. I'd like to give some feedback, I remember I've had some questions when scrolling through this quickly.

public:

//! Feature fields's roles
enum FeatureRoles
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be a good idea to rename this enum to something with Field, what do you think?

QgsMessageLog::logMessage( tr( "Cannot update feature" ),
QStringLiteral( "QgsQuick" ),
Qgis::Warning );
rv = commit();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be good to find a better approach to dealing with this in the long run.

I assume, this calls lower-level I/O functions which shouldn't be used in a Q_INVOKABLE because they can make the UI unresponsive.

* Returns whether file on path exists
* \since QGIS 3.4
*/
Q_INVOKABLE bool fileExists( QString path );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const?

* Extracts filename from path
* \since QGIS 3.4
*/
Q_INVOKABLE QString getFileName( QString path );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const QString &path?
const?

}
else
{
return QUrl( path.arg( "textedit" ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QStringLiteral

const QUrl QgsQuickUtils::getEditorComponentSource( const QString &widgetName )
{
QString path( "qgsquick%1.qml" );
QStringList supportedWidgets = { "textedit",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QStringLiteral

@m-kuhn
Copy link
Member

m-kuhn commented Jul 13, 2018

Good job.
Not much left to say. Have a look at where to use QStringLiteral and const and the naming of the enum if you like.
I think we'll in the future need to revisit some of the lower-level I/O calls and make sure they are executed async, but I wouldn't take that into the scope of this PR unless you are eager for the challenge.

I haven't looked at the QML code in detail, trusting you there.

@PeterPetrik
Copy link
Contributor Author

@m-kuhn thanks for review and time and I hope we can now all benefit from the common qgis mobile codebase!

@m-kuhn m-kuhn merged commit 12c8f39 into qgis:master Jul 13, 2018
@m-kuhn
Copy link
Member

m-kuhn commented Jul 13, 2018

Thank you @PeterPetrik , I'll create an updated docker image soon as a new base image for the QField builds, then pieces can be incrementally replaced with core changes.

What I am not completely sure yet is how to handle the development process there most efficiently. In explicit, when a file from qgis core needs to be changed, how to bring that in a timely manner to a fully packaged QField apk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants