Dynamically Hideable Property Status

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!
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: Dynamically Hideable Property Status

Post by realthunder »

wmayer wrote: Sat May 16, 2020 12:06 pm The old and more efficient behaviour can be easily restored. Therefore the PR should be reverted and inside the function PropertyView::onTimer() it must be avoided to use isPropertyHidden() because this checks for App::Prop_Hidden or the status bits. Instead a function is needed that only checks the App::Prop_Hidden.
The reason I did this is because App::Link has the ability to show property of a linked object, but only if the link itself does not have a property with the same name, or the link's property is marked as hidden. In other word, the Hidden bit actually affects what are the properties in the model. Refreshing the the property model is not a particularly expensive operation. What's expensive in the past is to refresh it immediately in response to various changes, especially selection change, which is why I put it in a timer.

wmayer wrote: Sat May 16, 2020 11:49 am Select the object in the tree view and in the property editor set Toggle to True. You will see that the combo box will be destroyed and you have to click the item again.
That is indeed a problem, but I'd say a minor one. If the programmer absolutely don't want this, then he can defer the change till recompute. In fact, I could argue that we should actually destroy the combo after user has made the selection, so that we can perform the recompute. Because for combo box, it is easy to detect user's intention that a choice has been made, and he expects to see the change, but often times visual change only happen after recompute. With the current implementation, the user must either click somewhere else or hit 'Return' key, which I myself find quite annoying.
Try Assembly3 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
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Dynamically Hideable Property Status

Post by abdullah »

wmayer wrote: Sat May 16, 2020 11:49 am The most important point is that the "Hidden" flag of the status bits of a property is ignored and as long as the App::Prop_Hidden is not set the property is always added. Afterwards the status bit for "Hidden" is checked and if set the item is made invisible with the call of QTreeView::setRowHidden(..., true).
Thanks for this insight. So indeed there was a intended difference between Prop "static" properties, such as Prop_Hidden and object specific ones, such as "Hidden".

Not rebuilding the model and only adding those "dynamically hideable" is what I intended. But I did not appreciate to leverage that difference and my PR involved creating a new status. This, I thought it was too much if it could be avoided.
wmayer wrote: Sat May 16, 2020 11:49 am Now with v0.19 the behaviour is quite different and very often the full model is reset which is way more expensive than just changing individual items and it has the bad side-effect that if an item has an active editor it will be destroyed.
I was aware of the rebuild of the model, but I totally missed the editor being destroyed. In the case of the combobox it is rather subtle to the user.

I am trying to think which kind of property changes would trigger a rebuild of the model while an editor is active would impact the user. If we would update the model while editing a property (have a length in the 3D view react to changes while editing), then it will have a high impact.
realthunder wrote: Sat May 16, 2020 11:26 pm The reason I did this is because App::Link has the ability to show property of a linked object, but only if the link itself does not have a property with the same name, or the link's property is marked as hidden. In other word, the Hidden bit actually affects what are the properties in the model.
I do not see how Werner's described behaviour affects App::Link. If a Property has the "PropHidden" status bit, it will not be added to the model regardless of the "Hidden" status bit. If it has the "Hidden" status bit (but not the "PropHidden" one), it will indeed be added to the model, but it won't be shown (if we update the showing properly (my PR allowed Hidden properties into the mode and did not show them, there is a run after constructing the mode hiding the rows of hidden properties)).

Please, let me know, if you differ.
realthunder wrote: Sat May 16, 2020 11:26 pm Refreshing the the property model is not a particularly expensive operation. What's expensive in the past is to refresh it immediately in response to various changes, especially selection change, which is why I put it in a timer.
If a rebuild of the model is necessary, the timer solution is IMO a good one, but I think we should consider if rebuilding the model is indeed necessary. Werner's solution appears to avoid rebuilding the model while covering all the functionality we (all) want.
realthunder wrote: Sat May 16, 2020 11:26 pm That is indeed a problem, but I'd say a minor one. If the programmer absolutely don't want this, then he can defer the change till recompute. In fact, I could argue that we should actually destroy the combo after user has made the selection, so that we can perform the recompute. Because for combo box, it is easy to detect user's intention that a choice has been made, and he expects to see the change, but often times visual change only happen after recompute. With the current implementation, the user must either click somewhere else or hit 'Return' key, which I myself find quite annoying.
I am failing to understand your point.

