I reviewed the code about settings, and found many unnecessary parts and sections that need modification. Since there is quite a bit of content, I wanted to discuss it with you here before submitting a pull request.

(1) Frame "EditModeExpandedSettingSlider", Created in line 770 and function in line 784 is not used. It should be deleted.

(2) "OnLoad" script are executed when a Frame is created. So we don't need to call it every time we update settings.

"OnLoad" in function EditModeExpandedSystemSettingsDialog:UpdateSettings, line 851
Slider are created with CreatePool in line 982 use "EditModeSettingSliderTemplate", and OnLoad script is execute.

For example:
Old:

if displayInfo.setting == Enum.EditModeUnitFrameSetting.FrameSize then
     savedValue = savedValue or 100
     settingFrame:OnLoad()
     CallbackRegistryMixin.OnLoad(settingFrame);
    
     local function OnValueChanged(self, value)
          if not self.initInProgress then
               EditModeExpandedSystemSettingsDialog:OnSettingValueChanged(self.setting, value);
          end
     end
                            
     settingFrame.cbrHandles = EventUtil.CreateCallbackHandleContainer();
     settingFrame.cbrHandles:RegisterCallback(settingFrame.Slider, MinimalSliderWithSteppersMixin.Event.OnValueChanged, OnValueChanged, settingFrame);
end

New:

if displayInfo.setting == Enum.EditModeUnitFrameSetting.FrameSize then
     savedValue = savedValue or 100
end

settingFrame.cbrHandles should be set in function EditModeExpandedSystemSettingsDialog:OnLoad in line 967, not function EditModeExpandedSystemSettingsDialog:UpdateSettings

Overall, I think the code related to Settings needs a complete refactoring. Its hard to explain all of it in an issue. I have already started this process on lib in my UI addons. And if you think it is worth to do, I will try to submit the changes to the PR once I have completed this work.
@teelolws

The EditModeExpandedSettingSlider is leftover code from when I was trying to make the Slider static. I couldn't get that to work so I let it use the framepool instead.

The cbrHandles stuff comes from function EditModeGridSpacingSliderMixin:OnLoad() in FrameXML\EditModeManager.lua

Its there to stop the original callback handlers being called instead. It needs to be changed after I :Acquire() it from the frame pool. It replaces the self.OnSliderValueChanged callback in EditModeManager with the custom one declared there.

This is the original UpdateSettings code from EditModeDialogs.lua. It turned into the... monstrosity... to make it work within my own variables. I'm open to suggestions on cleaning it up:

The cbrHandles stuff comes from function EditModeGridSpacingSliderMixin:OnLoad() in FrameXML\EditModeManager.lua

Its there to stop the original callback handlers being called instead. It needs to be changed after I :Acquire() it from the frame pool. It replaces the self.OnSliderValueChanged callback in EditModeManager with the custom one declared there.

Yeah, you are right. I find a new way. We don't have to changed anything after "Acquire" from frame pool, we can define a new template in our own XML file. For example, We can defined EditModeExpandedSettingSliderTemplate and inherits the original template EditModeSettingSliderTemplate from blizzard, and then we can use our own OnLoad script, and don't need to change it on other place.
This will make the code easier to read and run more efficiently.

<Frame name="EditModeExpandedSettingSliderTemplate" inherits="EditModeSettingSliderTemplate" hidden="true" virtual="true">
    <Scripts>
        <OnLoad>
            CallbackRegistryMixin.OnLoad(self)

            local function OnSliderValueChanged(self, value)
                if not self.initInProgress then
                    EditModeExpandedSettingsDialog:OnSettingValueChanged(self.setting, value)
                end
            end

            self.cbrHandles = EventUtil.CreateCallbackHandleContainer()
            self.cbrHandles:RegisterCallback(self.Slider, MinimalSliderWithSteppersMixin.Event.OnValueChanged, OnSliderValueChanged, self)
        </OnLoad>
    </Scripts>
</Frame>

And many other changes, I already finish cleanup work of settings part on EditModeExpanded lib in my AddOn, but I have to be careful in making PR to prevent any issues with the code.

Thats right. When I was first writing this, I tried using an xml file. It doesn't work with libraries. I had to rewrite a significant amount of code into Lua, which led to that unused EditModeExpandedSettingSlider. That also means I can't make my own custom Templates.

Its just an unfortunate side-effect of writing the addon as a portable library. I don't get to use xml.

So instead of my own SliderTemplate, I just used the existing SliderTemplate and overwrote its ValueChanged function every time its Acquired.

Thats right. When I was first writing this, I tried using an xml file. It doesn't work with libraries. I had to rewrite a significant amount of code into Lua, which led to that unused EditModeExpandedSettingSlider. That also means I can't make my own custom Templates.

Its just an unfortunate side-effect of writing the addon as a portable library. I don't get to use xml.

So instead of my own SliderTemplate, I just used the existing SliderTemplate and overwrote its ValueChanged function every time its Acquired.

Emmm... Why we can't use XML custom Templates in library?
I already test the XML code I write above, It works well. And people use library just to add EditModeExpanded-1.0.xml to their XML file or toc file.

Did I make mistake someplace?

I don't remember the issue I had. I was trying to include it in one of my addons, but it couldn't load in the xml. Maybe the addon was Load On Demand? Or maybe it couldn't find the template.

There it is: 64cdbfe

I don't remember why I did this. I would have had a reason, as its much easier to understand in xml.

This is the file path in my addon, I use toc to include Load_Libs.xml, and Load_Libs.xml include EditModeExpanded
<Include file='EditModeExpanded-Etern_UI-1.0\EditModeExpanded-Etern_UI-1.0.xml'/>, It works well.

I set addon to Load On Demand, and test it again later.

I'll roll it back to the template in XML. I will need to do this anyway eventually to make DropDown Menu support work. I can't use Blizzards DropDown Menu (taint issues) and will need to import LibUIDropDownMenu instead.

There it is: 64cdbfe

I don't remember why I did this. I would have had a reason, as its much easier to understand in xml.

A simple test, create an addon named !test with these files:
!test.toc

## Interface: 100007
## Title: test
## LoadOnDemand: 1

test.xml

test.xml

<Ui>
	<Frame name="testTemplate" inherits="EditModeSettingSliderTemplate" hidden="true" virtual="true">
		<Scripts>
			<OnLoad>
				print(123)
			</OnLoad>
		</Scripts>
	</Frame>
</Ui>

And then

/run a = CreateFrame("Frame", nil, nil, "testTemplate")   -- an error occured
/run LoadAddOn("!test")
/run a = CreateFrame("Frame", nil, nil, "testTemplate")   -- successfully print 123

Yeah I don't know why I removed the template. Maybe I was removing other xml stuff and said "well all I'm doing is changing one function in this template, I can just do that in lua instead"?

I'll roll it back to the template in XML. I will need to do this anyway eventually to make DropDown Menu support work. I can't use Blizzards DropDown Menu (taint issues) and will need to import LibUIDropDownMenu instead.

I saw in other issues that there is a Discord channel for EditModeExpanded. Maybe I can communicate with you better there than in this issue.

Okay, this commit moves it back into the xml. But as I now have to override GetSettingPool and ReleaseNonDraggingSliders to recognise the custom template, it means even more code than before.

Okay, this commit moves it back into the xml. But as I now have to override GetSettingPool and ReleaseNonDraggingSliders to recognise the custom template, it means even more code than before.

Only overwrite <OnLoad> script in XML will be fine, don't need to write other part, other part will inherits original template.

Even if code is more than before, I think we are worth to do it. we don't need to call and create cbrHandles every time we update settings. And I will try to do some cleanup.

Etern#6608, I must wait 10 mins to say something :(

True.

Okay so I remember why I said I can't use XML for libraries: because there is no way to ask "did we already load this addon? If so, don't load the rest of this XML".

In EditModeExpanded-1.0.lua, this is at the start:

local MAJOR, MINOR = "EditModeExpanded-1.0", 62

local lib = LibStub:NewLibrary(MAJOR, MINOR)

if not lib then return end

This stops the library loading again if its already loaded by another addon. I can't do this with XML.

It should be okay in most cases, but if we ever have to change that template, and another addon doesn't update the library, it could cause problems.

Yes, this is a very tricky question. Maybe this is the reason Blizzard use XML to create frame or template and other addons don't.

We use XML because we want to make full use of the OnLoad script, but OnLoad will only work just after CreateFrame, so it must defined in XML.

Some solutions I think:
solution1(best solution): We can add MINOR version checks in lua files to ensure that every plugin using this library is updated to the latest version(just like we check build before). If not updated, we can show a StaticPopup or print a warning.
solution2(alternative solution): Remove the template we defined in XML, and create a OnLoad function in lua, call it manually when pool create a new frame, like this:

local settingFrame = settingPool:Acquire()
if not settingFrame.IsInitialized then
    <OnLoad function we defined in lua>
    settingFrame.IsInitialized = true
end

solution3(worst solution): Revert the change we did before.

I have been very busy with some troublesome matters recently, I even don't have time to update my own UI addon to 10.1, so many of the things I wanted to do before have to be shelved for now. Sorry about that.

I'm going to stick with the current XML template. Its just something to keep in mind - if I ever have to make a change to it, need to remember that old versions of the library might cause problems.

Oh darn. This is what I meant. I wanted to use the library in another addon, but with both addons turned on:

Message: Interface/AddOns/WhereTheZaralek/libs/EditModeExpanded-1.0-62/EditModeExpanded-1.0.xml:3 Deferred XML Node object named EditModeExpandedSettingSliderTemplate already exists