Popup menu positioning (was PR #231)

#233 Merged at bbfa674
Repository
cbeck88
Branch
popup_menu_positioning
Repository
cegui
Branch
default
Author
  1. Christopher Beck
Reviewers
Description

This adds "smart" popup placement which avoids getting the menus clipped by the screen, and makes that the default behavior.

The manual "popup offset" member variable is removed in favor of a virtual member function which computes the appropriate offset on demand. (This was the consensus solution in discussion following PR #231)

Comments (36)

  1. Christopher Beck author

    Cool.

    There's a few changes from the original PR:

    1: I moved the "set position" code in MenuItem::openPopupMenu() to after the if (notify && p) block. I did this because on code review, that does always trigger a later call to MenuItem::openPopupMenu(false), it seems cleaner to do parent notification completely before we start doing anything with the popup menu, and then set its position and open it at the same time. I don't think this is a behavior change, just reorganization.

    2: I changed the way I determine the popup menu bounding box. In original PR I was using this: p->getParent() ? p->getParent()->getClipRect(false) : Rectf(Vector2f(0, 0), getRootContainerSize()) but in testing I concluded that doesn't always give good behavior. It's a bit fiddly unfortunately... I decided to just use the root container dimensions for now, since that's simple and gives good results in my programs. I put this bit of code in a virtual function to make it easier to tweak it if desired.

      1. Christopher Beck author

        So basically, the reason is that the parent might not be clipping the popup window.

        It sometimes happens that a parent doesn't even clip it's own children. For instance, the parent might be a scrollable pane, and we disable cached drawing to permit things like drop down menus inside the content pane to open up and expand outside of the pane.

        Beyond that though, the parent of the MenuItem is NOT typically the owner of the popup menu, so there's no reason it should clip it at all. For instance, suppose the menu item is a child of a menubar. The menubar is typically very wide at the top of the screen. The popup menu which appears when you click the menu item appears below the menubar, outside of its clipping rectangle. So, it's wrong to attempt to place the popup menu within the clip rectangle of that menubar (the parent of the menu item.)

        I have to admit, I'm not totally sure of the details. For instance, what is the parent of the popup menu? I think it's just the root container typically, but I'm not sure. Does it even have a parent before it is opened? It might make the most sense to ask the popup menu what its parent is and use the clip rectangle of that. The thing is, it's very fiddly. What happens if the parent exists only if the popupmenu is currently open? Does that mean that when a popup menu is opened once, it appears at one place, and when you open it while it's already in the open state, it appears at a different place? That sounds annoying. Also, there are always going to be corner cases esp. related to scrollable pane and windows that are supposed to render outside their parent I think. So, at some point I just say, let's do the simplest thing and put it in a virtual function, so that in more complicated cases someone can swap out this behavior. The root container thing worked well in all the scenarios I tested. And, I think it does no harm basically -- it only changes the behavior from the past in cases when there definitely would have been clipping.

      2. Yaron Cohen-Tal

        The root container size isn't the size of the root window, it's the display size, isn't it? Either case, it doesn't seem to make sense for a menubar. Wouldn't it b better, in the case of a menubar, to take the parent of the menubar? I think that would work best in almost all cases of a menubar, and if any1 makes a menubar a child of a scrollable pane.. well, then I'm sorry, I think he has then set his own path to hell.

        1. Christopher Beck author

          I guess I haven't tested "size of root window" vs. "size of display", that might indeed be better. You have to understand, you are rapidly reaching the limits of my knowledge of CEGUI internals, but using root window dimensions is probably worth testing, I agree.

          and if any1 makes a menubar a child of a scrollable pane.. well, then I'm sorry, I think he has then set his own path to hell.

          Far be it from me to pass judgement on your religious beliefs :p

      3. Yaron Cohen-Tal

        In the case the parent of the menu item is a popupmenu, I think we should clip by the root window, not the root container. Yes, the root contianer seems to make more sense, however we can't render out of the root window anyway so everything has to b clipped by the root window.

      4. Yaron Cohen-Tal

        Ah, forget that last msg, I got it wrong: I mistakenly thought GetRootContainerSize returns the display size, which may b what 1 can understand from its doc.

        So in the case the parent of the menu item is a popupmenu using GetRootContainerSize does indeed make sense to me, however in the case of a menubar I still think clipping by the menubar's parent is the better choice.

        1. Christopher Beck author

          So, let me try to describe a scenario in my application, and what I think would happen with the "clipping by menubar"'s parent.

          It often happens that you have a menubar, which contains many menu items with attached popup menus, and then those also contain menu items with attached popup menus, and so on. You can set this up easily in a drag-and-drop fashion using CEED.

          Also, one fairly common way to use menubar is if you have a bunch of frame windows floating around. For instance you might have some kind of chat integration kind of like steam has, so that if someone sends you a chat message, a chat window can appear, and you can drag it around whereever is convenient. In this case, the parent of the menu bar is likely to be the framewindow, but it might be some kind of "section" or "panel" widget for spacing or something.

          1. If there is a section or panel widget, it might be of size similar to the menubar, so it might not make much sense to try to jam popups into there.
          2. Even if there isn't, this may lead to odd behavior if the popup is a little larger than the framewindow.

          One of the things that this patch changes is, if you open a popup but it would be off the screen, it will try to open in the other direction. So if you put a menubar near the bottom of the screen (similar to windows XP start menu), the windows will realize they are clipped if they go down but not if they go up. If you put a chat window near the bottom of the screen, or the far right, and click one of the menus, it will open up / to the left rather than go off the screen. If we change the code so that the clip rectangle is in fact the chat window rather than the whole screen, it may subvert this behavior. If the popup is taller than the chat window, because the user resized it, or the popup contents are dynamically generated and sometimes large, etc., then the logic we have is "only flip up if we would be clipped when going down but not when going up" basically. If we would always be clipped when opening in the up direction, then we will never open in the up direction. So, in a specific case like, the chat window is not as tall as the popup, and the chat window is situated near the bottom of the screen, so that the popup would be clipped by the screen if it is opened, my code would do the "right" thing and open up, to avoid being clipped, while clipping to the parent of menu bar won't do that, because it won't agree that up is better than down (up goes out of bounds of the frame window).

          Also, I think it's potentially a little strange that the level 1 menus would behave differently from the level 2 menus. (But I guess they are already different in the sense that one of them goes vertical and one goes horizontal, so maybe it won't seem wierd to the user.)

          That said, I haven't actually tested your proposal, it might be that it usually works fine.

          I think there's not really a "right" way to do this -- if you want it to use this strategy in the default CEGUI configuration, I don't see a problem with it. I might inclined to override it in my application though, like I said, the way I proposed makes more sense to me, at least I can see why this way might do things I wouldn't like, and I'm already pretty happy with the way I showed.

          If you want to show me exactly what code you have in mind I can try to test it in my app. I don't remember if there is a good CEGUI samples for purposes of testing the popup menu behavior.

        2. Christopher Beck author

          Ah, forget that last msg, I got it wrong: I mistakenly thought GetRootContainerSize returns the display size, which may b what 1 can understand from its doc.

          So, when I look in the source here: https://bitbucket.org/cegui/cegui/src/e1c35f3b374c2cef6f2a96e81dd257b0d7354456/cegui/src/Window.cpp?at=v0-8&fileviewer=file-view-default

          it appears that getRootContainerSize returns

          getGUIContext().getSurfaceSize();

          so presumably it is basically the size of the window the OS gave us (?)

          It appears to be the same in v0-8 and default.

          If I wanted to use the size of root window instead, I assume it would be something like Window::getRootWindow().getClipRect() ?

          Afaik, these things will be different, but in most cases not by much.

          1. Lukas Meindl

            surfaceSize is the size of the renderTarget, which can be the size of the screen (if fullscreen) or of the host window or of a rendertarget. It has nothing to do with the OS actually.

            Remember that the root can be bigger than the window or fullscreen size, therefore your menu if clicked could be partially or fully in a non-visible area. Is this desired? I would expect it to be in the visible area, which is the surfaceSize (from what we can expect), similar to how I would expect it from tooltips.

          2. Lukas Meindl

            Actually on second thought: I am not sure anymore because I dont fully understand how it will feel without having played around with it.

            Did I get it right that you want it to flip up/down depending on the clipping? Why can't we make it a user-defined property if it is up or down?

            1. Christopher Beck author

              Sure, we could make it user-defined, but, I think in many widget toolkits popup menu behavior is mostly automatic, as a "convention over configuration" thing.

              If someone wants to have a user-defined property, they could still derive from menu item, add a flag, and override the computePopupPosition function.

      5. Yaron Cohen-Tal

        @Christopher Beck: Again, u make some very good points.. Still, however, in the case that the menubar is at the bottom of its (say, frame window) parent, then if we use the root container size for the clip rect, the menu may show downwards which seems unnatural to me.

        So, I'm thinking about a middle solution: Use the menubar's parent to determine whether the menu should pop up or down, but use the root container to actually fix the location of the menu. So, instead of 1 virtual method, we'd have 2, and what I've described above would b their default implementations.

        1. Christopher Beck author

          Yeah, so I think you are right and this should be re-engineered a bit.

          What I propose is that we add a property to CEGUI::Menubar, indicating whether the menus should open up or down. And the default state should be some intelligent guess based on the clipping like you describe. So it would be like a tristate property "up, down, auto" or something like this.

          I guess I'd want to implement that actual interpretation of this property again in the popup menu cpp file, b/c that is where the utility functions for clipping are.

          Let me know what you think, I will try to implement this sometime soon but not too soon.

        1. Christopher Beck author

          Hey, I made a little progress, in the sense that I have added a tristate option for up, down, and some automatic determination.

          I also tested this in my application, and it is working well. Also, I decided to use the explicit option in some cases where previously I was making it work automatically, since it might be more robust.

          I didn't implement the two-boxes strategy you described with both the menubar parent and the root container rectangles, I'm still thinking about that I guess. It might be better to get rid of automatic option altogether for menubars and just assume "down", at least it will be less work, and if someone wants to make an automatic one later they can add it to this enum / property without much struggle.

          Basically I'm not sure what's the most natural way to actually do this: "Use the menubar's parent to determine whether the menu should pop up or down, but use the root container to actually fix the location of the menu."

          I think what you are saying here makes a lot of sense, but I'm not sure if this is the best way to actually code it. I feel like it might be a good idea to look in some other UI toolkits like Swing or one of the Apple ones, or like ImGUI or Nuklear, and see what logic they use for this. If I'm not wrong then at least some of them are going to have some logic that tries to make the menus behave nicely depending on clipping and such. That would give me more confidence at least that I'm not doing things in an excessively complicated way.

      6. Yaron Cohen-Tal

        @Christopher Beck:

        It might be better to get rid of automatic option altogether for menubars and just assume "down", at least it will be less work, and if someone wants to make an automatic one later they can add it to this enum / property without much struggle.

        That sounds too like a reasonable solution. It seems like that's what Qt does?

      7. Yaron Cohen-Tal

        @Christopher Beck: Why do u add the menu bar direction property to MenuBase rather than MenuBar?

        Also, as for the automatic fit, I don't mind if in the final pull request there's no automatic fit at all. However, as I've explained, your current best fit algorithm is problematic for menu bars, so I'd prefer that u either implement the change that I suggested to that algorithm, or not provide a best fit algorithm at all (I'm refering only to popups whose parent is a menu bar. Automatic fitting for popup menus whose parent is a popup menu should still remain). I'm fine with either way.

        1. Christopher Beck author

          Hey, sorry for long absence, life intervened in a big way...

          I'm going to try to get this patch into a final state sometime in the next day or two, then I'll have intermittent internet access for about a week, and then no internet access for two months. There is not much work left and one way or another it's going to get where it needs to be I think :)

        1. Christopher Beck author

          Well, it's like this, I got back together with my girlfriend, and we have been on an extended camping trip in the deserts of Utah and then Nevada for about 3 weeks. We're going to be in California with friends for about a week, then hopefully camping and fishing in Baja Mexico for like a month or two. I don't expect to get internet anywhere in Baja, but I could be wrong, we're going to find out :)

        2. Christopher Beck author

          @Yaron Cohen-Tal Ok, I have removed MenuDirection::BestFit, the default is now Down, and Up is the other option. I tested with this patch in my other project, and it is all working as nearly as I can tell.

          The other question is, why MenuBase rather than MenuBar.

          So, the main reason it makes sense to me is that the code in MenuItem is based on taking the parent pointer and dynamic-casting it to discover the runtime type, whether the parent is specifically another popup menu or a menubar, in which case it has special behavior, and in other cases it has a generic behavior.

          Presumably the reason there is a menu-base and then a menubar is just to separate the class that implements the logic, from the actual widget class type, so that if someone wants to make a menubar that looks a bit different, they can do so fairly easily, the main "work" that is done in Menubar.cpp is in the layoutItemWidgets function. I guess you could assume that another menu type might have different geometry, so you envision "centered menubar" or "vertical menubar" or "radial menubar" like the menu in the game "Oblivion" or something like this?

          I guess you might very well make the argument that the enum and the property should be in menubar, and the dynamic cast in menuitem should be against menubar, not menubase.

          One difficulty is, menubase is what registers the actual properties though, not menubar. https://bitbucket.org/cegui/cegui/src/84c33a247ff8e62a2423d09618d1be3d664443af/cegui/src/widgets/MenuBase.cpp?at=v0-8&fileviewer=file-view-default#MenuBase.cpp-117

          So, I guess we could add a similar function to menubar, specifically for the new enum. However, I didn't really think through that yet, and this way was less of a change.

          In some ways it's easier to reason about what the code will do from menuitem perpsective if we only do test menubase.

          Idk, I felt like if someone wants to radically extend CEGUI menu capabilities they can easily change this stuff later, but it might make sense to organize it carefully in advance of such projects.

          Anyways let me know what you think, bbl.

  2. Yaron Cohen-Tal

    Is it possible U haven't yet added yourself to the credits?! This is outrageous. Plz immediately add yourself to doc/doxygen/authors.dox. Do it in a separate pull request that targets branch v0-8. Add yourself under "Main Contributors". List your main contributions (not only this pull request but all your contributions so far).

  3. Christopher Beck author

    I realized that, most of the variables in compute_popup_offset can be declared const, do you guys consider that kind of thing worthwhile? I guess I'm inclined to do it, if you are opposed let me know. I'll probably update the PR sometime tomorrow.

    1. Lukas Meindl

      It is cleaner and safer to make everything const that can be const - so feel free to do it if you want, it is really up to you though as we have no rule for it. I mostly don't do it myself.

  4. Yaron Cohen-Tal

    @Christopher Beck: I've manually merged your pull request.

    U analyze problems thoroughly from every aspect. u take the time to discuss and explain your views in length. And, even though it took quite some time, discussions and changes, u did eventually finish this project. From my little experience as reviewer I can already say that it's not obvious. It's fun to work like that, and I really appreciate it!

    I would like to remind u though that very long breaks with working on a pull request make it harder for us to review, coz each time we need to recall the issues (especially if they're delicate ones). So, unless some big change is required from u, I would like to ask u to shorten the breaks in such cases, as much as possible within your life circumstances. If u suspect ahead that u won't have time to deal with not big issues within a reasonable time, plz postpone the creation of the pull request. And always remember: cegui is above all, even if that means dumping your gf, resigning from work and abandoning your children.

    Thanx a lot for the contribution!