add clipping correction logic for popup menus of menu items (WIP)

#231 Declined
  1. Christopher Beck

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.

Thoughts and feedback welcome!

Comments (28)

    1. Lukas Meindl

      No idea. What is the supposed purpose of the offset? If it is only used as workaround for propert clipping correction logic, it definitely can be removed in default branch.

      @Christopher Beck Do the changes here btw. make any difference to any existing v0-8 applications? if so it might make more sense to add this to default branch instead.

      1. Christopher Beck author

        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.

          1. Christopher Beck author

            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.

              1. Christopher Beck author

                I can't think of one. The only thing I could think of was some kind of hack for making context menus, but I guess there are better ways to do that that don't involve menu item.

                It might be better to make some of this logic as a method of popup_menu instead of menu_item so that it can be easily recycled for context menus...

                1. Lukas Meindl

                  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?

                  1. Christopher Beck author

                    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.

                    I tried running "git blame" on menu item to see when this was introduced, I got this: so, sometime in 0.7. I'll dig a little more and see if I can find an informative commit message.

                      1. Lukas Meindl

                        If it is useful to some people and we already have it we should keep it! It is not a good idea to throw features away without good reason.

                        If you say this has absolutely no use and/or is not possible to keep without huge hassle, throw it away.

                        Is it really big hassle to keep? I can look into the conversion, is there anything problematic aside from it?

                        1. Christopher Beck author

                          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.

                          So, I think it's better to just have two modes.

    2. Yaron Cohen-Tal

      Btw I've been looking into Qt's QMenu and QMenuBar and couldn't find a clue to something that allows manual location control. I'm just not sure we should b over-flexible about it..

  1. Yaron Cohen-Tal

    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 :)