Order of Operation bug with ObjectOp.opPropertyEnumerations

Here's the place for discussion related to CAM/CNC and the development of the Path module.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
captain_morgan
Posts: 30
Joined: Tue Mar 12, 2019 5:42 pm

Order of Operation bug with ObjectOp.opPropertyEnumerations

Post by captain_morgan »

Howdy all, while following up on this code review, @cam72cam pointed out I should be using opPropertyEnumerations. Looking through the code it appeared straight forward, override the method on the CustomOp class returning an object with the values for the Source property. But it just wouldn't work. After several hours hacking I realized the issue is in ObjectOp.__init__. What I see is that self.initOperation if called after self.opPropertyEnumerations(), resulting in the subclass not having had time to setup it's properties before defaults are applied. Moving code to set defaults to right after intiOperations seems to work as expected, though this could have side effects I'm not aware of.

Additionally part of this just kind of doesn't feel right, this is the resulting override I ended up with.

Code: Select all

    def opPropertyEnumerations(self, dataType="data"):
        customOpProperties = {
            "Source": [
                (translate("PathCustom", "Text"), "Text"),
                (translate("PathCustom", "File"), "File"),
            ]
        }

        enums = super().opPropertyEnumerations(dataType)
        Path.Log.debug(enums)

        if dataType == "raw":
            enums["Source"] = customOpProperties["Source"]

            return enums


        idx = 0 if dataType == "translated" else 1

        enums.append(("Source", [prop[idx] for prop in customOpProperties["Source"]]))
        Path.Log.debug(enums)

        return enums
Note the call to enums = super().opPropertyEnumerations(dataType), this feels like a mistake waiting to happen (like loosing CoolantMode). I feel like a better implementation if to move the content of ObjectOp.opPropertyEnumeration into __init__ and make it a stub function like other part of the abstraction. Following this method a subclassed Op would look more like...

Code: Select all

    def opPropertyEnumerations(self, dataType="data"):
        return {
            "Source": [
                (translate("PathCustom", "Text"), "Text"),
                (translate("PathCustom", "File"), "File"),
            ]
        }
Sorry if this is a lot to take in, I wanted to just brain dump some thoughts while they were fresh, start the conversation as I see some important areas that can be refactored.
Post Reply