Snapping enabled on a configurable scale range #35110

obrix · 2020-03-16T14:06:36Z

Description

This pull request follows the qgis enhancement proposal 167 described in details here : Snapping enabled on a configurable scale range

The goal is to provide a way to optimizing the snapping feature by allowing the user to enable it only on specified scale range. A global setting and a per layer setting are available.

For more details please refer to the qgis enhancement proposal description and discussion.

troopa81

It's not clear to me how the scale limit apply:

If global limit to scale is disabled, no scale range (global nor per layer) are applied
Else we use global range only if per layer one hasn't been defined

As a user, I would have expected

If global limit to scale is disable, per layer scale range are applied
Else we check global scale range AND per layer scale range before snapping on this layer

But maybe, I'm a weird user :)

troopa81 · 2020-03-24T15:39:15Z

src/app/qgssnappinglayertreemodel.cpp

+      {
+        if ( ls.minScale() <= 0.0 )
+        {
+          return QString( "not set" );


I think it should be translated

troopa81 · 2020-03-24T15:39:31Z

src/app/qgssnappinglayertreemodel.cpp

+      {
+        if ( ls.maxScale() <= 0.0 )
+        {
+          return QString( "not set" );


translation

troopa81 · 2020-03-24T15:47:14Z

src/core/qgssnappingconfig.h

         * \deprecated since QGIS 3.12 use the method with SnappingTypeFlag instead.
         */
-        Q_DECL_DEPRECATED IndividualLayerSettings( bool enabled, SnappingType type, double tolerance, QgsTolerance::UnitType units ) SIP_DEPRECATED;
+        Q_DECL_DEPRECATED IndividualLayerSettings( bool enabled, SnappingType type, double tolerance, QgsTolerance::UnitType units, double minScale, double maxScale ) SIP_DEPRECATED;


I'm wondering if it's usefull/needed to update this method signature while it has been deprecated.

Probably not.

troopa81 · 2020-03-24T15:57:04Z

src/core/qgssnappingutils.cpp

    QList<LayerAndAreaOfInterest> layers;
-    for ( const LayerConfig &layerConfig : qgis::as_const( mLayers ) )
+    QList<LayerConfigIterator> filteredConfigs;


Why do you store a list of iterator instead of LayerConfig? to avoid LayerConfig copy?

obrix · 2020-03-24T16:34:07Z

@troopa81 Yes you got it the global boolean disable the feature globally (including the per layer configuration). The layer specific setting applies if specified, otherwise the global setting applies.

I think it is useful to have a simple way to completely disable the feature. Your expectation give no way to disable the optimization except resetting manually all settings which might be a pain I think.

troopa81 · 2020-03-25T07:49:14Z

I think it is useful to have a simple way to completely disable the feature.

I understand your point, so OK for a button which disable all limit scale range. Maybe it would be good to disable (grayed) per layer min/max configuration when the button is disable, as it is done for the global min/max. This way, the user understand it applies to both global and per layer configuration.

3nids

could you provide a small screencast of the config widget + toolbar
would it make sense to add some tests to this?
small thing but rather than talking about "Limit on scale snapping" I would rather use "Scale dependent snapping". (to be approved by better english speakers)

3nids · 2020-03-25T07:33:09Z

src/app/qgssnappingwidget.cpp

@@ -173,6 +175,23 @@ QgsSnappingWidget::QgsSnappingWidget( QgsProject *project, QgsMapCanvas *canvas,
  mToleranceSpinBox->setObjectName( QStringLiteral( "SnappingToleranceSpinBox" ) );
  connect( mToleranceSpinBox, static_cast < void ( QDoubleSpinBox::* )( double ) > ( &QDoubleSpinBox::valueChanged ), this, &QgsSnappingWidget::changeTolerance );

+  mMinScaleWidget = new QgsScaleWidget();
+  mMinScaleWidget->setToolTip( tr( "Min scale on which snapping is enabled" ) );


Suggested change

mMinScaleWidget->setToolTip( tr( "Min scale on which snapping is enabled" ) );

mMinScaleWidget->setToolTip( tr( "Minimum scale from which snapping is enabled" ) );

maybe some native speaker can come up with something better

if no ones comes up with something better, I would recommend to accept this one

Yes, I went with it.

3nids · 2020-03-25T07:34:16Z

src/app/qgssnappingwidget.cpp

+  connect( mMinScaleWidget, &QgsScaleWidget::scaleChanged, this, &QgsSnappingWidget::changeMinScale );
+
+  mMaxScaleWidget = new QgsScaleWidget();
+  mMaxScaleWidget->setToolTip( tr( "Max scale on which snapping is enabled" ) );


Suggested change

mMaxScaleWidget->setToolTip( tr( "Max scale on which snapping is enabled" ) );

mMaxScaleWidget->setToolTip( tr( "Maximum scale up to which snapping is enabled" ) );

...not so sure

src/app/qgssnappingwidget.cpp

3nids · 2020-03-25T07:41:12Z

src/core/qgssnappingconfig.h

         * \deprecated since QGIS 3.12 use the method with SnappingTypeFlag instead.
         */
-        Q_DECL_DEPRECATED IndividualLayerSettings( bool enabled, SnappingType type, double tolerance, QgsTolerance::UnitType units ) SIP_DEPRECATED;
+        Q_DECL_DEPRECATED IndividualLayerSettings( bool enabled, SnappingType type, double tolerance, QgsTolerance::UnitType units, double minScale, double maxScale ) SIP_DEPRECATED;


src/core/qgssnappingconfig.h

src/core/qgssnappingutils.cpp

tests/src/core/testqgssnappingutils.cpp

3nids · 2020-03-25T07:52:10Z

Yes you got it the global boolean disable the feature globally (including the per layer configuration). The layer specific setting applies if specified, otherwise the global setting applies.

good idea!

troopa81 · 2020-03-25T08:01:24Z

@obrix As soon as you define a per layer min, both per layer min/max applies even if you define a global max and your per layer max is not set. I find it a little bit confusing. Wouldn't be better to separate min et max ? Or at least, make the current behavior more explicit (tooltip?)

obrix · 2020-03-25T08:16:44Z

@troopa81

I understand your point, so OK for a button which disable all limit scale range. Maybe it would be good to disable (grayed) per layer min/max configuration when the button is disable, as it is done for the global min/max. This way, the user understand it applies to both global and per layer configuration.

Yes good idea to also explicitly grey out the per layer configuration when the feature is disabled.

As soon as you define a per layer min, both per layer min/max applies even if you define a global max and your per layer max is not set. I find it a little bit confusing. Wouldn't be better to separate min et max ? Or at least, make the current behavior more explicit (tooltip?)

Yes this is a deliberate choice, from my point of view mixing some configuration from the global settings and some configuration from the per layer settings was confusing. I wanted to avoid that but I am not really satisfied by the way it is presented to the user though. If this is confusing for you I should probably separate min/max and use global setting when one is not set.

@3nids

would it make sense to add some tests to this?
small thing but rather than talking about "Limit on scale snapping" I would rather use "Scale dependent snapping". (to be approved by better english speakers)

Not sure about a test. I can take a look. I Agree with "Scale dependent snapping".

Coming back to you with some new commits soon.

troopa81 · 2020-03-25T09:11:33Z

Not sure about a test. I can take a look.

+1 for a test. at least in testqgssnappingutils.cpp to check the different snapping behavior (with or without global min/max, with or without per layer configuration...)

obrix · 2020-03-26T09:37:33Z

@troopa81 If I handle separately min and max range for layer specific settings (ie if not set -> use global configuration), it makes it impossible for the user to specify a global setting and disable completely snapping optimization on a specific layer. This is also the case for the current implementation (ie if I unset both min and max, it will be replaced by global settings).

A solution I can think of is to re add a check box to specifically enable/disable the custom setting on a specific layer. I think it makes more sense and is more explicit for the user.

So we would have

Global checkbox button to enable/disable the feature globally, with global min max range applied to all layer
A checkbox per layer to enable/disable a specific layer setting with min and max (with tooltip "Use layer specific snapping range")

I think this way the user will not have to guess which min max is used, it is explicit.

troopa81 · 2020-03-26T10:04:52Z

@obrix I'm concerned that it will make it even more complicated. I'd prefer to keep it simple

a default (global) config min/max
a per layer config min/max
a button to disable scale dependent snapping

if both global and per layer min (resp. max) are defined (different from "not set"), we check only the per layer one.

troopa81 · 2020-03-26T12:49:05Z

So we would have

Global checkbox button to enable/disable the feature globally, with global min max range applied to all layer
A checkbox per layer to enable/disable a specific layer setting with min and max (with tooltip "Use layer specific snapping range")

I think this way the user will not have to guess which min max is used, it is explicit.

After a second thoughts, it may be in fact better to do it this way so it removes any ambiguity. @3nids do you agree?

3nids · 2020-03-26T13:51:30Z

A maybe simpler alternative would be a tri-state for the main one:

inactive
active - global
active - advanced / per layer

So, if we are in active global we can hide the columns in the table.

I think this feature is very very specific and even more when going per layer. So we should try not to complexify too much the UI.
I think the approach proposed above has both the advantage of the lightest possible UX and the clarity (at a small cost of not being able to fine tune the global setting per layer, but it sounds acceptable to me)

obrix · 2020-03-26T14:11:54Z

The tri-state approach is okay for me as it also remove any ambiguity.

3nids · 2020-04-06T07:10:26Z

ready to re-review / merge?

3nids · 2020-04-06T07:13:01Z

src/core/qgssnappingconfig.h

         * \since QGIS 3.12
         */
-        IndividualLayerSettings( bool enabled, SnappingTypeFlag type, double tolerance, QgsTolerance::UnitType units );
+        IndividualLayerSettings( bool enabled, SnappingTypeFlag type, double tolerance, QgsTolerance::UnitType units, double minScale, double maxScale );


this is an API break, can you use default values?

3nids · 2020-04-06T07:15:09Z

src/core/qgssnappingconfig.h

+        /**
+         * !
+         * Returns the tolerance
+         * \since QGIS 3.12


I don't think this method and the following were added in 3.12
https://github.com/qgis/QGIS/blame/master/src/core/qgssnappingconfig.h#L172

Yes I think I went too fast with some doc modification.

3nids · 2020-04-06T07:16:20Z

src/core/qgssnappingconfig.h

+         * Returns min scale on which snapping is limited
+         * \since QGIS 3.14
+         */
+        double minScale() const;


both in the docstring and the method names can you use minimum and maximum in whole words rather than min/max?

src/app/qgssnappingwidget.h

- LimitToScaleRange : Allows to enable snapping only when current scale on the canvas is in a specified range. This range is controled by the two parameters scale min and scale max. - MinScale : Minimum scale in which the snapping is enabled - MaxScale : Maximum scale in which the snapping is enabled When LimitToScaleRange is set to true, snapping is disabled if the current scale out of [MinScale, MaxScale]. When LimitToScaleRange is set to false, the behavior remain unchanged and snapping is enabled whatever the scale. This allows to limit the costly cache refresh for some heavy layers when panning said layer at a level where the snapping is not useful.

…after having rebuilt the index.

…ues disable the limits.

…o enable). Also use 0.0 for disabling value and display NULL to be more explicit.

…ix style.

…tion widget from the toolbar.

* better documentation * translation on non translated string * Grey out column min and max of individual layer snapping settings when snapping limit on scale is disabled. Force the refresh when rowChanged is called even if the individual settings are the same to enable/disable immediately the columns when the snapping limit on scale button is pushed.

…rceRefresh.

…and per layer.

…per layer modes.

…ated API.

… and max. Add default value for min and max in IndividualLayerSettings as the two new parameters break compatibility.

github-actions bot added this to the 3.14.0 milestone Mar 16, 2020

obrix force-pushed the snapping_zoomlvl_optim branch from 5571fbe to 3580df2 Compare March 19, 2020 15:43

obrix requested review from troopa81 and 3nids March 24, 2020 14:37

troopa81 requested changes Mar 24, 2020

View reviewed changes

3nids reviewed Mar 25, 2020

View reviewed changes

3nids reviewed Apr 6, 2020

View reviewed changes

obrix force-pushed the snapping_zoomlvl_optim branch from 7825f8e to b21c5e3 Compare April 6, 2020 10:10

3nids reviewed Apr 6, 2020

View reviewed changes

src/app/qgssnappingwidget.h Outdated Show resolved Hide resolved

3nids reviewed Apr 6, 2020

View reviewed changes

src/app/qgssnappingwidget.h Outdated Show resolved Hide resolved

3nids approved these changes Apr 6, 2020

View reviewed changes

obrix added 5 commits April 7, 2020 09:27

Some labels adjustments.

b5bbc63

Remove useless log.

5c6fa57

Add serialization of new snapping parameters.

3d5b924

Keep iterator on filtered layers and iterate only on filtered layers …

5eedfe1

…after having rebuilt the index.

obrix added 20 commits April 7, 2020 09:27

Remove boolean limitToScaleRange, default value is -1.0. Negative val…

7b4be2b

…ues disable the limits.

Add global config for snapping limit on scale (with action and icon t…

b484cae

…o enable). Also use 0.0 for disabling value and display NULL to be more explicit.

Enable snapping whatever the scale when limit to scale is off. Also f…

2346718

…ix style.

Update with master.

c2b8106

Use QgsScaleWidget instead of spinboxes. Remove the snapping optimiza…

c291a17

…tion widget from the toolbar.

Fix style.

6b9fb08

Fix PyQgsLayerDependencies test and add some doc.

30a52ac

Keep the check on oldsettings to refresh rows but add an option to fo…

b3e96ff

…rceRefresh.

Use a tri state button to configure snapping limit : disable, global …

03c522a

…and per layer.

Add a test for snapping enabled/disabled using disabled, global, and …

cfc1989

…per layer modes.

Some style adjustments

563e396

Add missing doc and update test_layer_dependencies.py with non deprec…

99780ee

…ated API.

Regenerate qgssnappingconfig.py

c7f7c98

Remove uneeded doc.

7a599bb

Remove useless comments in sip.

080d0fa

Fix some documentation issues. Use minimum and maximum instead of min…

8b88115

… and max. Add default value for min and max in IndividualLayerSettings as the two new parameters break compatibility.

Fix spelling.

f2a1712

Remove prefix on parameters name. Remove whitespace.

b5fa0fa

Update tooltips.

a7c4bbb

obrix force-pushed the snapping_zoomlvl_optim branch from 82f48b7 to a7c4bbb Compare April 7, 2020 07:27

troopa81 approved these changes Apr 7, 2020

View reviewed changes

3nids merged commit 6ba8ccc into qgis:master Apr 7, 2020

obrix mentioned this pull request Apr 28, 2020

Snapping enabled on a configurable scale range qgis/QGIS-Enhancement-Proposals#167

Closed

	mMinScaleWidget->setToolTip( tr( "Min scale on which snapping is enabled" ) );
	mMinScaleWidget->setToolTip( tr( "Minimum scale from which snapping is enabled" ) );

	mMaxScaleWidget->setToolTip( tr( "Max scale on which snapping is enabled" ) );
	mMaxScaleWidget->setToolTip( tr( "Maximum scale up to which snapping is enabled" ) );

Snapping enabled on a configurable scale range #35110

Snapping enabled on a configurable scale range #35110

Conversation

obrix commented Mar 16, 2020

Description

troopa81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obrix commented Mar 24, 2020

troopa81 commented Mar 25, 2020

3nids left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3nids commented Mar 25, 2020

troopa81 commented Mar 25, 2020

obrix commented Mar 25, 2020 • edited

troopa81 commented Mar 25, 2020 via email

obrix commented Mar 26, 2020

troopa81 commented Mar 26, 2020

troopa81 commented Mar 26, 2020

3nids commented Mar 26, 2020 • edited

obrix commented Mar 26, 2020

3nids commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obrix commented Mar 25, 2020 •

edited

3nids commented Mar 26, 2020 •

edited