Data properties editor for Sketcher constraints

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
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Data properties editor for Sketcher constraints

Post by abdullah »

Dear Developers,

This is related to providing the possibility of changing the datum named properties of a sketch from the data widget.

I have a Property that is also a PropertyList (PropertyList derives from Property). This is the PropertyConstraintList in the Sketcher module. When properties associated to a QTreeView element are requested, the framework tries to get an editor for every property in that element. For those properties that it gets an editor, it calls the members of the editor to fill-in the properties.

The problem I have is that because my Property is a PropertyList, I do not have a single Item, but a lot of them (as many as named datum constraints). Basically the solution I have come to (inspired in PropertyVectorItem):

1) return as QVariant for the editor (ConstraintPropertyListItem) a QVariantList (that is a QVariant itself) and create a widget that can handle that QVariantList. I do not know which widget might be suitable for this?

So in the most simple case, use the same as PropertyVectorItem, so that you press the +, enter edition mode, a list of all the constraints appears, you change the constraint value, it updates, ...

- Do you know which widget could better be used if we finally go in this direction?

A) I have also thought of embeding another property editor widget (like the data property view, how is this called in QT, QTableEditor?) within that one row of the data property editor widget, and paint it within that item area (properly adjusting the size of that row)... What do you think of that solution?

B) Do you see an easier/tidier way of doing this than those mentioned above?

Sorry for the inaccurate language, I am not that proficient neither in QT nor in FreeCAD code.

Thanks in Advance,
Abdullah
Last edited by abdullah on Sat Aug 16, 2014 12:56 pm, edited 2 times in total.
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Several priorities to data widget from a PropertyList ed

Post by wmayer »

1) return as QVariant for the editor (ConstraintPropertyListItem) a QVariantList (that is a QVariant itself) and create a widget that can handle that QVariantList. I do not know which widget might be suitable for this?
So in the most simple case, use the same as PropertyVectorItem, so that you press the +, enter edition mode, a list of all the constraints appears, you change the constraint value, it updates, ...
- Do you know which widget could better be used if we finally go in this direction?
IMO, the best is to implement an editor similar to ConstraintPropertyListItem. The sub-items of this widget can keep their information as Base::Quantity which is able to be transported inside a QVariant. The sub-editor of a sub-item can be PropertyUnitItem.
A) I have also thought of embeding another property editor widget (like the data property view, how is this called in QT, QTableEditor?) within that one row of the data property editor widget, and paint it within that item area (properly adjusting the size of that row)... What do you think of that solution?
You mean a third tab page then? To me this doesn't sound very convenient. Btw, the widget is a QTreeView.
B) Do you see an easier/tidier way of doing this than those mentioned above?
A completely different way could be to implement an editor with a single row with a "..." button in it. When pressing on it a new modal dialog comes up where you can modify the properties.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Several priorities to data widget from a PropertyList ed

Post by abdullah »

Hi Werner,

Thanks for your patience. I do not properly call things.

I most probably have a problem because I do not understand how Property(Lists) work.

Properties get called by the current data widget. Those which can provide an editor get "added" to that data widget.

The case of a Property I think I understand:
PropertyXXXItem::value()
PropertyXXXItem::toString()
get executed to get the second column in the data widget (a string representation of the value). So in this case Constraints and whatever I put in the toString method

The problem I have is that my property is a propertylist (PropertyConstraintList). This property has as data member the list of constraints.

The problem I have is a one-to-many mapping.

If I have N constraints, what I would like is to have N calls to value and toString to fill N rows in the data widget. But I only get one call, like any other property, and the first column is already with the property name "Constraints".
IMO, the best is to implement an editor similar to ConstraintPropertyListItem.
The editor ConstraintPropertyListItem is created by me. It has the following:

Code: Select all

namespace SketcherGui {

class GuiExport PropertyConstraintListItem: public Gui::PropertyEditor::PropertyItem
{
    TYPESYSTEM_HEADER();

