Mixedrealitytoolkit-unity: Refactor Input System's modal input stack.

[AdamMitchell-ms]

Issue

Various methods on InputManager such as PushModalInputHandler, PushFallbackInputHandler, and others require the callers to pop things in the exact reverse order from how they were pushed. In complicated apps with various modal dialogs, this seems like an unreasonable requirement. A modal dialog may decide to close itself for any number of reasons even if it is not on top.

Proposal

Instead of Push/Pop methods, there should only be Add methods which return a handle to the caller. The caller can then remove their instance by calling the appropriate function on their handle. The handle could be an IDisposable, so the caller can call Dispose when they are done. This also allows for a nice 'using' syntax when appropriate.

I have an implementation of this working on an old fork. If this sounds like a good idea to others, I could make the changes here and send out a PR.

Note that this is an API-breaking change, but IMO, it's a better API because it makes it harder for the caller to make a mistake.

[StephenHodgson]

Note that this is an API-breaking change, but IMO, it's a better API because it makes it harder for the caller to make a mistake.

Sounds like it belongs in vNEXT then.

The handle could be an IDisposable, so the caller can call Dispose when they are done. This also allows for a nice 'using' syntax when appropriate.

Not sure that's needed. We may just want to have the caller be able to pass that object back to be popped, once finished.

[davidkline-ms]

The handle could be an IDisposable, so the caller can call Dispose when they are done. This also allows for a nice 'using' syntax when appropriate.

I like this pattern. Good suggestion.

[StephenHodgson]

@maxouellet do you have any feedback about this proposal?

[maxouellet]

The main concern that I have with this proposal is that the order in which events would be handled in modal scenarios might become unclear. The Push/Pop methods make it very clear that this is a stack-like behavior, where the order of input handling respects LIFO stack model (last element added will receive input first).

If you change it to just "Add" and "Remove", it becomes unclear what will handle input first. In fact, I'd question whether this change really fixes the problem. It seems to me like the problem at hand is the following (which I have encountered in MS Layout):

• We have more than one piece of UI, and each of them needs to handle modal input because they might have some blocking interaction. I want the popup on UI "A" to block every interaction on menu "A", but not prevent interactions with menu (or component, or whatever it is) "B". Same logic with a popup on menu "B".

The current implementation of InputManager doesn't really support this scenario: it just assumes that modals are global and thus can block everything else. The above scenario thus becomes really messy to implement without doing some messy hack. I don't think changing the APIs to support arbitrary removal that doesn't respect the stack would address this problem, since the problem really is that you have more than one UI context that are independent from one another, and thus can't easily be handled byh a centralized InputManager that is unaware of input contexts.

It's not really clear to me what real problem the current proposal would address, if any. @AdamMitchell-ms, can you describe a scenario different than the one I just described where your proposal would properly fix a real problem?

[AdamMitchell-ms]

@maxouellet , I am not trying to address the scenario of two separate modal input stacks. If this is a scenario that we want to support, then we would need to expose a way to create new input stacks and push modal input handlers onto them separately from the concept of global modal input. If we did expose a way to create multiple stacks, then I think what I've proposed would still be useful for each stack instance.

The scenario I'm trying to address is this:

1. Modal dialog A pops up and asks the user for input
2. Before A is closed, a possibly unrelated dialog B pops up and requests modal input
3. Before B is closed, the code owning dialog A decides that it no longer needs modal input for whatever reason, so it calls PopModalInput()

Since these pushes and pops were called out of order, modal input for B is removed and modal input is now given to A, which is no longer listening for it. Now it is impossible for dialog B to receive input and it will therefore never close. The user is stuck.

What I've proposed addresses this situation. Callers still push model input handlers only onto the top of the stack. But then whenever they decide that they no longer need modal input, they can only remove _themselves_ from wherever they are in the stack (even if they are not on top). They can never accidentally remove someone else's modal input.

This exact issue did occur in our app (MS Layout actually, I joined the Layout team a month ago). We had previously modified our version of InputManager to take in the inputHandler in the call to PopModalInputHandler and we would throw if it didn't match what was at the top of the stack. This caused a crash that we saw in our telemetry. I resolved it by making the changes described above to our branch of MRTK.

But I in a more general sense, I think it is good API design to make it hard or impossible for the caller to mess things up for themselves or others (for example, to call things in the wrong order resulting in broken behavior). With the current public API of InputManager, there is nothing stopping anyone from accidentally calling PopModalInputManager more than once. My proposed API would make it so that the only one who can remove a modal input handler from the stack is the one who put it there.

[AdamMitchell-ms]

@maxouellet
PS. If you feel that the name "Add" is problematic, we could keep it as "Pop" but still make the changes I described.

@StephenHodgson
If we let the user pass in the modal input handler to the Pop/Remove function, it is still possible for them to accidentally call it more than once or to pass in the wrong item. If we force them to call a function on the (preferably IDisposable) object returned from the Push function, then they can't possibly do the wrong thing.

[StephenHodgson]

@AdamMitchell-ms I'd like to see an example usage. Could you open a PR with the suggested changes on the mrtk-deveopment branch?