I am not sure if you say that you want to have updates/recomputes while editing (like we have when editing in the Task panels). We did not have this before this PR and we do not have it after this PR.
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Dynamically Hideable Property Status

Post by wmayer »

realthunder wrote: Sat May 16, 2020 11:26 pm The reason I did this is because App::Link has the ability to show property of a linked object, but only if the link itself does not have a property with the same name, or the link's property is marked as hidden. In other word, the Hidden bit actually affects what are the properties in the model.
I agree with Abdullah that don't really understand your argument. I don't see why it shouldn't be possible to add those properties and simply hide them if they shouldn't be shown.
Refreshing the the property model is not a particularly expensive operation. What's expensive in the past is to refresh it immediately in response to various changes, especially selection change, which is why I put it in a timer.
This is not fully correct because it depends on when e.g. adding user-defined properties. At the time when extending the capabilities of the property editor I remember a use case where adding or removing properties is controlled by another property.

Look at this (somewhat constructed) example that shows two problems of the current implementation:

Code: Select all

class TestObject:
  def __init__(self, fp):
    fp.Proxy = self
    fp.addProperty("App::PropertyInteger","Number").Number = 1
  def onChanged(self, fp, prop):
    if prop == "Number":
      num = fp.Number
      fp.addProperty("App::PropertyVector","Point{}".format(num))
  def execute(self, fp):
    print("execute")

doc=App.newDocument()
obj=doc.addObject("App::FeaturePython","test")
TestObject(obj)
Select the object in the tree view and change the Number property. When doing so in v0.19 then
  • it always destroys the editor widget as said above
  • each time the PropertyModel is reset it will automatically recompute the document
And depending on how complex your document is this can easily become a very expensive operation.

When testing the code snippet with v0.18 then the editor widget stays intact and the document is recomputed at the very end when the editor widget loses the focus.
In fact, I could argue that we should actually destroy the combo after user has made the selection, so that we can perform the recompute.
I disagree. The property editor has been designed and implemented to delay the recompute of a document as long as possible and do it at the very end when the editor destroyed by manually changing the focus. This is because the property editor is a very general mechanism and has no knowledge of how expensive a recompute can be.
Because for combo box, it is easy to detect user's intention that a choice has been made, and he expects to see the change, but often times visual change only happen after recompute. With the current implementation, the user must either click somewhere else or hit 'Return' key, which I myself find quite annoying.
But if a little change can cause a recompute of several seconds or minutes what is more annoying?

In many of our specialized editors (e.g. the sketcher) we naively performed a recompute for each little change and as long as a document was small you never realized what bottleneck this can become. After users started to complain a lot about the slowness we started (special thanks to Abdullah) to remove all these automatic recomputes. We shouldn't make the same mistake twice.
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: Dynamically Hideable Property Status

Post by realthunder »

wmayer wrote: Sun May 17, 2020 10:34 am
realthunder wrote: Sat May 16, 2020 11:26 pm The reason I did this is because App::Link has the ability to show property of a linked object, but only if the link itself does not have a property with the same name, or the link's property is marked as hidden. In other word, the Hidden bit actually affects what are the properties in the model.
I agree with Abdullah that don't really understand your argument. I don't see why it shouldn't be possible to add those properties and simply hide them if they shouldn't be shown.
I was explaining why I choose not to include property with 'Hidden' status bit. Because the property view will populate the linked object's property if a link is selected, as shown in the screencast below with green font (which is broken in the upstream, and fixed in one of my pending PR). Unless, the link itself contain a property of the same name, but not hidden. So depending on the 'Hidden' status bit of Link's property, the property view may either show the link's property, or the linked object's property. The difficult part is that they both have the same name. The property population is done in PropertyView::onTimer(). It is possible to include both properties, but requires more complex data structure in an already not so simple algorithm. And the PR I submit can't really deal the complexity introduced by linked property. As shown in the screencast, a manual refresh is required to show back the green linked property.
link-property.gif
link-property.gif (267.44 KiB) Viewed 1218 times
And depending on how complex your document is this can easily become a very expensive operation.
Yes, I agree. Full update should be avoided if possible. It is just not as simple as you have put it.

In fact, I could argue that we should actually destroy the combo after user has made the selection, so that we can perform the recompute.
I disagree. The property editor has been designed and implemented to delay the recompute of a document as long as possible and do it at the very end when the editor destroyed by manually changing the focus. This is because the property editor is a very general mechanism and has no knowledge of how expensive a recompute can be.
This is obviously a usability issue. I specifically mention the combo box. If the user opens the drop down and selects a different option, it is an obvious indication that he has finished editing. The user is often not aware of this lost focus recomputation step. As a matter of fact, I as a developer occasionally forget this when testing, and wondering why there is no change. The only complication may be to differentiate selection without activate drop down, e.g. through mouse wheel.
Try Assembly3 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
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Dynamically Hideable Property Status

Post by abdullah »

realthunder wrote: Sun May 17, 2020 1:47 pm I was explaining why I choose not to include property with 'Hidden' status bit. Because the property view will populate the linked object's property if a link is selected, as shown in the screencast below with green font (which is broken in the upstream, and fixed in one of my pending PR). Unless, the link itself contain a property of the same name, but not hidden. So depending on the 'Hidden' status bit of Link's property, the property view may either show the link's property, or the linked object's property. The difficult part is that they both have the same name. The property population is done in PropertyView::onTimer(). It is possible to include both properties, but requires more complex data structure in an already not so simple algorithm. And the PR I submit can't really deal the complexity introduced by linked property. As shown in the screencast, a manual refresh is required to show back the green linked property.
Unfortunately I continue to fail to fully understand the issue. I think that there must be something that it is obvious for you and that you are not saying that is not obvious for me. I would appreciate if you could take the time to try to understand what this is.

First sentence

I understand that you choose not to include the properties with 'Hidden' status bit (not PropHidden, but Hidden). From this statement I understand there must be an underlying problem that you are trying to solve.

Second sentence
I understand that, you chose to do this because the property view would populate the linked object's property if a link is selected.

When you say "the linked object's property", I am not sure if you refer to a specific property or you mean the properties.

This is an example of a link:
Screenshot_20200518_152322.png
Screenshot_20200518_152322.png (49.54 KiB) Viewed 1165 times
The link is selected. I see the linked object properties.

I see a contradiction in that it appears that the wanted behaviour is to see the liked object properties. You indicate that you chose to exclude the "hidden" properties in the list because the linked properties will be populated (as if this were a problem).

I am beginning to think that maybe you are using the "hidden" status for something specific in the link implementation that goes beyond wanting or not wanting to show a property. Am I right? If yes, could you please elaborate?

Third sentence

Unless, the link itself contain a property of the same name, but not hidden. So depending on the 'Hidden' status bit of Link's property, the property view may either show the link's property, or the linked object's property.

I see that you introduce the aspect of properties having a same name. I can understand that properties having a same name may be an issue. But I am not sure I understand why a link will have a property with the same name hidden and not hidden. I could understand that a link and the linked object have a property with the same name (for example "Label" in the example above, the Link has "Cube001" and the Linked object has "Cube"). However, I do not see why one of them would be "Hidden". Conversely, I can understand that Cube will have several hidden properties (PropHidden) not shown (because they are not relevant to the user for editing purposes) and Cube001 may well have several such feature too.

At this point, I am beginning to think that you may be using the "Hidden" status bit for something that has a different meaning that "do not show it to the user because it is not relevant for him". This might be the reason why I cannot understand your argument.

My understanding is based in that you mark a property "Hidden" when it is not relevant for the user editing the object. However, this is in my understanding the same for a link and linked object. This is, if the decision is to show the properties of a linked object in the link (for example to be able to change the original size directly from the link, as in the cube above), then it should show the same properties that were not hidden in the linked object in the link.

If this is like this, save for the issue of the "same name", the PropHidden/Hidden mechanism described by Werner should work fine. If this is not the case, I would appreciate understanding it.

Regarding the issue with the "same name", if hiding is used to avoid duplicated property names in the property model, this might be an indication that it may not be the right mechanism.

I do not want to extend myself further. I think you will be able to understand why I am not understanding you from the text above. Hopefully I will read your answer in a day I am bright enough ;)
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: Dynamically Hideable Property Status

Post by realthunder »

