#3325: DlgPropertyLink as editor of Tip property

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
abdullah
Posts: 3174
Joined: Sun May 04, 2014 3:16 pm

#3325: DlgPropertyLink as editor of Tip property

Postby abdullah » Sun Jan 28, 2018 3:36 pm

The Tip property of Mod/Part/BodyBase.h, is edited using DlgPropertyLink.

The dialog is populated using DlgPropertyLink::findObjects. The code is below.

This function seems made to populate features that do not depend on the (link parameter 3) body. This is about the contrary of what is needed in the case of a body.

I guess that the current DlgPropertyLink is thought for general property links and it is not suitable for body. However, I do not know much about this kind of property editors. I am not sure how should I do to have specific logic for the Tip PropertyLink, as it is body specific.
wmayer wrote:... commits ...
I won't say this bug is blocking for 0.17, but it is misleading for a user. I think we should either remove the editability of the Tip from the Property editor of the body, or fix it. I am not able to fix it.

Code: Select all

void DlgPropertyLink::findObjects(bool on, const QString& searchText)
{
    QString docName = link[0]; // document name
    QString objName = link[1]; // internal object name
    QString parName = link[3]; // internal object name of the parent of the link property

    bool isSingleSelection = (ui->listWidget->selectionMode() == QAbstractItemView::SingleSelection);
    App::Document* doc = App::GetApplication().getDocument((const char*)docName.toLatin1());
    if (doc) {
        Base::Type baseType = App::DocumentObject::getClassTypeId();
        if (!on) {
            App::DocumentObject* obj = doc->getObject((const char*)objName.toLatin1());
            if (obj) {
                Base::Type objType = obj->getTypeId();
                // get only geometric types
                if (objType.isDerivedFrom(App::GeoFeature::getClassTypeId()))
                    baseType = App::GeoFeature::getClassTypeId();

                // get the direct base class of App::DocumentObject which 'obj' is derived from
                while (!objType.isBad()) {
                    std::string name = objType.getName();
                    Base::Type parType = objType.getParent();
                    if (parType == baseType) {
                        baseType = objType;
                        break;
                    }
                    objType = parType;
                }
            }
        }

        std::vector<App::DocumentObject*> outList;
        App::DocumentObject* par = doc->getObject((const char*)parName.toLatin1());
        if (par) {
            // for multi-selection we need all objects
            if (isSingleSelection)
                outList = par->getOutList();
            outList.push_back(par);
        }

        // Add a "None" entry on top
        QListWidgetItem* item = new QListWidgetItem(ui->listWidget);
        item->setText(tr("None (Remove link)"));
        QByteArray ba("");
        item->setData(Qt::UserRole, ba);

        std::vector<App::DocumentObject*> obj = doc->getObjectsOfType(baseType);
        for (std::vector<App::DocumentObject*>::iterator it = obj.begin(); it != obj.end(); ++it) {
            Gui::ViewProvider* vp = Gui::Application::Instance->getViewProvider(*it);
            bool nameOk = true;
            if (!searchText.isEmpty()) { 
                QString label = QString::fromUtf8((*it)->Label.getValue());
                if (!label.contains(searchText,Qt::CaseInsensitive))
                    nameOk = false;
            }
            if (vp && nameOk) {
                // filter out the objects
                if (std::find(outList.begin(), outList.end(), *it) == outList.end()) {
                    QListWidgetItem* item = new QListWidgetItem(ui->listWidget);
                    item->setIcon(vp->getIcon());
                    item->setText(QString::fromUtf8((*it)->Label.getValue()));
                    QByteArray ba((*it)->getNameInDocument());
                    item->setData(Qt::UserRole, ba);
                }
            }
        }
    }
}
chrisb
Posts: 16867
Joined: Tue Mar 17, 2015 9:14 am

Re: #3325: DlgPropertyLink as editor of Tip property

Postby chrisb » Sun Jan 28, 2018 5:36 pm

abdullah wrote:
Sun Jan 28, 2018 3:36 pm
I won't say this bug is blocking for 0.17, but it is misleading for a user. I think we should either remove the editability of the Tip from the Property editor of the body, or fix it. I am not able to fix it.
Since I brought it up: Already while reading and thus before Abdullah's proposal I had the idea that removing it would be a more than acceptable solution. One way to perform this rare task is enough.
Nevertheless, the tip should of course be displayed in the body's DataTab.
wmayer
Site Admin
Posts: 14438
Joined: Thu Feb 19, 2009 10:32 am

Re: #3325: DlgPropertyLink as editor of Tip property

Postby wmayer » Sun Jan 28, 2018 6:19 pm

The property editor is considered as a universal tool to change the property of a feature or view provider and it basically doesn't care at all about certain limitations in the context it is used. This means it's in the user's responsibility not to make any nonsense with it. The same applies to the Python API where don't limit the possibilities either.

