Skip to content

Rework cancellation logic for LayerFeaturesModel searches#4526

Open
uclaros wants to merge 5 commits into
masterfrom
bugfix/rework-cancel-requests
Open

Rework cancellation logic for LayerFeaturesModel searches#4526
uclaros wants to merge 5 commits into
masterfrom
bugfix/rework-cancel-requests

Conversation

@uclaros

@uclaros uclaros commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Rework of the cancellation logic in LayerFeaturesModel feature requests.

Main changes:

  • Each search gets its own QgsFeedback and QFutureWatcher instances.
  • To cancel a request we use QgsFeedback::cancel() and let the provider stop the iteration.
  • Pending request is cancelled when model or layer is destroyed.

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 27276990546

Warning

No base build found for commit 1905517 on master.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 57.877%

Details

  • Patch coverage: No coverable lines changed in this PR.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →


Coverage Stats

Coverage Status
Relevant Lines: 15253
Covered Lines: 8828
Line Coverage: 57.88%
Coverage Strength: 98.88 hits per line

💛 - Coveralls

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown

📦 Build Artifacts Ready

OS Status Build Info Workflow run
macOS Build 📬 Mergin Maps 68871 dmg Expires: 02/09/2026 #6887
linux Build 📬 Mergin Maps 69131 x86_64 Expires: 02/09/2026 #6913
win64 Build 📬 Mergin Maps 60811 win64 Expires: 02/09/2026 #6081
Android Build 📬 Mergin Maps 819711 APK [armeabi-v7a] Expires: 02/09/2026 #8197
📬 Mergin Maps 819711 APK [armeabi-v7a] Google Play Store #8197
Android Build 📬 Mergin Maps 819751 APK [arm64-v8a] Expires: 02/09/2026 #8197
📬 Mergin Maps 819751 APK [arm64-v8a] Google Play Store #8197
iOS Build Build failed or not found. #9139

@tomasMizera tomasMizera added this to the 2026.4.0 milestone Jun 8, 2026
@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

📦 Build Artifacts Ready

OS Status Build Info Workflow run
macOS Build 📬 Mergin Maps 68961 dmg Expires: 06/09/2026 #6896
linux Build 📬 Mergin Maps 69221 x86_64 Expires: 06/09/2026 #6922
win64 Build 📬 Mergin Maps 60901 win64 Expires: 06/09/2026 #6090
Android Build 📬 Mergin Maps 820651 APK [arm64-v8a] Expires: 06/09/2026 #8206
📬 Mergin Maps 820651 APK [arm64-v8a] Google Play Store #8206
Android Build 📬 Mergin Maps 820611 APK [armeabi-v7a] Expires: 06/09/2026 #8206
📬 Mergin Maps 820611 APK [armeabi-v7a] Google Play Store #8206
iOS Build Build failed or not found. #9148

@Withalion Withalion modified the milestones: 2026.4.0, 2026.3.0 Jun 8, 2026

@Withalion Withalion left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's rebase before merging

Comment thread app/layerfeaturesmodel.cpp Outdated
beginResetModel();
mFeatures.clear();
for ( const auto &f : features )
QFutureWatcher<SearchResultData> *watcher = static_cast< QFutureWatcher<SearchResultData> *>( sender() );

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be dynamic_cast instead

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The sender here is guaranteed to be the future watcher.
I'd argue that actually crashing would be a better choice in case future me tries to call onFutureFinished() by other means!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alright, I don't really have a strong argument here. It was also pointed out by clang-tidy

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If type safety is the issue, I'd suggest a0a46c0

@uclaros uclaros force-pushed the bugfix/rework-cancel-requests branch from abda380 to a0a46c0 Compare June 10, 2026 12:42
@uclaros

uclaros commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

I switched back to using the QgsVectorLayerFeatureSource for iteration instead of the dataSource feature source in 1dbadb5.
Now searching over virtual fields will work again.

@github-actions

Copy link
Copy Markdown

📦 Build Artifacts Ready

OS Status Build Info Workflow run
macOS Build 📬 Mergin Maps 69091 dmg Expires: 08/09/2026 #6909
linux Build 📬 Mergin Maps 69351 x86_64 Expires: 08/09/2026 #6935
win64 Build Build failed or not found. #6105
Android Build 📬 Mergin Maps 821911 APK [armeabi-v7a] Expires: 08/09/2026 #8219
📬 Mergin Maps 821911 APK [armeabi-v7a] Google Play Store #8219
Android Build 📬 Mergin Maps 821951 APK [arm64-v8a] Expires: 08/09/2026 #8219
📬 Mergin Maps 821951 APK [arm64-v8a] Google Play Store #8219
iOS Build 📬 Build number: 26.06.916111 #9161

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants