Fix new profile map scales

[YoannQDQ]

• Fixes Scale listbox do not working in QGIS version 3.30.0-'s-Hertogenbosch #52383

Description

This completes #52511 to fix issues regarding the map scale in QGIS 3.30.1

The map scale setting format has changed (QString -> QStringList). When QGIS launches, if the new setting do not exist, it is initailized from the old key. For new profiles however, the old key does not exist either, causing the default scales to be overwritten with an empty string (which also caused crashes, solved by #52511).

This PR checks if the old key actually exists before attempting to migrate it.

[3nids]

well caught, sorry for this!

[agiudiceandrea]

@YoannQDQ, I've tested the MingW64 Windows 64bit Build of this PR.
It fixes the issue using a new QGIS user profile with QGIS 3.31, while it seems to me it doesn't fix the issue using a QGIS 3.28 user profile with QGIS 3.31 as described e.g. in #52383 (comment) and #52383 (comment).

[3nids]

// handle bad migration (TODO remove in 3.32 or later)
if ( settingsMapScales->value() == QString( "" ) )
settingsMapScales->remove();

Adding this should fix former issues

[agiudiceandrea]

It seems to me the behaviour with the new commit is the same as with the previous one: it fixes the issue using a new QGIS user profile with QGIS 3.31, while it doesn't fix the issue using a QGIS 3.28 user profile with QGIS 3.31 as described e.g. in #52383 (comment) and #52383 (comment).

[YoannQDQ]

Hopefuly fixed now.

[agiudiceandrea]

@YoannQDQ, now the list of scales that is set by an user in Settings->Options->Map Tools in QGIS 3.28 and previous versions, is completely removed by new versions of QGIS and substituted with the default predefined list of scales.
Why not actually use the list of scales already present in the settings, instead of removing it? I think it would be better.

[agiudiceandrea]

It seems to me now the reported issues (with a new user profile and with an old user profile) are completely fixed in the MingW64 Windows 64bit build of this PR. Thanks @YoannQDQ!

@3nids do you know if there are other settings changed from QString to QStringList that needs a better handling like it was needed for the scales setting?

It remains open the question about the backward incompatibility: using a QGIS <= 3.28 user profile with QGIS >= 3.30.2 will permanently change the scales settings in a way that it will not be properly read again by QGIS <= 3.28.

@3nids do you think the users should be made aware of such backward incompatibility of the scales settings and any other settings changed from QString to QStringList?

[3nids]

Thanks a lot for this @YoannQDQ and @agiudiceandrea for the detailed report.

There is no backward incompatibility since the settings keys are not the same (it's a migration).

There are other QStringList based settings but I think they are not affected by this issue (I'll check this when I'll be back with a computer).

Thanks again

[agiudiceandrea]

There is no backward incompatibility since the settings keys are not the same (it's a migration).

@3nids, it seems to me it is the same setting and I've checked that the very scales setting is permanently changed and it is no longer properly read again by QGIS <= 3.28 after the QGIS user profile is used by QGIS from the MingW64 Windows 64bit build of this PR: this means that in QGIS <= 3.28 the map scales listbox and the Predefined Scales (Setting->Option->Map Tools) are empty after the QGIS user profile is used by QGIS form the MingW64 Windows 64bit build of this PR.

[3nids]

Strange, the section was changed from Map to map. I believe the case matters. And there is no deletion of the old setting in the migration. Can you check if both exist (upper and lower case section)?

[agiudiceandrea]

Can you check if both exist (upper and lower case section)?

After opening a QGIS <= 3.28 user profile with QGIS from the MingW64 Windows 64bit build of this PR, there is only 1 scales setting in the [Map] (uppercase) section in QGIS3.ini. There is no [map] (lower case) section.

I believe the case matters.

QgsSettings().contains("qgis/style"), QgsSettings().contains("Qgis/Style"), QgsSettings().contains("QGIS/STYLE"), all return True and QgsSettings().value("qgis/style"), QgsSettings().value("Qgis/Style"), QgsSettings().value("QGIS/STYLE"), all return the same value, although there is only 1 style setting in the only 1 qgis section in QGIS3.ini.

[YoannQDQ]

I believe the case matters

Actually, it does on Ubuntu for instance, but not on Windows. So Windows users will encounter the problem @agiudiceandrea describes. We can either rename the sTreeMap key to something meaningful other than "map" (not too difficult since it isn't wide-spread yet) or rename the new map/scales property to default_scales for instance. Otherwise, profiles will effectively be backward incompatible.Thoughts @3nids ?

[3nids]

I was thinking about renaming the node but we would need to check if other keys are using it which don't require migration.
Renaming the setting sounds like the way to go. We can drop the bad migration handling code at the same time.

[YoannQDQ]

IMO We still need some kind of migration, from the map/scales key to the new map/default_scales key, as well as a "backward migration" to fix the Map/scales key on Windows. The last commit should address all the issues.

[YoannQDQ]

@3nids Do you feel like this can now be merged?

[agiudiceandrea]

@YoannQDQ, it seems to me the issue is not yet fixed.

In a QGIS <= 3.28 user profile, QGIS3.ini contains e.g.:

[Map]

scales="1:5000,1:4000,1:3000,1:2000,1:1000"

Using such user profile with QGIS from this PR (MingW64 Windows 64bit Build, actually from agiudiceandrea#104, because the MingW64 build here failed), QGIS3.ini contains:

[Map]

scales="1:5000,1:4000,1:3000,1:2000,1:1000"
default_scales="1:5000,1:4000,1:3000,1:2000,1:1000"

and again...

[YoannQDQ]

... You're right. Let's try again!

[3nids]

This looks good to me.
Sorry for this and thanks a lot for fixing it!

[agiudiceandrea]

@YoannQDQ, only now have I had the opportunity to test this PR.

1. With a new user profile: QGIS from this PR now only stores the scales list in default_scales setting as QStringList.
   This setting is not used by QGIS 3.28, so the custom scales set in QGIS from this PR will not be read by QGIS 3.28 using the same user profile.
2. With a profile created by QGIS 3.30.1 which contains e.g. a QStringList scales=1:1000, 1:2000, 1:3000 setting: QGIS from this PR creates two settings: a QString scales="1:1000,1:2000,1:3000" setting and a QStringList default_scales=1:1000, 1:2000, 1:3000 setting.
3. With a profile created by QGIS <= 3.28 which contains e.g. a QString scales=1:1000,1:2000,1:3000 setting: QGIS from this PR adds the QStringList default_scales=1:1000, 1:2000, 1:3000 setting.

For all the cases, the scales list modified by QGIS 3.28 and QGIS >= 3.30.2 will be stored in separate settings in the same user profile so the list read by QGIS 3.28 may be different from the list read by QGIS >= 3.30.2 for the same user profile.

Anyway, now QGIS no longer crashes and the scales list in the old settings is preserved and it is correctly migrated from the old setting to the new setting, but only one-way (3.28 -> 3.30.2) while it seems to me it is not for the other way round (3.30.2 -> 3.28).

@3nids, I'm concerned if there are other settings changed from QString to QStringList (or other type changing) that needs a better handling like it was needed for the scales setting.

[3nids]

while it seems to me it is not for the other way round (3.30.2 -> 3.28).

Yes, migration is only forward and not backward for this one. It could be added now that we have changed the name of the keys. But is it really worth it?

I'm concerned if there are other settings changed from QString to QStringList

as far as I remember and from a quick check now, no it's the only migration. There are other usages but they were already a list of strings.

[agiudiceandrea]

rom a quick check now, no it's the only migration.

Thanks for checking. Since it is the only one, I think it would have been preferable to leave also such setting as it was, in order to avoid these issues.