Code review of merged Link3 branch

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
wmayer
Site Admin
Posts: 14624
Joined: Thu Feb 19, 2009 10:32 am

Re: Code review of merged Link3 branch

Postby wmayer » Fri Sep 13, 2019 10:56 pm

git commit 503f987cf fixes the most urgent issue. With the method allowOverrideViewProviderName() all document objects by default don't allow to override its view provider type.
realthunder
Posts: 1103
Joined: Tue Jan 03, 2017 10:55 am

Re: Code review of merged Link3 branch

Postby realthunder » Mon Sep 16, 2019 2:41 am

wmayer wrote:
Fri Sep 13, 2019 10:56 pm
git commit 503f987cf fixes the most urgent issue. With the method allowOverrideViewProviderName() all document objects by default don't allow to override its view provider type.
I think this allowOverrideViewProviderName() function should be added to ViewProvider, because whether it is safe for use as an override or not is really determined by the corresponding ViewProvider's implementation. Maybe shorten the name to just allowOverride(). This function can be called right after view provider creation.

Edit: even better, define the function as allowOverride(pObj) and let the view provider decide whether the object is supported or not. Currently, App::ViewProviderLink and PartGui::ViewProviderPartExt can accept any object.
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
wmayer
Site Admin
Posts: 14624
Joined: Thu Feb 19, 2009 10:32 am

Re: Code review of merged Link3 branch

Postby wmayer » Mon Sep 16, 2019 9:47 am

I think this allowOverrideViewProviderName() function should be added to ViewProvider, because whether it is safe for use as an override or not is really determined by the corresponding ViewProvider's implementation. Maybe shorten the name to just allowOverride(). This function can be called right after view provider creation.
Yes, it's true that a DocumentObject can't really decide if its ViewProvider has this flexibility or not, only the ViewProvider itself knows it. When I realized the consequences of git commit ff1d1cd341 I was not really sure what the intended behaviour is supposed to be. Is it really to allow to couple an arbitrary ViewProvider with a DocumentObject or was the actual intention to allow that a Python proxy can implement the method getViewProviderName().

For the former I couldn't find an example in the code where this is currently used (and probably there aren't many examples where this is even useful) and for the latter I only found a single example in the Draft workbench.

So, because I was not quite sure about the intentions and plans I decided to implement a quick fix that doesn't touch too much of the new possibility but at least avoids its downside.
Edit: even better, define the function as allowOverride(pObj) and let the view provider decide whether the object is supported or not. Currently, Gui::ViewProviderLink and PartGui::ViewProviderPartExt can accept any object.
OK, so then it must be checked inside Gui::Document::slotNewObject() whether the created view provider is compatible and if not an instance of the type delivered with getViewProviderName() must be created.
realthunder
Posts: 1103
Joined: Tue Jan 03, 2017 10:55 am

Re: Code review of merged Link3 branch

Postby realthunder » Mon Sep 16, 2019 10:26 am

wmayer wrote:
Mon Sep 16, 2019 9:47 am
OK, so then it must be checked inside Gui::Document::slotNewObject() whether the created view provider is compatible and if not an instance of the type delivered with getViewProviderName() must be created.
I'll submit a PR for that soon.
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