This Engineering Notebook post discusses issues with the nested_splitter (NS) plugin uncovered two days ago while working on #3892: Add VR commands to cover/uncover the body pane. I've been considering what to do ever since.
tl;dr: See the Summary.
Background
PR #3893 is a thorough revision of the VR plugin. This PR leaves the VR3 plugin unchanged.
Late-stage testing revealed that Leo's caches were not restoring the ratio between the body and VR panes. A lengthy investigation showed that the caching code was not to blame. Instead, the culprit is the LeoQtFrame.ratio property, aka c.frame.ratio.
The PR adds the VR pane in a way that breaks the c.frame.ratio property. After several days of noodling, I suspect this property is not well-defined!
Remedies
First, I'll replace the ratio properties with explicit calls to two new methods, say c.frame.compute_ratio and c.frame.compute_secondary_ratio. There is no need to obscure where the computations happen!
Second, I'll attempt to fix the computations. This work will likely involve changes to the nested_splitter plugin. For example, the primary and secondary splitters should have well-defined names. Computing the ratios should depend on the names of various splitters, not their containment relationships. Why? Because the NS plugin can change those relationships!
What I won't do
I don't want to change the PR to accommodate the NS plugin. That would have the tail wag the dog. Instead, c.frame.compute_ratio and c.frame.compute_secondary_ratio should produce correct results in all situations.
Summary
Leo's Qt gui defines two (hard-to-find!!) and buggy properties: c.frame.ratio and c.frame.secondary_ratio. I suspect these properties are not well-defined!
For clarity, Leo's Qt code will use two new methods instead: c.frame.compute_ratio and c.frame.compute_secondary_ratio. These methods may use enhancements to the NestedSplitter class. All aspects of this work are experimental.
The existing properties must remain for compatibility. However, each property will print a deprecation warning the first time scripts call them.
All your questions and comments are welcome.
Edward
Going by last night's devel rev, VR was close to working as expected when I used the toggle command, at least with the particular layout I was using when I tried it.
The branch for this PR, though, was a mess so I'm looking forward to your new approach.
On Friday, May 10, 2024 at 8:47:46 AM UTC-5 Thomas wrote:Going by last night's devel rev, VR was close to working as expected when I used the toggle command, at least with the particular layout I was using when I tried it.That's good to know.
The branch for this PR, though, was a mess so I'm looking forward to your new approach.I'm not sure what you mean by a mess. PR #3906 cleans up a lot of cruft.
Edward