    virtual QWidget* createEditor(QWidget* parent, const QObject* receiver, const char* method) const;
    virtual void setEditorData(QWidget *editor, const QVariant& data) const;
    virtual QVariant editorData(QWidget *editor) const;

protected:
    virtual QVariant toString(const QVariant&) const;
    virtual QVariant value(const App::Property*) const;
    virtual void setValue(const QVariant&);

protected:
    PropertyConstraintListItem();
    
//protected:
    
};
The sub-items of this widget can keep their information as Base::Quantity which is able to be transported inside a QVariant.
Here widget I think you refer to the editor widget (e.g. QDoubleSpinBox for double values). Then the answer is yes, I have the Quantity information of all the constraints already in the propertylist PropertyConstraintsList. I could put this information into a editor widget and get it back (via QVariant) (I see no reason to localy store the values in the PropertyConstraintListItem, if I have to do it? why just not read it from the proplist when needed?).
The sub-editor of a sub-item can be PropertyUnitItem.
Ok. This might be my problem. How do I define a subeditor or sub-item in PropertyConstraintListItem? How would the flow work?

The only I can think of is:
Because I only get called once for value function, I only see that I could:
1. Get a sublist of Base:Quantity of the datum named constraints QList <Base:Quantity> and return this as QVariant (because it is a QVariantList, and I can pass a QVariantList as a QVariant).
2. Use an editor widget (e.g. QDoubleSpinBox for double values), that has two columns and as many rows as I want to define (as many as Datum Named Constraints). It will receive the QVariant of step 1. It would read the QVariantList and fill in the names and values (fill both columns for each constraint).
3. Upon edition return the QVariantList to be set.
You mean a third tab page then? To me this doesn't sound very convenient. Btw, the widget is a QTreeView.
No by no means a third tab. I was referring to the editor widget (property/value). Because "Constraints" have several names/values I would like to see under Constraints a constraint/value table. Maybe I need a brain upgrade. In FreeCAD there is a TreeView on top and properties/value under the TreeView. Is the properties/value part of the QTreeView widget? If yes, I do need an upgrade :lol:

I am really thrilled to know how your solution with the "PropertyUnitItem" would work. Could you please tell me?
wmayer
Founder
Posts: 20243
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Several priorities to data widget from a PropertyList ed

Post by wmayer »

If I have N constraints, what I would like is to have N calls to value and toString to fill N rows in the data widget. But I only get one call, like any other property, and the first column is already with the property name "Constraints".
For a Vector it's easy because we always have three elements and thus the toString() can e.g. return "[1.2, 2.7, -3.8]". For a constraint list you can have many elements. So, I would do: if there is only one datum constraint write e.g. "2.56 mm" if there are more then simply write "[2.56 mm, ...]". The toString() is just a visual helper for the user but we don't transport any important back and forth this way.
he only I can think of is:...
Now it gets a bit complicated to follow.
I am really thrilled to know how your solution with the "PropertyUnitItem" would work. Could you please tell me?
Give me some time.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Data properties editor for Sketcher constraints

Post by abdullah »

Ok. I really needed to better understand how properties work.

What is this?
constraints_editor.png
constraints_editor.png (27.36 KiB) Viewed 3371 times
I have a preliminary working version in:
repo: https://github.com/abdullahtahiriyo/Fre ... master.git
branch: Sketcher_properties_constcast

EDIT: I have rebased onto master all the changes in a single commit in branch: Sketcher_properties_constcast_rebased

You can inspect the differences here:
https://github.com/abdullahtahiriyo/Fre ... 7bd2324bb5


The beauty (by contrast of what follows)

It works (or at least it seems to work in the little test I did, I am sure that a pro bug-finder can do much better).

The beast

The code is awful (even for my standard). I want to explain what I did. The problems that I encountered. How I solved them. Maybe (most probably) you will have better ideas that can contribute the a less awful code (something Werner can actually accept).

The main challenge of making the constraints appear in the data editor is that you do not have a fixed number of elements (like, for example, in a 3d vector). This means that the propertyItems (each of the rows under constraints) have to be created dynamically (are qt dynamic properties). This generates a plurality of problems (e.g. failure to link from the main editor PropertyConstraintListItem to the sub-Items, based on PropertyUnitItem (no READ WRITE, like in Q_PROPERTY for dynamic properties), imposibility to create the Items from the existing PropertyItem infrastructure because all the virtual functions are const, except for the constructor, failure to properly show in the GUI,...).

This is the class declarations:

Code: Select all

namespace SketcherGui {

class GuiExport PropertyConstraintUnitItem: public Gui::PropertyEditor::PropertyUnitItem
{
    TYPESYSTEM_HEADER();
    
public:
    virtual bool setData (const QVariant& value);

protected:
    virtual void setValue(const QVariant&);

