This is a WIP branch which tries to make popup menus "smart" and avoid getting clipped.
It would benefit from more testing and review, but maybe in a few weeks it might make sense to merge it.
If the popup would be off the screen or clipped by the parent, it slides over in order to be on the
screen. If the menubar is near the bottom of the screen, the popupmenu flips up above the menubar instead, and similarly if a popupmenu is the child of a popupmenu which is near to the right of the screen.
It is working alright in my application right now, but it needs more testing and possibly review.
There's also an issue of how the "automatic" logic introduced here should be reconciled with
the manual control offered by "d_popupOffset" which users might already have been using
to solve problems like this.
Previous logic was
- If parent is a menubar, then position just below the menu item
- If parent is also a popupmenu, then position just to the right
of the menu item
- If the user wants custom placement, they can use popupOffset,
which gets added to the position always
In this commit, we do the following:
If the parent is a menubar, then there are two candidate positions:
just below the menu item (primary), and just above the menu item (secondary)
Check the clipping rectangle of our parent, or if no parent, the root container.
Compute for both positions the minimum shift we need to do from the candidate spot
in order to not be clipped.
If primary requires vertical correction (shift up into menubar) and secondary does
not (so the menubar is near bottom of clipping region) then we use the (corrected)
secondary position, otherwise we use the corrected primary position.
If the parent is a popupmenu, then we do the same thing, but the primary position
is just to the left, and the secondary is just to the right, and we are concerned
about horizontal correction.
We always add "popupOffset" to the result, after these computations have happened.
However this might be bad -- for someone using popupOffset to do fine control,
they might not be happy with the new behavior.
It might be better to do one of the following
Add popupOffset to both the candidate positions first, before doing the clipping
checks. However, this is hard
because IIRC the popupOffset is in unified dimensions, and the candidate positions
are just absolute floating point values. But if we don't do this we might get
bad behavior when both features come into effect.
Disable the clipping checks if popupOffset is nonzero -- the argument being,
if the user set a custom offset then they know what they are doing.
Add a boolean flag to disable "smart" placement, but make the smart placement
on by default. Then we can be backwards compatible with people who wanted to
have fine-control over the positioning using popupOffset, they can just set
the smart placement to off.
The PR in current form does make a difference to v0-8 applications.
If you wanted to be fully compatible with existing v0-8 behavior, then I guess you would want to add a property to enable the smart clipping, but then we need to store it somewhere and can't without breaking ABI.
So yeah, it might make more sense to do it in default instead. In the application where I'm testing this though, it is against v0-8.
Ok, if you want to close this one, I can make one against default later.
Let me know if you guys want to remove the popup offset in default branch.
We could also make a popup positioning mode enum or something, I guess, so that people who want manual control of offsets from c++ can do that.
Idk, maybe not necessary.
A popup/context menu sounds like something that should be implemented as a combination of widgets and not as a separate widget type if you ask me. That said, I might be overlooking some special feature of context menus here.
After taking a second look, I think we should not drop this d_popupOffset member in default branch. From documentation it is not 100% clear what purpose in a use-case it has butthere is no property related to it ("PopupOffset") and it all seems like it wasnt implemented for shits and giggles, so somebody out there might be depending on this. Would it make it easier if this was in absolute positions instead? I think it would be better to change that than to remove it entirely. There is of course also the possibility to just convert the uvector into absolute values and use that, has that been considered? Is there any other issue than the type of it?
I guess I wasn't sure what is the 100% correct way to convert the uvector to absolute coordinates. The other thing is, if they want fine manual control, maybe they don't want the "smart" behavior interfering? For instance I don't think my patch will necessarily do what the user wants if the popupmenu is inside of a scrollable pane. I'm still sort of pondering if it's better just to always use the root container size as the clipping boundary. If they are doing manual control then presumably they want it exactly at that offset.
I mean if compatibility is the concern, I think adding a "smart mode" might be a good idea. I think most people use popupmenu and menu item because it is a batteries-included popup menu -- if you need special behavior then probably I think you are right that it's easier to use a combination of widgets. So I'd advocate making the "smart mode" the default mode, in default branch, and we could have a "compat" mode which is unchanged.
No, it's not a big hassle to keep. But I'm not sure exactly how to keep it.
The way my patch works, basically there are two "candidate" offsets and it checks if one of them is a better fit. Should I add the user offset to both of my offsets? Should the user offset be a third candidate offset, preferred to the other two? Should I always defer to the user if they specify an offset? The thing is that the user may use a (0, 0) offset to mean they specifically want the menu item at the (0, 0) place and they don't want me to move it. I don't have a good way to know if the user actually specified an offset.
@Christopher Beck: Come on, that can b trivially solved. We can just decide that an offset of (29384.8250929, -57834.83658298374) signifies no manual offset. What's the chance any1 would use that value exactly?
I think also manual offset is for advanced users anyway. Such users should b advanced enough to edit cegui's code or inherit from cegui's classes. I think we can just compromise on having a virtual method decide where the popup will popup (if that's not already the case), using only our logic (without d_popupOffset). Advanced users, wishing to override that behaviour, r welcome to inherit the appropriate class and override that method. Any objections? Good, case closed :)
@Christopher Beck: Do plz try to organize the code in a way that a virtual method decides where the popup will popup (if that's not already the case), so that cegui users can override our logic by inheritance.
@Christopher Beck The general consesus seems to be that we do not want a manual offset, as the use-case is unclear, and that an advanced user will be able to add it by themselves. So let's not have it in default anymore.