`ShowOnlyNames` value not written to the DB – Regression in the recent releases?

[tflo]

I have downloaded TPTP from CF (11.2.6) and noticed that the “Only Names” setting (in General > Blizzard Settings) is resetting at reload.

I checked the code and found that the corresponding CVar value is nowhere saved to the addon’s database (which is necessary, because it’s one of those CVars that get reset on reload).

After I forked from your master branch to make a PR, I noticed that the master is an old branch, but there the code is correct (i.e., the CVar value is saved to the DB via Addon.db.profile.BlizzardSettings.Names.ShowOnlyNames = val in Options.lua, line 5449).

This needed line is also present in your devel/113/main branch, also in the devel/111/main, but not in the devel/112/main, which is the source of your recent releases.

This is the commit that is missing in devel/112/main.

I'm a little confused with your many branches, so I'm probably missing something(?). But in any case, the CurseForge builds starting with 11.2.0-beta4 up to the current one (11.2.6) are missing the needed line. (The ones before seem to be based on devel/111/main and are OK).

[Backupiseasy]

First, I wanted to answer that TP in general does not store CVars in its profile (as this could result in side-effets with other addons that I want to avoid), but ... nameplateShowOnlyNames is an exception. I even added a comment for that - which is good, otherwise I would have forgotten.

So, in short, you are right. Not sure, why it got removed, but this is a bug.

The branches, well, devel/113/main is 11.3.x (not releases), devel/112/main is 11.2.x and so on. It does not make sense anymore as there seldom are sub-branches besides /main, but I used that at the beginning. Maybe it's time to clean up a bit ...

[tflo]

Maybe it's time to clean up a bit ...

Yes, I thought you might be interested in this "lost" commit also because there may be other commits that have been accidentally discarded when merging branches in the past. (Not that I've noticed any other bugs, but of course I haven't tried all the settings...)

[tflo]

BTW, the fix I've made in my TP copy, before I noticed that there already was a solution in the old branches, was a bit different:

diff --git a/TidyPlates_ThreatPlates/Options.lua b/TidyPlates_ThreatPlates/Options.lua
--- a/TidyPlates_ThreatPlates/Options.lua
+++ b/TidyPlates_ThreatPlates/Options.lua
@@ -5766,12 +5766,7 @@
               ShowOnlyNames = {
                 name = L["Only Names"],
                 order = 30,
                 type = "toggle",
-                set = function(info, val)
-                  SetCVarBoolTPTP(info, val)
-                  ReloadUI()
-                end,
-                get = GetCVarBoolTPTP,
                 desc = L["Show only unit names and hide healthbars (requires /reload). Note that the clickable area of friendly nameplates will also be set to zero so that they don't interfere with enemy nameplates stacking (not in Classic or TBC Classic)."],
-                arg = "nameplateShowOnlyNames",            
+                arg = { "BlizzardSettings", "Names", "ShowOnlyNames" },            
               },

  I.e., just using the default setter/getter.

This seems to work fine, and is – IMHO – a bit more elegant. A very positive additional effect of this solution is that the addon no longer needs to force a reload on the user after changing the setting (I hate forced reloads! Especially when they happen 5 seconds before I log out anyway 🤯). This is also consistent with the tooltip now, which says "requires /reload" (it doesn't say "a reload will be triggered immediately when you touch this setting").

I understand that we need a reload here because of the SetNamePlateFriendlySize, but nothing bad will happen if I don't reload immediately, since with this solution the nameplateShowOnlyNames CVar is also only changed on reload, not immediately.

But I've only tested this briefly, so I might be overlooking something. (But I checked if the SetNamePlateFriendlySize(0.1, 0.1) function in Core.lua, line 99, is still called after reload, and yes, it is.)

[Backupiseasy]

I don't think I agree with that. I believe it's more confusing to enable an option and then the change is not applied immediately, but only after a releoad which you have to do manually.

I agree that reloads are not a good thing and I avoid it wherever possible, but I don't think that I should do that here.

[tflo]

more confusing

Well, you could add an asterisk to the label:

* Only names

This is often understood as "requires reload", or at least it indicates that with this setting there is something that needs attention (and the tooltip already tells the user that he has to do a /reload to make the change effective).

But that was just a suggestion; maybe my acute allergy to forced reloads is not very common 😉 (whenever I get an addon that does this, I immediately mod it out, at least if it's a setting that I change more than once (which is not necessarily the case here, I think)).