BWindow shouldn't dispatch messages on behalf of BView

[simonsouth]

(TLDR: I think our BWindow isn't sending to BView all the messages it should. If you have access to BeOS, please compile and run on it the attached program. Use the mouse and keyboard to try to get as many checkboxes checked as possible, then post a screenshot with the results and I'll work on a patch for BWindow::DispatchMessage().)

---

In a number of cases, when BWindow's DispatchMessage() method receives a message targeting one of its BView instances it will invoke a handler method on the BView object directly rather than passing the message on and letting the BView handle it itself. In some cases (and there may be more) this means BView isn't receiving messages it should be.

A clear example is BWindow's handling of keystroke-related messages. ​For example:

case B_KEY_DOWN:
{
    if (!_HandleKeyDown(message)) {
        if (BView* view = dynamic_cast<BView*>(target)) {
            // TODO: cannot use "string" here if we support having
            // different font encoding per view (it's supposed to be
            // converted by _HandleKeyDown() one day)
            const char* string;
            ssize_t bytes;
            if (message->FindData("bytes", B_STRING_TYPE,
                (const void**)&string, &bytes) == B_OK) {
                view->KeyDown(string, bytes - 1);
            }
        } else
            target->MessageReceived(message);
    }
    break;
}

Here, if BWindow decides not to capture the keypress itself it "reaches into" the target BView and invokes its KeyDown() method directly, rather than passing the message on and letting the BView itself decide how to handle it (by invoking KeyDown() on its own, presumably). As a result, it's difficult for a BView subclass to override or extend this behaviour.

This strikes me as incorrect, for several reasons:

1. It embeds in BWindow knowledge about BView and its behaviour, coupling the two classes in much the way message-passing systems are meant to eliminate.

2. Since BView's KeyDown() handler is passed only limited information about the original message, subclasses that need to work at a lower level have to do so in a roundabout manner, re-requesting the message currently being handled as ​this example from Terminal illustrates:

void
TermView::DefaultState::KeyDown(const char* bytes, int32 numBytes)
{
    // (...)
    BMessage* currentMessage = fView->Looper()->CurrentMessage();
    if (currentMessage == NULL)
        return;

    currentMessage->FindInt32("modifiers", &mod);
    currentMessage->FindInt32("key", &key);
    currentMessage->FindInt32("raw_char", &rawChar);

This kind of processing should really be done in an overridden MessageReceived() method, but that's not possible right now because the object never actually receives the message.

3. This is a departure from BeOS' behaviour. The Be Book's ​section on keystroke-event handling makes this clear:

The [B_KEY_DOWN] message finally arrives at the view that's currently focused in the active window. This view's MessageReceived() function processes the keypress in whatever way it wants to.

Note there are at least eight messages that BWindow "forcibly" dispatches in this manner; it's not just keyboard events.

I'd like to fix this, and to me it makes sense that BWindow should pass on to the target BView any relevant message it decides not to handle on its own. However, to make sure this matches what was originally intended, I'm attaching to this ticket a little program with a BView subclass that listens for specific messages and indicates their receipt by checking a checkbox in its UI. This should help reveal which messages our BView should be receiving but presently isn't.

If you have access to BeOS, please compile and run the attached program, then use the mouse and keyboard to generate events and try to get all the checkboxes to "light up". Post a screenshot with the results, and I can work on updating BWindow::DispatchMessage() to match.

[simonsouth]

Indicates which messages are actually received by BView.

[pulkomandy]

I have a BeOS machine at home but I wil not be there until sunday. Remind me to try then if no one gets to it before.

[waddlesplash]

I had to make 3 changes to get this to compile on BeOS: B_INVALIDATE and CenterOnScreen() are Haiku extensions, so that didn't work of course; and the BCheckBox constructor needed more arguments specified than it did.

After that, it started ... but I can't get any of the checkboxes to light up. I added some debugging code and it appears MessageReceived is never getting called?! I added messages to the BCheckBoxes and called SetTarget(this), but still no dice.

Pretty confused as to what's going wrong here...

[waddlesplash]

OK, I finally managed to get something to happen: requesting the view's Frame via hey:

hey ViewMessageTest get Frame of View 0 of Window 0

... causes my printf inside the view's MessageReceived to trigger. But no other messages are.

[waddlesplash]

I think the problem was focus. I changed the view to a BTextView and I now do get events to it.

It seems UNMAPPED_KEY_DOWN and KEY_UP are all it gets. I tried setting the EventMask to include B_POINTER_EVENTS and B_MOUSE_EVENTS, and then I got 2 characters for every 1 keypress, but still no mouse events in MessageReceived (and I even added printfs to dump every message what to stdout to double check that.)

[waddlesplash]

CC axeld: Can you look over this? I think I agree with simonsouth here that sending KEY/MOUSE/etc. messages to a view's MessageReceived, instead of calling the hooks from BWindow, makes more sense. But it does indeed seem the current behavior matches BeOS; so I'd like you to double-check that you don't see a problem with that, or if you could remember one :)

[pulkomandy]

If BeOS did it like this, I don't see a reason to change it. It's worth documenting it in the Haiku book and that's about it.

Otherwise I don't see how the existing trick to get the CurrentMessage would still work, which would break all apps trying to do that (pretty much anything that handles keyboard events). This pretty much requires that KeyDown is handled while the window is inside its MessageReceived call.

This makes some sense:

• The input server is only concerned with dispatching the event to a window. It does not know what the window wants to do with it. In some cases there aren't views at all in awindow (BDirectWindow), or none of them has keyboard focus, or simply key down events need to get through some global processing first (menu shortcuts, etc). So from here it makes sense to deliver to the window.
• To forward the message to a view, one could call the view MessageReceived directly, but that breaks some invariants about it (for example, BView::CurrentMessage wouldn't work). It could re-send the message properly (through another run of the event loop) but that seems not so useful and will add latency. Sothe remaining option is direct delivery to the keydown callback.

[pulkomandy]

Oh and peware of SetTarget: it works only after the handler is attached to a looper, so you can't do it in the constructor of the window. AttachedToWindow can be used for this, or possibly the window's ReadyToRun

[waddlesplash]

Otherwise I don't see how the existing trick to get the CurrentMessage would still work, which would break all apps trying to do that (pretty much anything that handles keyboard events). This pretty much requires that KeyDown is handled while the window is inside its MessageReceived call.

I don't think so? Looper()->CurrentMessage() is still valid when the View's MessageReceived is called, and thus it will still be when MouseDown() or one of the other hook functions is. So this potential change should not break this at all.

The BWindow is responsible for dispatching messages to child views. Currently it has special cases for B_KEY_DOWN and the like to just call the BView's hook function directly. We could stop doing that and instead call the BView's MessageReceived and let that handle it.

[simonsouth]

So this potential change should not break this at all.

Right, it doesn't; I've built a Haiku image with my changes applied (moving all the dispatch logic into BView::MessageReceived()) and it runs just fine without changes to the apps. (BView doesnt inherit from BLooper, so BView::CurrentMessage() doesn't exist.)

Although I'm surprised BeOS works the way it does, applying this change would make Haiku consistent with Be's ​description of how the system was meant to work:

If it's not a key the BWindow class (or derived class) wants to intercept, DispatchMessage() passes the message along to the currently focused view's BView::MessageReceived() function.

[simonsouth]

Here's the exact change I'm proposing, for review: ​https://review.haiku-os.org/c/haiku/+/1723

[simonsouth]

(Posting this here as I am still without access to Gerrit; see #14884.)

Following on from ​the discussion on Gerrit:

To be clear, there isn't a specific problem this change solves for me. What it does do is lead to code that is simpler, more efficient and more general in nature. It also brings Haiku in line with how Be's documentation (as well as common sense, in my opinion) says the API should behave: I would argue the ​principle of least astonishment is violated by a message-passing system that stops a message's flow halfway through and starts invoking hard-coded handler methods instead.

It seems unlikely the patch will lead to any significant regressions; for a week now I've been running a Haiku image with the patch applied and haven't noticed any issues. However, if anything does break, I'd rather fix it all now while Haiku is still young, before design quirks like these have had time to ossify.

[waddlesplash]

Merged in hrev53446.

[diver]

This patch breaks BillardGL and Celestia.

[waddlesplash]

Indeed: ​https://xref.plausible.coop/source/xref/haiku/src/libs/glut/glutEvent.cpp#787

Fortunately since our GLUT is in-tree, we can just patch this here. If it weren't, we'd need to revert the patch, but it seems we have escaped that so far :)

[waddlesplash]

That issue was fixed in hrev53479.

[nielx]

Assign tickets with status=closed and resolution=fixed within the R1/beta2 development window to the R1/beta2 Milestone