I noticed another weird handling related to _GroupTouched. Consider a Group inside a BuildingPart:
- With the inner group selected, press space, which calls ViewProvider::show() on the inner group, which sets Visible=False
- This triggers the GroupExtension on the outer BuildingPart, which touches its _GroupTouched property
- This triggers Gui::ViewProvider::update (via Gui::Document::slotChangedObject), which does something peculiar: it *hides* the updated object, then calls extensionUpdateData on all its extensions, and then *shows* the object (the outer BuildingPart) again.
- This then causes ViewProviderGroupExtension::extensionShow, as it should, to set the Visibility of all its invisible children to true
- Then, because the inner group becomes visible again the above sequence actually *repeats* once more, but because ViewProviderGroupExtension::extensionShow has a recursion guard, this does not become an infinite recursion.
- The end result, is that the inner group effectively stays visible.
The above sequence was discovered using some gdb breakpoints, so I'm reasonably sure it is accurate.
Interestingly enough I could only reproduce this behavior for a newly created buildingpart/group in an isolated testfile, after saving and restoring that file, I ended up with a BuildingPart that was marked as invisible (grayed out) even though it had Visibility=True, and gdb showed that toggling visibility actually tried to *show* it (isShow() apparently returned false, because pcModeSwitch->whichChild was -1, but because viewOverrideMode was 0x48ffbcb6 (seems corrupt AFAICT, should be a low number), setModeSwitch would refuse to change whichChild and the BuildingPart can no longer be made visible... Maybe a different bug, maybe related, dunno.
Looking at the above, I wonder (again) why this _GroupTouched stuff is really needed. But also, why does ViewProvider::update actuall do this trick of hiding, applying an update and then showing? The comment claims to speed up updates, but at first glance, this seems like a rather fragile and also ineffective way: Instead of doing a potentially slow redraw (I guess that's what's being prevented?), it now ends up triggering a whole bunch of stuff because of the hide and show. And if a bunch of properties are updated in a single transaction, you'd end up doing this a bunch of times (and I guess everytime you call show, a redraw is triggered anyway?). I'm not too familiar with the code, but if the problem is slow redraws during updates, wouldn't it be better to globally implemented delayed redraws anyway (i.e. mark things as dirty while updating data, then afterwards redraw all dirty objects). Maybe there is something else going on, though.
Looking further into the code, I've noticed that ViewProviderDocumentObject::updateView also
does the same trick, but it sets the User1 bit on the Visibility property, to prevent further syncing of the visibility property. Additionally, ViewProviderDocumentObject::update
also sets this bit, so it is in effect when ViewProviderGroupExtension::extensionShow is called. And looking at ViewProviderGroupExtension::extensionHide, that actually checks for this bit and does not propagate visibility when it is set, so maybe this is how it is intended to work (though again, this looks fragile at first glance: What if some property update actually triggers a genuine visibility update, maybe in some user Python code, which would now, I think, not be propagated to children because it is indistinguishable from the temporary changes).
The User1 check in ViewProviderGroupExtension::extensionHide was introduced in
this commit by Realthunder. At first glance, it looks like an oversight that extensionHide was modified but extensionShow was not, and an obvious and quick fix would be to add the same check to extensionShow as well (haven't tried it yet, though).
Looking at Realthunder's PR 2759, I see that that also makes extensive changes to this area, but from a first glance there, the code that toggles child visibilities was moved and refactored in
this commit, which actually drops the User1 check. There is a new _checkParentGroup guard that prevents visibility propagation, but from the looks of it, that is used for a different purpose. And looking at the other code, the _GroupTouched and ViewProvider::update behavior that causes this problem is still there. The PR does modify the visibility handling to save the old visibility state of children, so maybe that could mask this problem (it would look fixed, but do a lot of unneeded churning on visibilities in the background).
matthijskooijman wrote: ↑Wed May 19, 2021 4:45 pm
Also, I wonder why bother with this _GroupTouched property at all? Why not just touch the group object itself directly, rather than this dummy property?
I think I realized the reason for this: When touching _GroupTouched which is marked as Output, then the group itself is *not* touched, but anyone listening for changes in the group is notified, so this can e.g. touch other objects that reference the group when needed (without those objects having to listen to, or even be aware of, group members directly). I guess using a dummy property in this way is still a bit confusing, but it's probably a pragmatic way to handle this. I do wonder if there would be a cleaner and/or more semantic way to handle this, maybe a nicer name for the property could already help.