abdullah wrote: Mon May 18, 2020 2:17 pm Unfortunately I continue to fail to fully understand the issue. I think that there must be something that it is obvious for you and that you are not saying that is not obvious for me.
Just compare your picture with my screencast above, you'll see the obvious difference. Mine is running from my fork where the property font is correctly displayed. The Green properties are not from Link, but from the linked Box. If I add a property of the same name, say 'Length' to Link, then it will display as Black, because it replaces the property with the same name in the view. I think the thing you are missing is to realize the fact that the green properties does not belong to the Link, so there is no 'Hidden and not Hidden' at the same time as you have put it. The benefit of displaying linked object's property is that you can modify those properties and see immediate change in the Link. The reason to have a mechanism to mask linked object's property is so that Link can have overriding behaviors, such as the 'Placement'.

You can find out more technical reason by checking out the implementation of PropertyView::onTimer(), regarding using std::map for sorting and searching etc. It is not a simple matter as to replace it with, say multimap, which requires additional state variable to mark which one is chosen. I will improve the full update when I got time. I am thinking of keeping the existing data structure, not clear the property tree before populating, and perform a search and replace for each existing property tree item, similar to what I did in FC tree view.
Try Assembly3 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
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Dynamically Hideable Property Status

Post by wmayer »

realthunder wrote: Sun May 17, 2020 1:47 pm I was explaining why I choose not to include property with 'Hidden' status bit. Because the property view will populate the linked object's property if a link is selected, as shown in the screencast below with green font (which is broken in the upstream, and fixed in one of my pending PR). Unless, the link itself contain a property of the same name, but not hidden. So depending on the 'Hidden' status bit of Link's property, the property view may either show the link's property, or the linked object's property. The difficult part is that they both have the same name. The property population is done in PropertyView::onTimer(). It is possible to include both properties, but requires more complex data structure in an already not so simple algorithm. And the PR I submit can't really deal the complexity introduced by linked property. As shown in the screencast, a manual refresh is required to show back the green linked property.
I tried this example on my locally built version where I have restored the old behaviour of the property editor. When entering the two Python code lines to add the property and hide it afterwards it shows the same behaviour as in your animation.

The only difference is that the item is not displayed in green but black color (which you say has changed in one of the PRs). So, is this the difference why a model reset should be performed to show the item in the appropriate color? At least I still don't see why adding, removing or changing the visibility status of a property would justify a model reset.
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: Dynamically Hideable Property Status

Post by realthunder »

wmayer wrote: Wed May 20, 2020 5:33 pm I tried this example on my locally built version where I have restored the old behaviour of the property editor. When entering the two Python code lines to add the property and hide it afterwards it shows the same behaviour as in your animation.
What the screencast shows is not the desirable behavior, but the problem. After adding the black property and then hide it, the black one disappeared, no problem, but the original green property should appear, and the screencast shows that you need a full reset to get it back. I haven't seen your actual fix, so can't be sure, but unless you've changed the internal data structure, the green property will not be in the model. Even if you've change it, and there are two property items with the same name hidden in the tree view, you'll still have to go through the PropertyView::onTimer() logic again to be sure which one should be unhidden.

Maybe you misunderstand my intention here, I am not against your fix, it does improve things when the Link is not involved, and it does not break existing behavior of the linked property. What I mean is that a proper fix that covers all situations is not that simple.
Try Assembly3 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
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Dynamically Hideable Property Status

Post by abdullah »

realthunder wrote: Wed May 20, 2020 11:08 pm
wmayer wrote: Wed May 20, 2020 5:33 pm I tried this example on my locally built version where I have restored the old behaviour of the property editor. When entering the two Python code lines to add the property and hide it afterwards it shows the same behaviour as in your animation.
What the screencast shows is not the desirable behavior, but the problem. After adding the black property and then hide it, the black one disappeared, no problem, but the original green property should appear, and the screencast shows that you need a full reset to get it back. I haven't seen your actual fix, so can't be sure, but unless you've changed the internal data structure, the green property will not be in the model. Even if you've change it, and there are two property items with the same name hidden in the tree view, you'll still have to go through the PropertyView::onTimer() logic again to be sure which one should be unhidden.

Maybe you misunderstand my intention here, I am not against your fix, it does improve things when the Link is not involved, and it does not break existing behavior of the linked property. What I mean is that a proper fix that covers all situations is not that simple.
I think we all have good intentions. I still think we are not understanding each other well.

Definitely, what (I understand from what) you now explain was not obvious from your original message. Let me see if I got it right now:

1. You do not add hidden properties to the model because you want to avoid property duplicates (and this is the only reason).

