Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New function IsBlockedByModal and new flag ImGuiInputFlags_RouteUnlessModal #7646

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rodrigorc
Copy link
Contributor

This is my draft for implementing #7643.

I'm aware that modal windows are not a priority, because they are seldom useful in most ImGui use cases. But I happen to need them, so I'm trying to push them a little bit.

I'm not sure that is the proper way to implement it, that routing thing is still a bit of a mystery to me, but it seems to work just fine.

I'll be happy to receive any correction or feedback.

@ocornut
Copy link
Owner

ocornut commented Jun 3, 2024

I'll leave this open until I have can investigate everything properly, I agree #7643 is an issue. But it's almost certainly a little more complex than that, given one of the property of CalcRoutingScore() (not exercised by SetShortcutRouting() yet), is that we could want to query routing for a location that this not the current location. The use of g.CurrentFocusScopeId in SetShortcutRouting() to compute focus_scope_id is expected to get turned into a parameter somehow.

Moreover, calling FindBlockingModal() for every shortcut is expected to be too slow. This is why we flatten things into g.NavFocusRoute[]: the only non-O(1) processing is iterating over a dedicated dense array instead of peeking into miscellaneous memory.

I'm aware that modal windows are not a priority

Not sure where you got that from :) I admit they brought more problems that I would have expected, but we'll keep fixing things as required.

that routing thing is still a bit of a mystery to me, but it seems to work just fine.

I realize the codebase is not simple to work with. As a general rule of thumb:

  • if you are not sure what something is for, a nice trick if to disable/comment/modify it and run the imgui_test_suite: the failing tests are generally giving a good indications of what it could be for.
  • use debug log and metrics windows to inspect things.

@rodrigorc
Copy link
Contributor Author

I'm aware that modal windows are not a priority

Not sure where you got that from :) I admit they brought more problems that I would have expected, but we'll keep fixing things as required.

Ah, now that you mention it...

For first, the workaround you mention of calling FindBlockingModal(NULL) != NULL is an internal API, I should think twice before using it. BTW, wouldn't better be FindBlockingModal(GImGui->CurrentWindow) != NULL as I wrote in my new IsBlockedByModal()? IIUIC, that will return true if the current element is below the modal and false if it is above, as it should be by default.

For second, there is this one little, insignificant issue... if I open a modal, and then over it I open a pop-up (such as the "Combo" in the "Stacked modals" demo, and then I click outside of the modal window, in the void dimmed space, I would expect the popup to dismiss, but it does nothing! I can even resize the "Stacked 1" window to make it small, and then open the "Color picker" popup, move it to cover the whole parent window, and I will be unable to dismiss it (I can move the "color picker" but that's not the point):

image

I don't use popups over modals, so this doesn't affect me and I didn't bother to report it before... But these kinds of small paper cut issues, that plague other UI libraries, are rare in Dear ImGui ( ❤️ ), so the fact that this has survived for so long should mean something.

@ocornut
Copy link
Owner

ocornut commented Jun 3, 2024

Please open a separate issue.

@rodrigorc
Copy link
Contributor Author

Created #7654 for the that unrelated modal issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants