Sanity check change to ParameterGrp

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
chennes
Veteran
Posts: 3884
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Sanity check change to ParameterGrp

Post by chennes »

Right now the functions "ParameterGrp::GetASCIIs" and "ParameterGrp::GetASCIIMap" ignore any FCText elements that do not have any text: this means that if an FCText element is set to an empty string, it's not included in the returned map. This causes problems for the Preference Packs, because actually the default value of the StyleSheet preference is an empty string. But it's getting stripped out by GetASCIIMap, so the code that applies the preference pack skips it.

In order to make this work, I need at least GetASCIIMap to include FCText elements that have empty strings, so I propose modifying the code to add the else condition here (this is around line 780 of Parameter.cpp):

Code: Select all

            DOMNode *pcElem2 = pcTemp->getFirstChild();
            if (pcElem2)
                vrValues.emplace_back(Name, std::string(StrXUTF8(pcElem2->getNodeValue()).c_str()));
            else
                vrValues.emplace_back(Name, std::string()); // For a string, an empty value is possible and allowed
Can anyone think of things this might break that I should test before submitting? I'm also thinking of adding those empty strings into "GetASCIIs" for consistency, but that's not important to my PR because I don't use that one, so I could go either way on that.
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sanity check change to ParameterGrp

Post by wmayer »

chennes wrote: Sat Aug 21, 2021 12:58 am Can anyone think of things this might break that I should test before submitting? I'm also thinking of adding those empty strings into "GetASCIIs" for consistency, but that's not important to my PR because I don't use that one, so I could go either way on that.
The change looks good to me because leaving the value of the element empty is the only way to support empty strings.

However, for consistency reasons the method ParameterGrp::GetASCII() should be adjusted, too.

Code: Select all

std::string ParameterGrp::GetASCII(const char* Name, const char * pPreset) const
{
    // check if Element in group
    DOMElement *pcElem = FindElement(_pGroupNode,"FCText",Name);
    // if not return preset
    if (!pcElem) {
        if (pPreset==0)
            return std::string("");
        else
            return std::string(pPreset);
    }
    // if yes check the value and return
    DOMNode *pcElem2 = pcElem->getFirstChild();
    if (pcElem2)
        return std::string(StrXUTF8(pcElem2->getNodeValue()).c_str());
    else if (pPreset==0)
        return std::string("");

    else
        return std::string(pPreset);
}
When evaluating the value of the element (i.e. checking if pcElem2 is null) it should not check again pPreset but it should return an empty string.

Code: Select all

...
    // if yes check the value and return
    DOMNode *pcElem2 = pcElem->getFirstChild();
    if (pcElem2)
        return std::string(StrXUTF8(pcElem2->getNodeValue()).c_str());
    else
        return std::string("");
This is because when you have this XML block

Code: Select all

      <FCParamGroup Name="Group">
        <FCText Name="Attribute"></FCText>
      </FCParamGroup>
The function GetASCIIMap() would return {"Attribute" : ""} while GetASCII("Attribute", "Default") would return "Default" instead of "".
The default value "Default" should only be returned if the passed attribute name doesn't exist, i.e. GetASCII("UnknownAttribute", "Default") returns "Default".
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Sanity check change to ParameterGrp

Post by wmayer »

User avatar
chennes
Veteran
Posts: 3884
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Sanity check change to ParameterGrp

Post by chennes »

Nice, thanks!
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
Post Reply