Opened 12 months ago

Closed 12 months ago

Last modified 4 months ago

#15254 closed bug (fixed)

BWindow shouldn't dispatch messages on behalf of BView

Reported by: simonsouth Owned by: nobody
Priority: normal Milestone: R1/beta2
Component: Kits/Interface Kit Version: R1/Development
Keywords: Cc: axeld
Blocked By: Blocking:
Platform: All

Description

(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.
  1. 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.

  1. 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.

Attachments (1)

ViewMessageTest.cpp (3.4 KB ) - added by simonsouth 12 months ago.
Indicates which messages are actually received by BView.

Download all attachments as: .zip

Change History (17)

by simonsouth, 12 months ago

Attachment: ViewMessageTest.cpp added

Indicates which messages are actually received by BView.

comment:1 by pulkomandy, 12 months ago

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.

comment:2 by waddlesplash, 12 months ago

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...

comment:3 by waddlesplash, 12 months ago

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.

comment:4 by waddlesplash, 12 months ago

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

comment:5 by waddlesplash, 12 months ago

Cc: axeld added

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

comment:6 by pulkomandy, 12 months ago

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.

comment:7 by pulkomandy, 12 months ago

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

comment:8 by waddlesplash, 12 months ago

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.

comment:9 by simonsouth, 12 months ago

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.

comment:10 by simonsouth, 12 months ago

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

comment:11 by simonsouth, 12 months ago

(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.

comment:12 by waddlesplash, 12 months ago

Resolution: fixed
Status: newclosed

Merged in hrev53446.

comment:13 by diver, 11 months ago

This patch breaks GLUT apps like BillardGL and Celestia.

Last edited 11 months ago by diver (previous) (diff)

comment:14 by waddlesplash, 11 months ago

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

comment:15 by waddlesplash, 11 months ago

That issue was fixed in hrev53479.

comment:16 by nielx, 4 months ago

Milestone: UnscheduledR1/beta2

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

Note: See TracTickets for help on using tickets.