[maxouellet]

I guess it depends on what you consider correct. I'd argue that the behavior you describe is incorrect, since dialog A shouldn't close before dialog B in this case. I understand how this scenario could happen (maybe dialog A has some condition that can be automatically met that closes the dialog while dialog B is still up), but the fact that two independent modal dialogs can be open at the same time and be on the same modal stack seems like a confusing design (which is an entirely separate discussion that I won't get into).

I guess in the end, the question is whether we'd rather offer more flexibility, at the cost of not catching actual errors in the code. I personally would rather have the API tell me when things are wrong rather than offer me flexibility that can lead to unexpected bugs, but that's a wider discussion :) I think there are a bunch of scenarios where modal inputs should be unregistered in the opposite order they were registered in, and not doing so indicates potential programming errors. Catching those can be valuable and prevent having to hunt down hard-to-find issues.

All that being said, I understand the edge cases such as the ones you describe: if a majority of people feel that would be better, go ahead and make the change. Maybe one possibility would be to just log a warning when things are unregistered in the wrong order, if we think it's valuable to know that information.

Also, I think disposables are great, but I'm not sure that their usage will be obvious to everyone using the MRTK, so people might end up being confused that their modal listeners are getting disposed of early if they don't keep a pointer to the handle.

[StephenHodgson]

I'm not sure that their usage will be obvious to everyone using the MRTK, so people might end up being confused that their modal listeners are getting disposed of early

That's my main concern with the IDisposable, for people who are more familar with development in Unity but not having a more traditional programming background.

[AdamMitchell-ms]

That's a good point. I'll try to think about a way to make it very clear to callers that they need to hold onto the return value. It's too bad that C# lacks anything equivalent to [[nodiscard]] from C++ 17.

[Ecnassianer]

PS. If you feel that the name "Add" is problematic, we could keep it as "Pop" but still make the changes I described.

Just gonna pop in to say that I do think the naming is important here. "Pop" strongly implies LIFO behavior, so I don't think we should use it if we're not strictly a stack. However, because modal focus is essentially moving up and down a stack, I don't think we should use "Add" either, since it strongly implies there is no implicit ordering. Here are some options that I think wouldn't mislead API users:

ApplyModalInputHandler/DismissModalInputHandler
CaptureModalInput/ReleaseModalInput
ApplyModalInputHandler/RemoveModalInputHandler
PushModalInputHandler/RemoveModalInputHandler

These also lean into the idea that the most recent modal input gets focus, even if a previous manager is trying to hog it thus prompting API users to plan accordingly.

[StephenHodgson]

These also lean into the idea that the most recent modal input manager gets focus, even if a previous manager is trying to hog it thus prompting API users to plan accordingly.

Do you mean input source? not manager? (We only ever have one manager)

[StephenHodgson]

As far as the naming is concerned in vNEXT we don't have any concept of "Add".

#region Modal Input Options

/// <summary>
/// Push a game object into the modal input stack. Any input handlers
/// on the game object are given priority to input events before any focused objects.
/// </summary>
/// <param name="inputHandler">The input handler to push</param>
public void PushModalInputHandler(GameObject inputHandler)
{
    modalInputStack.Push(inputHandler);
}

/// <summary>
/// Remove the last game object from the modal input stack.
/// </summary>
public void PopModalInputHandler()
{
    if (modalInputStack.Count > 0)
    {
        modalInputStack.Pop();

    }
}

/// <summary>
/// Clear all modal input handlers off the stack.
/// </summary>
public void ClearModalInputStack()
{
    modalInputStack.Clear();
}

#endregion Modal Input Options

#region Fallback Input Handler Options

/// <summary>
/// Push a game object into the fallback input stack. Any input handlers on
/// the game object are given input events when no modal or focused objects consume the event.
/// </summary>
/// <param name="inputHandler">The input handler to push</param>
public void PushFallbackInputHandler(GameObject inputHandler)
{
    fallbackInputStack.Push(inputHandler);
}

/// <summary>
/// Remove the last game object from the fallback input stack.
/// </summary>
public void PopFallbackInputHandler()
{
    fallbackInputStack.Pop();
}

/// <summary>
/// Clear all fallback input handlers off the stack.
/// </summary>
public void ClearFallbackInputStack()
{
    fallbackInputStack.Clear();
}

#endregion Fallback Input Handler Options

[StephenHodgson]

I'm also wondering if we should update the GameObject parameter to be something more specific, like an interface that's meant for only handling modal or fallback input. That way we could also raise events to them when the stack it popped, pushed, or cleared.

[Ecnassianer]

Do you mean input source? not manager? (We only ever have one manager)

Yes!

As far as the naming is concerned in vNEXT we don't have any concept of "Add".

Add was suggested in the proposal.

[PJBowron]

While I don't agree that this change should be made to modal handlers (in the example presented above, I feel dialog A should not close as soon as it _can_, but rather as soon as it _should_, which is after dialog B closes) I do feel that we should make it to Fallback handlers :)

[Yoyozilla]

bulk closing older bugs. please reopen if still relevant.