Once you choose a sub-item from one parent, the next time you choose a subitem from the same parent the event doesn’t fire. That is if you choose Sub1 / SubSub1.1 / Item 1.1.1 then you can’t choose anything below Sub1 again until you choose something from Sub2.
here’s a graphical display which shows it.
This basically breaks patch selection in surge.
The commit is changing how you look up whether something is a child window. I think what’s happening is that it is getting confused as to whether you are a child or not the second time - something is cached in the component peer - but I couldn’t figure it out.
But this one is a real show stopper for us. Would love to know what to do but i might have to walk surge back to JUCE 7 to get our nightlies usable for windows users again if the fix is hard.
If you want, I’ve made changes to surge now so you can build it with any 8-era juce (after the 8.0.1/8.0.2 font API changes). So you can just checkout surge, update the submodule to where you want (by default it will be 802 with our changes but I tested with vanilla 802 801 and 800 and bisected on the main repo tags).
If I make this change to 8.0.2 in juce_Windowing_windows.cpp
bool contains (Point<int> localPos, bool trueIfInAChildWindow) const override
{
auto r = convertPhysicalScreenRectangleToLogical (D2DUtilities::toRectangle (getWindowScreenRect (hwnd)), hwnd);
if (! r.withZeroOrigin().contains (localPos))
return false;
const auto screenPos = convertLogicalScreenPointToPhysical (localPos + getScreenPosition(), hwnd);
//if (trueIfInAChildWindow)
// return getClientRectInScreen().contains (screenPos);
auto w = WindowFromPoint (D2DUtilities::toPOINT (screenPos));
return w == hwnd || (trueIfInAChildWindow && (IsChild (hwnd, w) != 0));
}
which is the equivalent of reverting 51539b9, surge works again as expected. It’s easy for me to apply that to our fork to get back in the water. Is it safe?
Ugh. @dave96 suggested I try same with the menu demo in the widgets demo so I modified it to do
for (int z = 0; z < 5; ++z)
subSubMenu.addItem ("Sub-sub-item " + String (z), [&]() {
auto s = std::string("rand name " ) + std::to_string(rand());
nestedMenusButton.setButtonText(s);
});
and that worked. Namely it didn’t exhibit the same bug.
THis makes me think that part of the problem could be that surge uses a multi-column parent menu which is under the menu item. Although I haven’t confirmed that yet. But is maybe a hint.
OK in good news I can’t make the widgets demo show the same break.
I’m using custom components in menus also. I wonder if thats part of the cause somehow? I don’t have a small repro alas but i fyou need one i can hack on it.
I wonder whether it’s something specifically to do with overlapping menu windows, i.e. the top-level menu is underneath the nested menu when the nested menu is clicked. Did you test this scenario with the widgets demo?
Yeah I have a hacked version of the widgets demo which adds custom components, turns on accessibility, adds columns, and adds ticks, and it works fine.
OK no answer, but maybe helpful if you manage to look
if (trueIfInAChildWindow)
{
auto w = WindowFromPoint(D2DUtilities::toPOINT(screenPos));
auto oldVal = w == hwnd || (trueIfInAChildWindow && (IsChild (hwnd, w) != 0));
auto cid = getClientRectInScreen();
auto newVal = cid.contains(screenPos);
if (oldVal != newVal)
{
std::cout << "----------------" << std::endl;
std::cout << "OLD VAL is " << oldVal << " NEW VAL is " << newVal << std::endl;
std::cout << "Local Pos is " << localPos.x << " " << localPos.y << std::endl;
std::cout << "Screen Pos is " << screenPos.x << " " << screenPos.y << std::endl;
std::cout << "hwnd is " << std::hex << hwnd << std::dec << std::endl;
std::cout << "w is " << std::hex << w << std::dec << std::endl;
std::cout << "cir is " << cid.toString() << " cont=" << cid.contains(screenPos)
<< std::endl;
std::cout << "IsChild " << IsChild(hwnd, w) << std::endl;
std::cout << "IsChild other way " << IsChild(w, hwnd) << std::endl;
auto hris = getClientRectInScreenFor(hwnd);
auto wris = getClientRectInScreenFor(w);
std::cout << "HWND Rect is " << hris.toString() << std::endl;
std::cout << "W Rect is " << wris.toString() << std::endl;
}
}
I added that to the contains method in component above the ifTrueIn … return
it shows when the new method would give a different answer than the old method. Some observations
with the surge triple nested menu just a mouse overing is calling contains continuously once i open menu 3.
the window-at-point and hwnd in question have no child relationship. That makes sense
but the hwnd does contain the point
So that leads to something like ‘in this very wide menu you are picking the wrong item for containment check’. Somehow the menu code has a z-order problem or something? but as a result in this case contains will be true with the new version and false with the old, which is whats making the wrong thing happen.
It looks like the PopupMenu window uses a timer to detect clicks on menu items, but each window gets its own timer. In Surge I think we’re getting unlucky, and the timer for the root popup ends up firing before the timer for the leaf popup, and then the contains function for the top level window reports that the mouse is inside that window, despite it actually being occluded by the leaf popup window.
I wish I’d added a bit more detail to the WM_NCHITTEST commit message. I remember that WindowFromPoint may end up sending WM_NCHITTEST to our window, which might cause problems if WM_NCHITTEST itself then tries to call ComponentPeer::contains to see whether the point is inside the peer. That includes calls to Component::findControlAtPoint.
I think the best course of action is probably to revert the WM_NCHITTEST change, so ComponentPeer::contains always calls through to WindowFromPoint, and then to add some docs to Component::findControlAtPoint explaining that it’s dangerous to call Component::contains from this function. I suspect the JUCE-vendored DocumentWindow class is the only class that will be affected by this change.
Oooh where is the timer code? I wonder if there’s a contains but occluded method we could write and call that rather than revert the change whole cloth
Hey and thanks for all your help last few days. It’s been a bit rocky but the rendering and fonts are really a lot better with 8. And I appreciate the co debugging
It’s MouseSourceState in juce_PopupMenu.cpp. I misunderstood it a bit; although there is a timer, it acts as an async one-shot on each mouse event.
I’m not sure - I don’t think that’s possible without calling WindowFromPoint, which is the main thing I was trying to avoid.
I think reverting it is the right thing to do. Better to leave the old code more or less as it was, and to restrict the changes to the newer code with fewer users.
Sure, no worries. Thanks for all the bug reports! There were a lot of big changes in J8, but it feels like we’re close to discovering and fixing all of the regressions now.