Now, if the risk of misuse is too high it would be fine to mark the Tip property as read-only.
But what looks wrong to me is that when you set the option to show all objects it doesn't show the objects that are part of a body's group property.
User avatar
DeepSOIC
Posts: 6639
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: #3325: DlgPropertyLink as editor of Tip property

Postby DeepSOIC » Mon Jan 29, 2018 12:34 am

Tip property is a scoped link with a very special scope. Use this information to populate the object list accordingly.
wmayer
Site Admin
Posts: 14438
Joined: Thu Feb 19, 2009 10:32 am

Re: #3325: DlgPropertyLink as editor of Tip property

Postby wmayer » Mon Jan 29, 2018 10:20 am

DeepSOIC wrote:
Mon Jan 29, 2018 12:34 am
Tip property is a scoped link with a very special scope. Use this information to populate the object list accordingly.
This information alone is not sufficient. For a body feature we would need a dialog that only shows objects that are part of a body's group property but the DlgPropertyLink is part of the core system and thus shouldn't and mustn't know anything about special behaviour in extension modules.
abdullah
Posts: 3174
Joined: Sun May 04, 2014 3:16 pm

Re: #3325: DlgPropertyLink as editor of Tip property

Postby abdullah » Mon Jan 29, 2018 1:12 pm

wmayer wrote:
Sun Jan 28, 2018 6:19 pm
The property editor is considered as a universal tool to change the property of a feature or view provider and it basically doesn't care at all about certain limitations in the context it is used.
I agree. For this reason, what I am trying to say is that either we define a different specific editor for the Tip within PDN, or we should not have the possibility to use the editor at all.
wmayer wrote:
Sun Jan 28, 2018 6:19 pm
Now, if the risk of misuse is too high it would be fine to mark the Tip property as read-only.
The risk of not understanding at all what is being shown in this dialog and why, is indeed high. It is totally misleading. It will be reported as a bug.
wmayer wrote:
Sun Jan 28, 2018 6:19 pm
But what looks wrong to me is that when you set the option to show all objects it doesn't show the objects that are part of a body's group property.
This, I think, is due to the implementation and the checks on the OutList. I did not wrote that code myself, but at the end there is a specific check to avoid inserting those features. "if (std::find(outList.begin(), outList.end(), *it) == outList.end()) {". It might have been coded this way to avoid circular dependencies. It is just a hypothesis.

I am unsure if that is a wanted behaviour for the general case of editing a propertylink.
abdullah
Posts: 3174
Joined: Sun May 04, 2014 3:16 pm

Re: #3325: DlgPropertyLink as editor of Tip property

Postby abdullah » Mon Jan 29, 2018 2:50 pm

BTW, Werner has already fixed it. It does not restrict to the active body, but it works more than reasonably well as a general tool for setting the tip.
TipEdit.png
TipEdit.png (82.07 KiB) Viewed 398 times
He is simply too good. We should be thankful everyday that we have him here.
chrisb
Posts: 16867
Joined: Tue Mar 17, 2015 9:14 am

Re: #3325: DlgPropertyLink as editor of Tip property

Postby chrisb » Mon Jan 29, 2018 5:04 pm

abdullah wrote:
Mon Jan 29, 2018 2:50 pm
BTW, Werner has already fixed it. It does not restrict to the active body, but it works more than reasonably well as a general tool for setting the tip.

He is simply too good. We should be thankful everyday that we have him here.
Yes he is, and yes I am.

Now I hardly dare to ask: is Cylinder missing on purpose because it is the current Tip?
abdullah
Posts: 3174
Joined: Sun May 04, 2014 3:16 pm

Re: #3325: DlgPropertyLink as editor of Tip property

Postby abdullah » Mon Jan 29, 2018 6:22 pm

chrisb wrote:
Mon Jan 29, 2018 5:04 pm
Now I hardly dare to ask: is Cylinder missing on purpose because it is the current Tip?
Indeed. He is that good :)
realthunder
Posts: 1018
Joined: Tue Jan 03, 2017 10:55 am

Re: #3325: DlgPropertyLink as editor of Tip property

Postby realthunder » Tue Jan 30, 2018 8:36 am

wmayer wrote:
Mon Jan 29, 2018 10:20 am
This information alone is not sufficient. For a body feature we would need a dialog that only shows objects that are part of a body's group property but the DlgPropertyLink is part of the core system and thus shouldn't and mustn't know anything about special behaviour in extension modules.
Hi wmayer, I have trouble understanding the logic of DlgPropertyLink for quite some time. I did some modification in my fork of FC, and it always causes me great trouble when merging. My question is, if DlgPropertyLink is for changing the content of a link property, why do you want to use that (soon to be changed) content for filtering? The OutList derived from that link property is going to be invalid soon anyway. The real danger is actually for user to select some object in the InList of the owner of the editing property, causing recursive dependency. So, why not simply use the recursive InList for filtering?
Try Assembly3 (latest version 0.10.2) along with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal