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

Add processing algorithms/toolbox functionality #5388

Merged
merged 27 commits into from
Jun 25, 2024
Merged

Add processing algorithms/toolbox functionality #5388

merged 27 commits into from
Jun 25, 2024

Conversation

nirvn
Copy link
Member

@nirvn nirvn commented Jun 23, 2024

Another great leap forward: in-place processing algorithms support straight within QField. I apologizes for the size of the PR, but I think it's justified ;-P

Here's a screenshot showing the UI/UX integration into QField:

qfield_processing.mp4

~~ATM, only two algorithms - orthogonalize and rotation - are unlocked. I wanted to focus on those two as part of the initial PR. ~~

Note: yes, we have algorithm output preview, which I just love 😉

github-actions[bot]

This comment was marked as outdated.

@qfield-fairy
Copy link
Collaborator

qfield-fairy commented Jun 23, 2024

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

src/core/CMakeLists.txt Outdated Show resolved Hide resolved
github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

Copy link
Collaborator

@mohsenD98 mohsenD98 left a comment

Choose a reason for hiding this comment

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

Fantastic functionality, you are on fire @nirvn , well done :)
All good, i just have some cleanup comments.

src/qml/NavigationBar.qml Outdated Show resolved Hide resolved
src/qml/NavigationBar.qml Outdated Show resolved Hide resolved
src/qml/ProcessingAlgorithmForm.qml Show resolved Hide resolved
src/qml/ProcessingAlgorithmPreview.qml Outdated Show resolved Hide resolved
src/qml/ProcessingAlgorithmsList.qml Show resolved Hide resolved
src/qml/processingparameterwidgets/number.qml Show resolved Hide resolved
@nirvn nirvn merged commit 20ee497 into master Jun 25, 2024
19 checks passed
@nirvn nirvn deleted the processing branch June 25, 2024 08:18
@nirvn
Copy link
Member Author

nirvn commented Jun 25, 2024

@m-kuhn , @mohsenD98 , thanks for the pair of reviews guys.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

int ProcessingAlgorithmParametersModelBase::rowCount( const QModelIndex &parent ) const
{
if ( !parent.isValid() )
return mParameters.size();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'qsizetype' (aka 'long long') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]

    return mParameters.size();
           ^

if ( index.row() >= mParameters.size() || index.row() < 0 || !mParameters.at( index.row() ) )
return QVariant();

switch ( role )
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]

  switch ( role )
  ^

if ( index.row() >= mParameters.size() || index.row() < 0 || !mParameters.at( index.row() ) )
return false;

switch ( role )
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]

  switch ( role )
  ^


public:
//! Available filter flags for filtering the model
enum Filter
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: enum 'Filter' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

    enum Filter
         ^


public:
//!Roles of the model.
enum Role
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: enum 'Role' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]

    enum Role
         ^

int ProcessingAlgorithmsModelBase::rowCount( const QModelIndex &parent ) const
{
if ( !parent.isValid() )
return mAlgorithms.size();
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'qsizetype' (aka 'long long') to signed type 'int' is implementation-defined [bugprone-narrowing-conversions]

    return mAlgorithms.size();
           ^

if ( index.row() >= mAlgorithms.size() || index.row() < 0 || !mAlgorithms.at( index.row() ).algorithm() )
return QVariant();

switch ( role )
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]

  switch ( role )
  ^

if ( index.row() >= mAlgorithms.size() || index.row() < 0 || !mAlgorithms.at( index.row() ).algorithm() )
return false;

switch ( role )
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: switching on non-enum value without default case may not cover all cases [bugprone-switch-missing-default-case]

  switch ( role )
  ^


public:
//! Available filter flags for filtering the model
enum Filter
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: enum 'Filter' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint8_t' (1 byte) as the base type to reduce its size [performance-enum-size]

    enum Filter
         ^


public:
//!Roles to get the data of the model.
enum Role
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: enum 'Role' uses a larger base type ('unsigned int', size: 4 bytes) than necessary for its value set, consider using 'std::uint16_t' (2 bytes) as the base type to reduce its size [performance-enum-size]

    enum Role
         ^

}

QgsProcessingContext context;
context.setFlags( QgsProcessingContext::Flags() );
Copy link
Contributor

Choose a reason for hiding this comment

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

There's actually NO flags right now, you could drop this line

Copy link
Member Author

Choose a reason for hiding this comment

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

@nyalldawson , thanks, leftover.

src/core/processing/processingalgorithm.cpp Show resolved Hide resolved
-1,
Qgis::ProcessingFeatureSourceDefinitionFlags(),
Qgis::InvalidGeometryCheck(),
QStringLiteral( "$id IN (%1)" ).arg( featureIds.join( ',' ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these features already selected? If so, you could just set the "selectedFeaturesOnly" argument to true and avoid the IN expression

Copy link
Member Author

Choose a reason for hiding this comment

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

@nyalldawson , na, the features are actually not selected on the layer itself, it's a different handling (a selected features model), hence why I need to filter by IDs here.


if ( !previewMode )
{
mInPlaceLayer->startEditing();
Copy link
Contributor

Choose a reason for hiding this comment

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

You might consider mInPlaceLayer->beginEditCommand() here, if you want all the operations to undo-able in a single step

Copy link
Member Author

Choose a reason for hiding this comment

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

@nyalldawson , we don't use QGIS' internal buffer, so that's not needed ATM. But, that makes me think our own history handling should have a command-like concept to store multiple changes.

QgsProcessingFeatureBasedAlgorithm *alg = static_cast<QgsProcessingFeatureBasedAlgorithm *>( featureBasedAlgorithm->create( { { QStringLiteral( "IN_PLACE" ), true } } ) );
if ( !alg->prepare( parameters, context, &feedback ) )
{
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a leak here -- I'd use unique_ptr for alg

Copy link
Member Author

Choose a reason for hiding this comment

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

@nyalldawson , true true, thanks.

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

Successfully merging this pull request may close these issues.

None yet

5 participants