    PropertyConstraintUnitItem();
};


class GuiExport PropertyConstraintListItem: public Gui::PropertyEditor::PropertyItem
{
    Q_OBJECT
    TYPESYSTEM_HEADER();

    virtual QWidget* createEditor(QWidget* parent, const QObject* receiver, const char* method) const;
    virtual void setEditorData(QWidget *editor, const QVariant& data) const;
    virtual QVariant editorData(QWidget *editor) const;
    
public:
    virtual void setDynamicProperty( const char * name, const QVariant & value );

protected:
    virtual QVariant toString(const QVariant&) const;
    virtual QVariant value(const App::Property*) const;
    virtual void setValue(const QVariant&);
    
protected:
    PropertyConstraintListItem();
    void fillInSubProperties(const App::Property* prop, QString &valuestr) const;
    
protected:
    Gui::PropertyEditor::PropertyUnitItem * dummy;
    QList<SketcherGui::PropertyConstraintUnitItem> propertyUnitItem;
    bool isInit;
};

PropertyConstraintListItem is the editor now returned by the PropertyConstraintList, so is the class responsible for editing the values of the constraints.

PropertyConstraintUnitItem inherits from PropertyUnitItem to provide some extra functionality needed for a dynamic property.

Ugliness number one

Gui::PropertyEditor::PropertyUnitItem * dummy;

As the name suggests this is not part of the Gui or the object. If I do not define this, and add this append this static property as child, I do not get in the GUI, on the property editor the square with an (+ or -) symbol inside allowing to expand the Constraints property to see the editor for each of the constraints. The functionality is there (you can click in the area where the symbol should appear), but the symbol does not appear.
Expand_square_sign.png
Expand_square_sign.png (4.03 KiB) Viewed 3371 times
Why this happens, I do not know. I guess it is related to static/dynamic properties. If you know a better solution, please tell me!!

BTW, I remove it from the list via a PropertyItem->reset() see below

Ugliness number two

Adding dynamically the PropertyUnitItems to the list has been very challenging. The only non-const function is the constructor, but at that time, the PropertyConstraintList (property) has not been yet assigned to the PropertyConstraintListItem, so I simply do not know how many datum&named constraints I have, and I can not create the corresponding PropertyUnitItems (as opposed to the dummy variable, that is one and I can create there).

My first solution was to add a virtual non-const "Buildup" function in PropertyItem.h and made it be called before each Value call in the PropertyModel. However, to do this, I finally opted out of messing with the remainings of the implementation, and use one of the existing functions (Value) with the awful const_cast.

The current solution constructs the subItems in that Value function, by shamefully const_casting this to get away it. A member function (isInit) ensures that this shameful process is executed only once. The main advantage of this implementation is that requires no modifications to PropertyItem. The Value function also sets the values of all this subeditors (the flow is different from the other editors, as I can not access the setValue and Value of the sub-Items from the PropertyConstraintListItem).

Alternative solution I though about (not implemented). In PropertyItem, in the function that sets the PropertyData from the PropertyModel (i.e. setPropertyData) could either: A) be declared virtual, and provide a new definition in PropertyConstraintListItem that just calls the base implementation + does the initialization (it is not const, so no const_cast) B) create a new virtual function in PropertyItem (e.g. virtual void initialize(const std::vector<App::Property*>&)), call it from setPropertyData, empty function in PropertyItem, overridden in our editor to do our initialization. The parameter is not even needed in B, because I can get the info there by "const Sketcher::PropertyConstraintList* item=static_cast<const Sketcher::PropertyConstraintList*>(getPropertyData()[0]);". Just for illustrative purposes.

Ugliness number three

It comes from the inability to access the getValue/setValue of the sub-Items with dynamic properties. In normal properties it gets set in:

Code: Select all