2. Your intended behaviour is that "green" properties (from the linked object) should appear in the Link property editor when the link is selected together with the link properties in "black". Here you give preference to the "black properties" (Link) when they have the same name. This means that you do not intend to show two "Length" properties one in green and one in black (please correct me if I am wrong).

3. To implement the intended behaviour of 2, you indicate that you need to rebuild the model (calling onTimer() which effectively rebuilds the model).

If you add all the hidden properties that Werner says (PropHidden vs Hidden), and you handle duplicates, it should be possible to hide and unhide without rebuilding the model. Here it should be possible to implement your priority system, so that it hides and unhides as needed without rebuilding the model (by providing the instructions to the model to hide what needs to be hidden and show what needs to be shown).

Now, looking at all this from outside (and I am used to be wrong, so take this with a big grain of salt), it appears to me that if we want to show the properties of one object together with the properties of another object, we do need to support property duplicates. Then I do not see why we need a priority system, it should be possible to show two properties "Length", one from each object, which may have different meanings and may need to be shown simultaneously.

I think the important bit in all this is though to reach a common understanding on whether we need to rebuild the model or not.
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: Dynamically Hideable Property Status

Post by realthunder »

wmayer wrote: Wed May 20, 2020 5:33 pm I tried this example on my locally built version where I have restored the old behaviour of the property editor. When entering the two Python code lines to add the property and hide it afterwards it shows the same behaviour as in your animation.
PR submitted here. Most of the changes happen in PropertyModel:buildUp(), which adds support of incremental update. PropertyEditor now uses persistent editor instead of normal editor in order to track the incremental update. The timer is now used for every type of changes, selection change, property change, editor mode change, add/remove dynamic properties, etc. Some critical changes, like deleting object/property is applied immediately followed by a full update with the timer.

The group items are promoted to be the only root items, which is heavily relied upon when doing incremental update. All the other property items are added as children of some group item. This allows the group to be collapsed/expanded, either with double click or the 'Left/Right' key.

The attachment contains a testing script. Put it in macro directory, and simply import the script for testing. The property 'Mode' holds five test case,
'Normal': unhide the 'Length' property
'Hide': hide the 'Length' property
'Add property': add the 'Length' property
'Remove property': remove the 'Length' property
'Kill': remove the 'Mode' property itself.

Property 'ModeNum' is an integer property with allowable value 0~4 corresponding to the 5 test cases. It can be used to bring back the 'Mode' after it kills itself. The 'Link' property can be used to test behavior of linked property. Point it to the 'box' object for testing.

Like I said, this is not simply fix, so more testing is required.

editor-test.gif
editor-test.gif (453.22 KiB) Viewed 917 times
abdullah wrote: Sun May 24, 2020 6:30 am If you add all the hidden properties that Werner says (PropHidden vs Hidden), and you handle duplicates, it should be possible to hide and unhide without rebuilding the model. Here it should be possible to implement your priority system, so that it hides and unhides as needed without rebuilding the model (by providing the instructions to the model to hide what needs to be hidden and show what needs to be shown).
The logic to determine which one to hide/unhide is not exactly simple. On the other hand, the same logic can handle the case of adding/removing property. Remember you mentioned interests of using dynamic properties? My PR above does not touch that logic, but simply re-apply it for all kinds of changes.

Now, looking at all this from outside (and I am used to be wrong, so take this with a big grain of salt), it appears to me that if we want to show the properties of one object together with the properties of another object, we do need to support property duplicates. Then I do not see why we need a priority system, it should be possible to show two properties "Length", one from each object, which may have different meanings and may need to be shown simultaneously.
The property is hidden for a reason, imaging the user sees two 'Placement' property of a Link. It will be confusing. That being said, I am not ruling out the possible use case of showing two properties with the same name, especially if the properties are in different groups. In fact, you can already do it, in some sense at least. When you use the property view 'Add property' menu action to add a dynamic property. The popup dialog has an option 'Prefix group name', which will insert the group name before the name you entered as the real property name. This is a simple way of achieving something like a namespace, to avoid name clash. So if you enter 'Length' and group name 'Box', the real property name will be Box_Length. The property view will auto hide the prefix 'Box_', so you'll see two 'Length' in the property view.
Attachments
editor.py
(2.88 KiB) Downloaded 23 times
Try Assembly3 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
Post Reply