bool PropertyItem::setData (const QVariant& value)
{
    // This is the basic mechanism to set the value to
    // a property and if no property is set for this item
    // it delegates it to its parent which sets then the
    // property or delegates again to its parent...
    if (propertyItems.empty()) {
        PropertyItem* parent = this->parent();
        if (!parent || !parent->parent())
            return false;
        parent->setProperty(qPrintable(objectName()),value);
        return true;
    }
    else {
        setValue(value);
        return true;
    }
}
Which in the case of a sub-editor executes the "parent->setProperty(qPrintable(objectName()),value);". To "reroute" this, I was forced to declare bool PropertyItem::setData (const QVariant& value) as virtual in PropertyItem.h, do not use PropertyUnitItem, but a new class derived from it PropertyConstraintUnitItem that provides an own implementation (overrides this function):

Code: Select all

bool PropertyConstraintUnitItem::setData (const QVariant& value)
{
    if(this->parent()->getTypeId().isDerivedFrom(PropertyConstraintListItem::getClassTypeId())){

        PropertyConstraintListItem * parent = static_cast<PropertyConstraintListItem *>(this->parent());
        
        if (!parent || !parent->parent())
            return false;
    
        //parent->setDynamicProperty(qPrintable(objectName()),value);
        parent->setDynamicProperty(this->propertyName().toStdString().c_str(),value);    
        return true;
    }

    return false;
 
}
So this subeditor is specific to the PropertyConstraintListItem editor.

Ugliness number four

Actually the only way I found to actually edit the constraints, which is a continuous abuse of const_cast

Code: Select all

void PropertyConstraintListItem::setDynamicProperty( const char * name, const QVariant & value )
{
    const Sketcher::PropertyConstraintList* item=static_cast<const Sketcher::PropertyConstraintList*>(getPropertyData()[0]);
    
    const std::vector< Sketcher::Constraint * > &vals = item->getValues();
    
    std::string sname(name);
    
    int i=0;
    for(std::vector< Sketcher::Constraint * >::const_iterator it= vals.begin();it!=vals.end();++it,++i){

        if((*it)->Name==sname){
            
            const Base::Quantity& val = value.value<Base::Quantity>();
            
            const_cast<Sketcher::Constraint *>((*it))->Value=val.getValue();
            
            const_cast<Sketcher::PropertyConstraintList*>(item)->set1Value(i,(*it));
            return;
  
        }
    }
}
Well, :oops: I wait for your comments, suggestions, bugs, ...
User avatar
yorik
Founder
Posts: 13640
Joined: Tue Feb 17, 2009 9:16 pm
Location: Brussels
Contact:

Re: Data properties editor for Sketcher constraints

Post by yorik »

My 2 cents:

I didn't test your code yet so I might be out of scope here, but my overall impression is that there is a pretty heavy mechanism in place to create these dynamic "expandable" property, which will cause headache at a moment or another when someone will want to extend or reuse that... Plus, the mix between the sketcher code and the properties system, which looks a bit unavoidable and at the same time feels wrong, as Werner already said above I think

Wouldn't it be a better strategy to use an existing property type that allow a variable number of subelements? I think the PropertyMap would be interesting in such a case, because its string:string type restriction is not much a problem here (the key would be the constraint name, thus a string, and the value, since it will pass through the unit parser, also a string...). And instead of putting alot of sweat in the internals, you could make a good editor for PropertyMaps, a bit like how the PropertyStringList editor works...

Also, there would be more advantages to this: Suppose a sketch is used as underlying base to a higher-level object (for example an object that has an underlying sketch, and an extrusion vector, to produce an extruded solid). That object could easily reflect the PropertyMap of the underlying sketch, so in the final object you could be able to modify some dimensions of the underlying sketch... This might be tremendously powerful (I'm thinking for example, in architectural context, of a window, based on a sketch, and in the window properties you would be able to modify the height and width, which would modify the same property of the base sketch).

Sorry if this sounds critical, but it really occurred to me only now, after seeing your results... Not sure I'm right either. In any case, this is a very useful topic to discuss.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Data properties editor for Sketcher constraints

Post by abdullah »

Dear Yorik,

I very much appreciate your comments. I am willing to hear critics more than compliments. It is by critics that one learns. Specially well reasoned critics.

I am very new to FreeCAD and I am learning together its internals, qt like properties and concepts of c++ new to me like const_cast. Therefore it is not surprising that it takes me a couple of weeks to reach to an inferior solution. Well I have learnt a lot in these two weeks...

I do not know that PropertyListItem. I will have a look into it. That might solve ugliness 1-3, I would still need const_cast for applying the new values (set dynamic property function above). I am not sure (due to lack of knowledge) how could I make this work graphically (which editor widget) could actually be so good integrated in the property view as the propertyunititems. It is different from showing one text list (variable number of value items) associated to a single property name...

I will give it some extra thought to this idea...

All the implementation above is Sketcher specific and is in the SketcherGui namespace. I rely only in one function of GUI namespace made virtual... I mean, I am not polluting Gui namespace (though I might be polluting SketcherGui... :oops:
User avatar
yorik
Founder
Posts: 13640
Joined: Tue Feb 17, 2009 9:16 pm
Location: Brussels
Contact:

Re: Data properties editor for Sketcher constraints

Post by yorik »

The "mix" I was referring to is the sketcher putting its hands inside the properties stuff... Ideally you should see this as two systems that communicate but can exist one without the other (what happens inside the sketcher and what happens in the properties editor).

I would honestly consider Werner's suggestion, that is, not trying to build something integrated into the properties editor at the moment, but go for an external dialog, like PropertyStringList (The App::Annotation object has one for example, or the Drawing::Page). This would remove a lot of the difficulty, keep things clean and easy to develop further. Later on, you or someone else can push that further by removing the dialog and make the property directly editable in the properties editor.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Data properties editor for Sketcher constraints

Post by abdullah »

Ok. Now I understand what you mean by "mix".

Part of the mix is unavoidable (and part of Werner's original prefered solution, see IMO his post). The reasons are:
1. PropertyConstraintList is defined in the Sketcher module, so unless you use a standard editor from Gui module (meaning (a)using a stock one or (b) putting my hands in Gui namespace and inserting my own, and here I mean inserting a general one, reusable, not specific to PropertyConstraint list), an specific editor should go in that namespace (makes sense to me and was Werner's reply). I implemented the specific editor, you propose if I understand you well solution (b).
2. PropertyContraintList is a PropertyList (that derives from property), I could certainly try to adapt PropertyItem to handle PropertyItemLists differently, opening the possibility of providing general support for dynamic properties, until the point where the value of the items (Contraints) within the PropertyList must be read or written (this I did not due to a hands off Gui namespace policy, but that could change if sufficient motivation is found to have a generic editor of this form, until said point where generic code stops). These "Contraints" are not properties, so I can not imagine how to modify the value of an specific contraint with generic code (unless you force all items of a PropertyList to inherit from some common class.
3. No matter the solution selected, (a), (b) or the current one, that limitation is always present, which forces (unless someone knows a way) at least to have a specific editor derived from that generic code to implement the specific code of setting/modifying the element of the PropertyList. An alternative for easier cases could be make a new full PropertyList and set it, however this works when you edit full objects (not only the value of a datum constraint, but also all the remaining info of a constraint (to which elements it is applied, ...), and all the objects must be written (also non-datum contraints like Horizontal)... Let me know if you know better...

Going for an external editor dialog is definitely something I would rather not do. Then it would not take much longer to open the Sketch and do the changes (a little longer of course). In any case, I do not see how to avoid problem 3 above.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Data properties editor for Sketcher constraints

Post by abdullah »

As a result of the discussion with Yorik, I have reconsider the possility of including some logic in PropertyItem (Gui namespace) to allow supporting dynamically generated properties. This solves the three ugliness shown before (now it relies on PropertyUnitItem, no more PropertyConstraintUnitItem; no initialization from Value function const_casting everything, no dummy, no reset), while still the code of PropertyConstraintListItem relies on const_cast (much less than before). I will still try to improve this. However, I show you what I did, so that you tell me if those modifications to PropertyItem are likely to be integrated into master or not.

The modifications are two new virtual functions:
initialize() to init a Item after the properties are known to it.
a setProperty wrapper, setQTProperty that in the general implemention just calls setProperty, but allow binding with the READ of Q_PROPERTY for a subprotety.

same repo, branch: Sketcher_properties_generic_dynamic_properties_rebased

All rebased for easy code inspection in:
https://github.com/abdullahtahiriyo/Fre ... 2a45adcf5a

Please let me know your impressions...
Post Reply