High-level mouse API

[mooffie]

We have about 20 widget classes and about 13 of them have mouse handlers.

The problem

There are two problems with our mouse handlers:

• They are complicated to read (and write).

• They are buggy. First, they all respond to events initiated inside other widgets. Second, each has its own peculiar bugs.

The cause

These two problems stem from the low-level nature of Gpm_Event, which is the structure around which our mouse handlers operate. This structure represents a mouse event occurring in the whole screen and is utterly oblivious to regions and to the concept of a region "owning" the mouse.

To see the problem, simply drag the mouse over the whole screen: the panel will respond, then the command line, then the history button, then the buttons bar. A snafu. If you release the mouse over a WButton, the WButton will always execute its command --even if you didn't initiate the mouse press inside this WButton.

Our mouse handlers are complex mainly because they try to work around this problem.

A solution

This simple patch solves this issue by introducing a higher level mouse API (or should I say protocol). Mouse handlers then become simpler (examples follow). The existing old-style handlers can still be used.

The patch introduces lib/widget/mouse.c and lib/widget/mouse.h. There is very little code here: the reason for new files is just to keep things tidy.

"But why bother?"

You may ask, "why bother? who's using mouse in console anyway?!". Well, this proposal makes the handlers simpler, often shorter, so it makes our code more maintainable.

[mooffie]

Let's see some examples.

We'll start with the simplest.

This patch (fix-wbutton-*.patch) re-writes WButton's mouse handler.

The old handler:

static int
button_event (Gpm_Event * event, void *data)
{
    Widget *w = WIDGET (data);

    if (!mouse_global_in_widget (event, w))
        return MOU_UNHANDLED;

    if ((event->type & (GPM_DOWN | GPM_UP)) != 0)
    {
        dlg_select_widget (w);
        if ((event->type & GPM_UP) != 0)
        {
            send_message (w, NULL, MSG_KEY, ' ', NULL);
            send_message (w->owner, w, MSG_POST_KEY, ' ', NULL);
        }
    }

    return MOU_NORMAL;
}

The new one:

static void
button_mouse_callback (Widget * w, mouse_msg_t msg, mouse_event_t * event)
{
    (void) event;

    switch (msg)
    {
    case MSG_MOUSE_DOWN:
        dlg_select_widget (w);
        break;

    case MSG_MOUSE_CLICK:
        send_message (w, NULL, MSG_KEY, ' ', NULL);
        send_message (w->owner, w, MSG_POST_KEY, ' ', NULL);
        break;

    default:
        break;
    }
}

While this may be too short to convince the reader of the advantages of the new API, it does rid us of all WButton's bugs.

[mooffie]

The next example:

This patch (fix-wlistbox-*.patch) re-writes WListbox's mouse handler.

This time we'll clearly see the advantages of the new API.

The old handler:

static int
listbox_event (Gpm_Event * event, void *data)
{
    WListbox *l = LISTBOX (data);
    Widget *w = WIDGET (data);

    if (!mouse_global_in_widget (event, w))
        return MOU_UNHANDLED;

    /* Single click */
    if ((event->type & GPM_DOWN) != 0)
        dlg_select_widget (l);

    if (listbox_is_empty (l))
        return MOU_NORMAL;

    if ((event->type & (GPM_DOWN | GPM_DRAG)) != 0)
    {
        int ret = MOU_REPEAT;
        Gpm_Event local;
        int i;

        local = mouse_get_local (event, w);
        if (local.y < 1)
            for (i = -local.y; i >= 0; i--)
                listbox_back (l);
        else if (local.y > w->lines)
            for (i = local.y - w->lines; i > 0; i--)
                listbox_fwd (l);
        else if ((local.buttons & GPM_B_UP) != 0)
        {
            listbox_back (l);
            ret = MOU_NORMAL;
        }
        else if ((local.buttons & GPM_B_DOWN) != 0)
        {
            listbox_fwd (l);
            ret = MOU_NORMAL;
        }
        else
            listbox_select_entry (l, listbox_select_pos (l, l->top, local.y - 1));

        listbox_draw (l, TRUE);
        return ret;
    }

    /* Double click */
    if ((event->type & (GPM_DOUBLE | GPM_UP)) == (GPM_UP | GPM_DOUBLE))
    {
        Gpm_Event local;
        int action;

        local = mouse_get_local (event, w);
        dlg_select_widget (l);
        listbox_select_entry (l, listbox_select_pos (l, l->top, local.y - 1));

        /*
            ...Code to activate the current item...
         */
    }

    return MOU_NORMAL;
}

Can you find your hands and legs in this mess? Probably not!

The new handler:

static void
listbox_mouse_callback (Widget * w, mouse_msg_t msg, mouse_event_t * event)
{
    WListbox *l = LISTBOX (w);

    switch (msg)
    {
    case MSG_MOUSE_DOWN:
        dlg_select_widget (l);
        listbox_select_entry (l, listbox_select_pos (l, l->top, event->y));
        break;

    case MSG_MOUSE_SCROLL_UP:
        listbox_back (l);
        break;

    case MSG_MOUSE_SCROLL_DOWN:
        listbox_fwd (l);
        break;

    case MSG_MOUSE_DRAG:
        event->result.repeat = TRUE;    /* It'd be functional even without this. */
        listbox_select_entry (l, listbox_select_pos (l, l->top, event->y));
        break;

    case MSG_MOUSE_CLICK:

        if ((event->count & GPM_DOUBLE) != 0)   /* Double click */
        {
            /*
                ...Code to activate the current item...
             */
        }

        break;

    default:
        break;
    }

    if (msg != MSG_MOUSE_UP)
        listbox_draw (l, TRUE);
}

Yep, that's all! This handler does everything the old handler was trying to do. And it leaves out the bugs (e.g. ticket:3559)!

A good place to check this patch is in the "Chown" dialog (C-x o). Drag inside the listboxes, outside of them, play with buttons, and note how they don't interfere one with another.

Note that the new handlers return void. It doesn't mean they're less powerful: the old handlers had to return something to indicate whether they handled the global Gpm_Event. The events in new API aren't [usually] global, so a return indication is no longer needed. Nevertheless, in the very rare cases where some return indication is needed, the new API does allow this (see event->result.repeat = TRUE above).

[mooffie]

(In #3559) This bug vanishes if ticket:3571 gets accepted (see comment 2 there).

[mooffie]

(A note to myself: "if ((event->count & GPM_DOUBLE) != 0)" can be written simply as "if (event->count == GPM_DOUBLE)".)

[andrew_b]

Branch: 3571_high_level_mouse_api
Initial changeset:421e1fb2f09ab40c06618fb3e0b58e7d11604497

[zaytsev]

Just looked at the handlers, but all I can say is "Wow!"...

[zaytsev]

Just FYI:

../../../lib/widget/menu.c: In function ‘menubar_mouse_callback’:
../../../lib/widget/menu.c:737:26: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
cc1: all warnings being treated as errors

[zaytsev-work]

I wonder if #2380 / #2381 are doable with the new mouse API.

[andrew_b]

I wonder if #2380 / #2381 are doable with the new mouse API.

As old mouse API as new one can be used for that.

[zaytsev-work]

FYI, Travis seems to be alive again, but:

Sources not indented:

lib/widget/mouse.c

[mooffie]

Andrew, you've done terrific work in this branch! Thank you.

The handlers are a pleasure to look at. They should be printed out and hanged on billboards around the country.

(I have some very minor fixes which I'll post soon.)

[mooffie]

The following patch fixes a few minor bugs in the menu:

• (Only observable when "Drop down menus" checkbox is cleared in "Configure options" dialog.)

Press F9. Then then click on active menu header: nothing happens (it should drop). Fixed by removing if (selected != menubar->selected).

• MSG_MOUSE_DOWN (not MSG_MOUSE_CLICK) outside menu should close it.

• Removed flawed implementation of the above two items from the MSG_MOUSE_CLICK case (if (!menubar->is_dropped)).

• (Again, only observable when "Drop down menus" is as above)

Press F9. Then click where the dropdown (which isn't shown on screen) should have been. The "invisible" dropdown acts as if it's there. Fixed by adding if (!menubar->is_dropped) ... to menubar_mouse_on_menu(). It's the right thing to do in any case.

• Removed if (event->y == 0) {} branch from MSG_MOUSE_CLICK (as it's already taken care of by MSG_MOUSE_DOWN/DRAG).

• Style: use mouse_on_drop variable instead of menubar_mouse_on_menu(...). (We don't name the variable mouse_on_menu to make it clearer to the reader that it doesn't include the case where it's on the menubar).

• Added w->Mouse.capture = FALSE when menu is closed from MSG_MOUSE_DOWN (explained there).

• MSG_MOUSE_DRAG handling: we add if (mouse_on_drop) condition to match the old behavior. This is largely a matter of preference but it's probably also the better behavior because if the item changes when dragging away (!mouse_on_drop) the user would expect that releasing the mouse away too should execute the command (but it doesn't).

• Mouse wheel on the menubar: the right/left directions were incorrect.

(Note: it's probably best not to squash this patch (that is, not to add "fixup!" to title) because if we later discover other bugs we might want to examine the history to see how we were thinking.)

[mooffie]

BTW, there's special treatment for the middle mouse button in MSG_MOUSE_CLICK (of WMenuBar). I'd imagine this is something we inherited from the nineties. Does anybody use this dubious feature? I'd vote to remove it.

[mooffie]

This following simple patch updates a radio's 'pos' in MSG_MOUSE_DOWN. It's the conventional behavior in GUIs and it was also our old behavior.

[andrew_b]

Replying to mooffie:

The following patch fixes a few minor bugs in the menu:

Greate! Thanks! I'm totally agree.

• Mouse wheel on the menubar: the right/left directions were incorrect.

Thanks! Moved to separate commit to squash with main one.

(Note: it's probably best not to squash this patch (that is, not to add "fixup!" to title) because if we later discover other bugs we might want to examine the history to see how we were thinking.)

I'm agree.

I think you know, but anyway: the "fixup!" keyword is added by git as a result of git commit --fixup command. Then git uses it in git rebase -i --autosquash action.

[mooffie]

The following patch makes mouse-clicking in WListbox notify the dialog of the change. This was done in ticket:3562 already, but the new mouse handler was written before ticket:3562 got committed and so it lacks this feature. The following patch re-introduces this feature.

If we don't apply this patch, the "Directory hotlist" and the "External panelize" dialogs won't work as expected.

(Note: I removed your w->owner->state == DLG_ACTIVE condition because msg != MSG_MOUSE_CLICK now precludes the possibility of the dialog being closed at this point (though I don't think a calamity would have ensued had there been no conditions at all there).)

[mooffie]

(BTW, Andrew, I see that you're working on WEdit, which uses GPM_TRIPLE. Triple works in GPM only, not in xterm, so don't be surprised if it "doesn't work" for you. I documented a few similar "trivia facts" in ​the ui.Custom page of mc^2 and I'll pluck them out later and put them in a comment in mouse.h.)

[zaytsev]

mooffie, andrew_b, you guys rock!

[andrew_b]

When the dragging action is performed, the following action sequence is generated:
MSG_MOUSE_DOWN, MSG_MOUSE_DRAG [n times], MSG_MOUSE_UP, MSG_MOUSE_CLICK.
I think we don't want click after drag.

[mooffie]

Replying to andrew_b:

When the dragging action is performed, the following action sequence is generated:
MSG_MOUSE_DOWN, MSG_MOUSE_DRAG [n times], MSG_MOUSE_UP, MSG_MOUSE_CLICK.
I think we don't want click after drag.

My "tdlr" reply:

Feel free to change the current behavior.

I assume you've encountered some difficulty with the current behavior. In WEdit, I guess. Yes, I can imagine how this might complicate things there.

My longer reply:

I see that DOM's click event ​behaves the was you propose. And Java's ​most probably too.

OTOH, this would make our buttons/radios/checkboxes not behave as in common GUIs (which allow for "internal dragging"). But I guess it's just a sentimental issue on my part because I can't state a real benefit for this "internal dragging". (I guess people with special needs need this, but in the console they're already free to "drag" within one character cell; in a pixel-addressed GUI this is harder).

(In the future we might have solved the problem in a different way: we're likely to extend the mouse API someday in order to implement the drag-drop feature requests, so perhaps, in this theoretical future, we'd have had an easy way to detect in MSG_MOUSE_CLICK if the mouse had been dragged.)

tdlr:

Feel free to change the current behavior if you feel like it. I can't come up with a good argument against this.

(I only hope it won't complicate the WMenuBar handler.)

[mooffie]

Replying to mooffie:

My "tdlr" reply:

I meant "tldr". No wonder the Urban Dictionary gave me a weird definition ;-)

[mooffie]

I've updated the "3571-fixup-WListbox-optimize-mouse-event-processing.patch" patch of comment:17. It now uses uses an old_pos variable to detect if the selection has changed. (The old patch can be ​seen here.)

[mooffie]

Replying to andrew_b:

I think we don't want click after drag.

I assume you've encountered some difficulty with the current behavior. In WEdit, I guess. Yes, I can imagine how this might complicate things there.

I see one more good thing in your proposal: it'd make it possible to use the MSG_MOUSE_CLICK event for "pseudo" buttons (like the many hotareas in the panel, or the frame buttons in a non-fullscreen WEdit). Currently we must use MSG_MOUSE_DOWN for them if we don't want them triggered accidentally at the end of a drag.

[ossi]

Replying to mooffie:

OTOH, this would make our buttons/radios/checkboxes not behave as in common GUIs (which allow for "internal dragging").

i would contend that this isn't really true. in Qt we have a pixel threshold above which a drag is triggered (​http://doc.qt.io/qt-5/qapplication.html#startDragDistance-prop), and once this happens, no click is emitted. as Qt generally tries to emulate native platform behaviors, i think it's a reasonable assumption that this is the "common behavior".

[andrew_b]

Replying to mooffie:

Feel free to change the current behavior if you feel like it. I can't come up with a good argument against this.

OK. In addition to, I have an idea to merge mouse_msg_t into widget_msg_t and thus handle all events in widget_cb_fn.

[mooffie]

Replying to andrew_b:

I have an idea to [...] handle all events in widget_cb_fn.

But widget_cb_fn returns a value. Will there be a meaning to the value programmers return there?

In other words: are you thinking of abolishing mouse_event_t.result and instead extending cb_ret_t to include all possible mouse handling results? To me this seems a step back in elegance (I'll elaborate once you confirm that it's indeed your plan).

[mooffie]

Replying to ossi:

Replying to mooffie:

OTOH, this would make our buttons/radios/checkboxes not behave as in common GUIs (which allow for "internal dragging").

i would contend that this isn't really true. in Qt we have a pixel threshold above which [...] no click is emitted.

We're talking specifically about the behavior of buttons/radios/checkboxes.

In all the Qt applications installed on my computer these widgets behave the same as in Gtk: any distance of "internal dragging" triggers the action.

BTW, I see that this is also how these widgets behave in Dos Navigator (Turbo Vision) and Microsoft's ​EDIT.COM. (I'm not mentioning here any native Linux console app because I don't know of one with decent mouse support (yes, I did examine dialog(1) and links2 -- both suck in this regard).)

What we're about to do (comment:20) will make our buttons/radios/checkboxes deviate from the "standard" behavior. If we feel sentimental about this, it's not the end of the the world: we can later think of simple ways to restore the "standard" behavior to these specific widgets. In any case, I'm becoming more and more convinced that Andrew's proposal is the right thing to do: MSG_MOUSE_CLICK, at least by default, shouldn't be triggered at the end of a drag.

(BTW, if anybody wonders about Far Manager: its radios/checkboxes suck: they trigger on mouse down, not mouse click. Its buttons do behave the standard way.)

[ossi]

hmm, you're evidently right about qt's buttons, but for a different reason: they don't care about click events at all, but react to mouse press/release if the pointer is in the button's area. in fact, the pressed state of the button directy follows the mouse's location. there is an imporant detail here: once the button press starts, the mouse is grabbed by the button, so it can't (accidentally) trigger a different action.

this behavior is different from what you're implementing now, and obviously somewhat more complicated.
i wouldn't insist on doing it that way (it's pretty much a corner case which i obviously had to investigate myself to even tell), but the grabbing mechanism behind it is useful for a different thing as well: scroll bars.

[mooffie]

@Andrew: great job you've done on that WEdit!

I've noticed one small bug: double-clicking on the first text line of a non-fullscreen WEdit maximizes the frame (and clicking under the restore/close buttons triggers them). That's because MSG_MOUSE_UP calls edit_update_cursor(), which modifies the event (event->y--. event->x--), and the MSG_MOUSE_CLICK triggered next unfortunately sees this modification. I'm attaching a fix that makes edit_update_cursor() modify local variables instead.

[mooffie]

WInput has a little bug: dragging past the left edge makes the cursor jump to the end of the text (instead of to its beginning). That's because input_update_point() doesn't consider the case where 'pos' is negative. (We didn't witness this bug before because the old mouse handler didn't support dragging past the edges.)

I'm attaching a fix. I renamed 'input_update_point()' to 'input_screen_to_point()' (and made it only return the value) because we already have a function there to update the point (input_set_point()).

[andrew_b]

Thanks! Temporary committed without squash (with my trivial changes).

[mooffie]

@Andrew: I have only skimmed over your very recent work in this branch (from "mcedit: use the new mouse API" till "drop old mouse API"), but I was wondering whether it'd be wiser to do this work in a separate ticket. That is, maybe we should first merge to master the "uncontested" work we already have (yes, with its few ugly things: w->Mouse, set_easy_mouse_callback), and then continue the next phase, of which there are issues of a different nature to tackle, in a new branch.

[zaytsev]

@mooffie, I'm not sure of what kind of advantages would creating a brand new ticket and a brand new branch would bring us in this case, as long as you are willing to keep reviewing it. Unfortunately, I'm out on this one.

I think having the new API and the removal of the old one in one branch is perfectly alright, as long as it's split in sensible commits. Of course, it's up to you and Andrew...

[mooffie]

(Andrew: I'll post a quick review of the code later today after I wake up ("quick" because I understand it's not finished).)

[mooffie]

Alas, I need to go (will be back on Sunday) so my review will be short:

As for "mcedit: use the new mouse API":

Looks fine.

As for "File manager: use the new mouse API":

• Could midnight_handle_widget_event() be renamed to resend_mouse_event() and moved to mouse.c? You seem to need a similar functionality in dlg_mouse_event().
• Speaking of better names: mouse_process_event() could be renamed to send_mouse_events() (plural because it sends 1..n events).
• As for resend_mouse_event(): Maybe mouse_event_t should keep a pointer to the Gpm_Event and resend_mouse_event() should simply "replay" this event (instead of doing coordinates arithmetic). Currently, there seems to be a flaw in the way you resend events because there's some regression in the behavior of the mouse.

• And here's the big comment: I think we could get rid of 90% of the code in this patch by making the mouse handler execute _after_ the widgets get to see the events (we may need to tweak the menubar code a bit, I don't yet know). I propose a way in ticket:3600 (a flag that governs whether the dialog's handler is executed before or after the widgets).

As for "Drop old mouse API and use the new one":

The big question: Do we really need to convert dlg_mouse_event() to use mouse_event_t? I don't see an "aesthetic" problem with it using Gpm_Event. If you feel we need to do it, maybe it'd be better in a separate ticket?

[zaytsev]

FYI: Sources not indented

+git ls-files --modified
src/editor/editwidget.c

[mooffie]

A few mouse regressions (they existed yesterday already so they're not a result of your most recent changes):

The following was introduced in "File manager: use the new mouse API":

• There's a problem with the top line (y==0) of the right panel. First, make sure "Menubar visible" is turned off (the purpose of midnight_mouse_callback() is mainly to handle this case). Now, click any of the top-line "hot areas" of the right panel, like the "<", ".", "[^]" or ">". The panel will carry out the command (as expected) but afterwards the menu will get activated (shouldn't happen).

The following were introduced in "Drop old mouse API and use the new one":

• Chown dialog (C-x o):
  1. Click & drag in the "User name" list to change the selected item. While not releasing the mouse button, hover on the "Group name" listbox: this listbox will respond to the mouse (it shouldn't, as it doesn't have the capture).
  2. Drag past the south/north edges of any of the listboxes: it should scroll, but it doesn't.

• Dragging & resizing frames in the editor (for !fullscreen WEdits) no longer works properly. (As a sidenote: I don't believe an event pump is needed; but we probably shouldn't waste time on removing it right now.)

[andrew_b]

Replying to mooffie:

The following was introduced in "File manager: use the new mouse API":

Unconfirmed. Works for me. 4.8.15-124-g8f39693

[mooffie]

Replying to andrew_b:

Replying to mooffie:

The following was introduced in "File manager: use the new mouse API":

Unconfirmed. Works for me. 4.8.15-124-g8f39693

Yes, I see you're right. Sorry. The problem exists only till 8f39693 (the branch tip). (I deliberately skipped this commit after assuming it doesn't contain bug fixes. It modifies header files, leading to broader compilation --something I try to avoid hard on my ancient computer.)

[mooffie]

We'd better get rid of the 'click' boolean (the one mouse_process_event() accepts). This logic should be moved inside mouse_process_event(). To see why it makes sense, lets first review the 2 central functions:

mouse_translate_event()

Returns 0 or 1 mouse_event_t.

mouse_process_event()

Sends 1 or more mouse_event_t's to a widget. It sends more than one when pseudo events, like MSG_MOUSE_CLICK, are involved. (Right now we have only one pseudo event but in the future we may have more (especially related to drag-drop).)

So, the task of expanding one mouse_event_t into several belongs to mouse_process_event(), and its logic as well.

Another thing:

Widget.mouse.was_drag would be more useful if we turn it into Widget.mouse.last_event. (This makes it easier for mouse_process_event() to synthesize even more pseudo events; e.g., when it sees MSG_MOUSE_DRAG it'd check to see if last_event was MSG_MOUSE_DOWN and if so it'd send MSG_MOUSE_DRAG_START.)

[zaytsev-work]

@mooffie, you have surely heard of the zram & tmpfs, haven't you? Might help depending on what comes at premium (IO, RAM, CPU).

[andrew_b]

Replying to mooffie:

The following were introduced in "Drop old mouse API and use the new one":

It seems everything works now.

[mooffie]

Replying to andrew_b:

It seems everything works now.

I'll post a review soon.

[mooffie]

There are a couple of bugs.

1. In the filemanager and the editor:

The buttonbar (and other widgets) receives MSG_CLICK events even if there was a intervening MSG_DRAG between the MSG_DOWN and the MSG_UP (fore easier typing I'm omitting the "_MOUSE_" in message names). To see the problem, press+drag+release over "[5] Copy" (for example). The Copy dialog will pop up (it shouldn't).

The problem occurs because GPM_UP is seen at least twice by mouse_translate_event (first for the dialog's handler, then for each widget's handler till the buttonbar):

// pseudo code.
int dlg_mouse_event(WDialog *dlg, Gpm_Event * event)
{
    ...
    if not dlg->handle_mouse(mouse_translate_event(event)) then:
      for w in dlg->widgets:      
        if w->handle_mouse(mouse_translate_event(event)) then:
          break
    ...
}

Now, the problem is that mouse_translate_event() uses a global variable (was_drag) and it sets it incorrectly because it remembers the GPM_UP processed for the previous widget, instead of the GPM_DRAG of the current widget. The solution is to use a widget member instead of a global variable. You (Andrew) have done this fix before but rolled it back.

2. In the filemanager:

First, turn on "Menubar visible" in the "Layout" dialog. Now, bring the mouse over the menubar, press the button, and drag the mouse down. A problem: the menubar doesn't respond to the drag. This happens (I think) because the menubar doesn't have the capture. If a widget doesn't have the capture (w->Mouse.capture), it doesn't get MSG_DRAG events. To understand why the menubar doesn't gain the capture, look at the filemanager's handler:

// pseudo code.
int midnight_mouse_callback(WDialog *dlg, mouse_msg_t msg, mouse_event_t * event)
{
   ...
   if (neither left nor right panels handled the event) then:
     mouse_resend_event (event, menubar);
   ...
}

For a widget to gain the capture, mouse_translate_event() has to be fed the widget and a GPM_DOWN. But mouse_resend_event() doesn't call mouse_translate_event() at all. It only does some coordinate arithmetic and disregards the widgets state (I hinted at the problem in comment:36). One solution is to do what the editor's mouse handler does: instead of sending the event directly to the menubar it just focuses it and aborts (and dlg_mouse_event() then takes control and sends the event to the menubar).

3. In the filemanager:

Click, then drag, in a panel. Don't release the mouse button. Now move the mouse pointer over the menu. The menu will respond (it shouldn't, because it doesn't have the capture). The problem (I think) is because the dialog gains the capture (even though it aborted the event!) and therefore it receive MSG_DRAG events, which it then passes directly to the menu. The solution: mouse_process_event() should release the capture of a widget that aborts a MSG_DOWN. But lets first see if maybe the solution for bug #1 happens to solve this one too.

===

Now, that's enough typing for one post. I have some more to write, later.

[andrew_b]

Fixed.

[mooffie]

Replying to andrew_b:

Fixed.

I'll test the new code later.

BTW, as you see, these bugs weren't new. It's just that the previous "Drop old mouse API and use the new one" patch masked them with bugs of its own. That's why I proposed to postpone it to a separate ticket: it's hard enough to fix bugs that come from just a single source. And there's a problem with mixing several issues in a single ticket: when there's both good and bad it's hard to rollback just the bad stuff.

So now I'm mostly happy, with dlg_handle_event() back to its original form. But the thing I am now afraid of is the new "Some unification of mouse and message events handling" --not because it has bugs. Can't we postpone it to a separate ticket? (Or even indefinitely?) Some reasons I'm not too fond of it:

• We'll need some "result value" for our future drag-drop API. It might be a void *. We can't extend cb_ret_t to include it. So what's the point in making MOU_REPEAT a special case? Why have (in the future) two communication mechanisms?
• Why the urgency?
• I considered the 'result' member as adhering to "making simple things simple": in most cases you want to process the event, so why bother the programmer with the question of what to return? There's also the ease of 'grep'ping for it.
• It sucks that !MSG_HANDLED != MSG_NOT_HANDLED.

But, of course, I'm not the decider. If you want to get rid of 'event.result' then I'm fine. Possibly I'm making a mountain out of a molehill. I just vented my opinion here and you won't heard it from me again.

[mooffie]

There's a problem with the filemanager's mouse handler:

1. Make sure "Menubar visible" is toggled off (in the "Layout" dialog).
2. Be in the left panel.
3. Click any of the "<", "[^]", ">" buttons (they're at the top of the screen) of the right panel. These buttons don't work: the "invisible" menubar gets activated instead.

Supporting this "invisible" menubar was the purpose of the code that's been removed. I suggest we add the following comment there:

/* When the menubar is "invisible" (when "Menubar visible" is turned off),
 * we first check to see if any of the widgets (the panels) underneath it
 * (the y==0 line) wants to handle the event. E.g., the panels display there
 * the "<", "[^]", ">" buttons. Only if the panels don't handle the event
 * we hand it over to the menubar. */

==

BTW, there's indeed a way to get rid of the filemanager's mouse handler altogether: by making dlg_mouse_event() visit the widgets from top to bottom (instead of starting at the focused widget), as GUIs customarily behave, and making the menubar the bottomost widget. This way the problem gets solved in a "natural" way: the menubar sees events only if nobody else wanted them. We'll (probably) also need to make the menubar the topmost when activated (and bottomost when exited).

[zaytsev]

Please check the indentation of src/editor/editwidget.c, thanks!

[zaytsev]

@mooffie, thanks for explaining your motivation behind suggesting to split this work into separate tickets. I don't think it really matters, as my understanding is that we are not going to push real hard to merge it for 4.8.16 anyways, and so as long as it's OK with you to work on this in one ticket it's fine. But, of course, it's up to Andrew to decide, I don't have a strong opinion on this issue and I don't think I'm entitled to have one.

[mooffie]

Another problem:

1. Make sure "Menubar visible" is toggled off (in the "Layout" dialog).
2. Click the frame (the north horizontal line) of the left panel to activate the menu.
3. Press ESC ESC to exit the menu.
4. Click any of the buttonbar buttons. E.g., click "[5] Copy". The button won't respond. If you click it again it will.

The cause of the bug is simple: When we click the panel's frame, it (correctly) aborts the event by returning MSG_NOT_HANDLED (on condition, of course, that we haven't clicked on one of its pseudo buttons: "<", ">", "[^]"). Now, even though it aborted the event it still gains the capture. So when we next click "[5] Copy", the panel intercepts the MSG_UP and prevents a MSG_CLICK to the buttonbar. This is the same scenario of bug #3 I described in comment:45. The solution is simple: "mouse_process_event() should release the capture of a widget that aborts a MSG_DOWN".

I could provide a patch (it's just 2 or 3 lines of code) but I don't know if you want to rollback "Some unification of mouse and message events handling", so I don't know which "version" to provide a patch for. (This is also the reason I didn't provide patches for the bugs I described earlier.)

I feel bad for not contributing patches and relieving you of this work, so next time let me know what's your position on the "unification" thing. I'll be fine with whatever you decide on it.

[mooffie]

Replying to mooffie:

BTW, there's indeed a way to get rid of the filemanager's mouse handler altogether: by making dlg_mouse_event() visit the widgets from top to bottom (instead of starting at the focused widget), as GUIs customarily behave, and making the menubar the bottomost widget. This way the problem gets solved in a "natural" way: the menubar sees events only if nobody else wanted them. We'll (probably) also need to make the menubar the topmost when activated (and bottomost when exited).

I wrote a quick patch just to verify the viability of this plan.

This patch, contrary to the plan, does not bring the menubar to the top/bottom (simply because I assumed this would require of me much more debug time which ATM I haven't), but instead sends the event to the focused widget (potentially the menubar) in addition to visiting all the widgets from top to bottom. The menubar happens to be the first widget added to the filemanager, so it's already at the bottom. The patch seems to work.

[mooffie]

@Andrew: I'll work on the patches tomorrow. Don't worry about it.

[mooffie]

(In short: ignore comment:48 till here.)

[mooffie]

Okey, here's the patch to fix the menu. It's the "proper" version of the one in comment:52.

dlg_mouse_event() now visits the widgets from top to bottom. The menu is placed at the bottom (unless when active, during which time it's at the top).

[mooffie]

Here's the patch to fix the bug described at comment:51.

(You actually no longer witness the bug if you commit the patch that fixes the menu, because the bug depends on the order the widgets are visited, but the bug still exists and it's better to get rid of it now.)

[mooffie]

So the situation now is that I'm not aware of any bugs.

There are some cleanups to do, but first I need to know your (Andrew's) stand on the "unification" patch.

[andrew_b]

Replying to mooffie:

first I need to know your (Andrew's) stand on the "unification" patch.

Ok. Let forget the unification and bound our work by switching to the new mouse API as much as possible. I'm not have an aim to get this changes surely in 4.8.16 release, so we have enough time to make this branch bugs-free.

Currently, the [8d43325bec728da21a9a89a82740865835be3c3f] commit is being proposed to be last one in the branch. All fixes should be based on it.

[mooffie]

Replying to andrew_b:

Replying to mooffie:

first I need to know your (Andrew's) stand on the "unification" patch.

Ok. Let forget the unification and bound our work by switching to the new mouse API as much as possible. I'm not have an aim to get this changes surely in 4.8.16 release, so we have enough time to make this branch bugs-free.

Excellent!

Currently, the [8d43325bec728da21a9a89a82740865835be3c3f] commit is being proposed to be last one in the branch. All fixes should be based on it.

Ok. I see that the current situation is that you've added the menubar_active() function only. There are still bugs in the menu because the rest of the things in the patch at comment:55 haven't been merged. Do you want me to rewrite that patch to conform to the tip of the branch? (I'm asking because I don't know if you're doing this too at this very time. I actually started something a tad earlier but I see you've beaten me to it. Let me know when/if you've stopped working and want me to take charge.)

[andrew_b]

Replying to mooffie:

There are still bugs in the menu because the rest of the things in the patch at comment:55 haven't been merged.

I don't like the idea to move widget (menubar) in Z-order only for mouse event handling. This looks like hack. I don't think that midnight_mouse_callback() is a problem and we should get rid of it.

I'm thinking about change order of event handling in dlg_mouse_event(): first, loop of widgets, then, if event is unhandled, call mouse callback. Unfortunately, right now I'm not have time to check this idea. So if you want to do something, welcome.

[mooffie]

Replying to andrew_b:

I don't like the idea to move widget (menubar) in Z-order only for mouse
event handling. This looks like hack.

Are you sure? It doesn't at all seem hackish to me. On the contrary. And no alternative I could think of today seemed better.

(Maybe the profusion of comments I added to the patch gave the unfortunate impression of a hack? It's merely my <del>coding style</del> mental disorder, to put lots of comments.)

I'll keep thinking of some other way, but I'm not optimistic about me coming up with something better.

I'm thinking about change order of event handling in dlg_mouse_event():
first, loop of widgets, then, if event is unhandled, call mouse callback.

BTW, when I wrote ticket:3600 I thought I had a similar idea regarding the menu, but later I couldn't think of a non-hackish way to actually implement it.

[zaytsev]

FYI:

../../../src/filemanager/midnight.c: In function ‘midnight_mouse_callback’:
../../../src/filemanager/midnight.c:1613:17: error: unused variable ‘b’ [-Werror=unused-variable]

[andrew_b]

Rebased to recent master.

Branch:3571_high_level_mouse_api
Initial changeset:701f91d127e1d70e3d1c8b0cdd29cf928421b027.

I hope everything works now.

[mooffie]

Replying to andrew_b:

Rebased to recent master.

Branch:3571_high_level_mouse_api
Initial changeset:701f91d127e1d70e3d1c8b0cdd29cf928421b027.

I hope everything works now.

Bug #2 of comment:45 still exists. It describes a situation where the menu dropdowns don't respond to mouse drag.

(BTW: the reason only the dropdowns don't respond to the mouse, but the top-line is, is described in bug #3 in that comment. So altogether we have two bugs.)

[mooffie]

Replying to andrew_b:

I don't like the idea to move widget (menubar) in Z-order only for mouse event handling. This looks like hack.

I feel that you misjudged that solution. Would you kindly re-consider your opinion of it? It looks an elegant solution to me, certainly not a hack.

That solution uses very conventional, every basic principles of proper GUIs:

1. It's a very common pattern in GUIs to bring forward (in the Z-order) a card/rectangle/window you click on.

2. The mouse event (in the event pump) is sent in reverse Z-order. Proper GUIs behave this way. The reason ours behaves in a strange way (it starts with the focused widget and wraps-around) is an anomaly. (BTW: Perchance you felt that reverse Z-order is less efficient because we call g_list_last(), at the beginning? That's not so: the wrap-around we currently do (in dlg_widget_prev()) calls it too.

That solution rids us of the the flawed mouse_resend_event() (flawed, as it doesn't respect a widget's mouse state) and altogether of the filemanager's mouse handler.

I'm attaching the updated patch of that solution.

[zaytsev]

This certainly does look elegant to me, but maybe just add a comment about the order of traversal somewhere? If this also fixes bugs from comment:45, even better, but I'm too confused about the whole thread now to see if that's what you meant or not :-/

[andrew_b]

Replying to mooffie:

The reason ours behaves in a strange way (it starts with the focused widget and wraps-around) is an anomaly.

Currently, the multi-window editor is only case where Z-order is modified. In other cases, changing of widget order is not required, especially widgets are not overlapped.

E.g. you have some input line. This widget is focused (current) but not first in Z-order. You click on this input line. Which widget should start handling of this event? Current but not top in Z-order (as currently) or some other but top (as you propose)?

(BTW: Perchance you felt that reverse Z-order is less efficient because we call g_list_last(), at the beginning? That's not so: the wrap-around we currently do (in dlg_widget_prev()) calls it too.

Probably, the reverse iterating is a legacy code and some refactoring is required (but in another ticket).

[mooffie]

Andrew, let me list some "cons" and "pros", as I understand them, of implementing the invisible menu under the two methods of disseminating mouse events:

Method A: start with the focused widget (the method currently used)

Cons:

1. function added: mouse_resend_event() (flawed)
2. function added: mouse handler for each dialog bearing invisible menu, containing ad hoc code.
3. Poor maintainability: The code, it seems, is not trivial: after several tries (comments 65, 48, 45, 38) we still haven't quite got satisfactory result.

Pros:

(I couldn't think of "pros" for method A.)

Method B: start with the topmost widget (the proposed method)

Cons:

1. function added: dlg_set_bottom_widget() (counterpart to dlg_set_top_widget())

Pros:

1. Menu works out of the box: no need for ad hoc code attached to dialogs.
2. No introduction of new mouse API functions.
3. Zero maintenance: works flawlessly.

---

Replying to andrew_b:

Replying to mooffie:

The reason ours behaves in a strange way (it starts with the focused widget and wraps-around) is an anomaly.

Currently, the multi-window editor is only case where Z-order is modified. In other cases, changing of widget order is not required,

In a sense, what you're saying here is "method A is better because we can do without method B." I don't feel that it's a very legitimate argument: it ignores the cons and pros of each method. I.e., the cost.

Replying to andrew_b:

E.g. you have some input line. This widget is focused (current) but not first in Z-order. You click on this input line. Which widget should start handling of this event? Current but not top in Z-order (as currently) or some other but top (as you propose)?

It doesn't matter with which widget we start as long as we use a logical method. Both A and B are logical. But whereas method A doesn't lend itself well to the overlapping case (invisible menu), method B does.

(BTW, when I wrote that method A was "strange / anomaly" I didn't mean to say that it made no sense -- just that it was different from the common Z-order one.)

---

Now,

having said all the above, I see you, as before, the sole decider here. If you think method A is the way, then I'll accept that. I'll of course keep testing any new patch (though I myself won't be able to propose a patch that fixes the menu, at least not in the near future, because I don't currently see an elegant way to do that under method A).

[andrew_b]

@mooffie: I've applied your patch as is. Let's finish this long story.

[mooffie]

Replying to andrew_b:

@mooffie: I've applied your patch as is.

Excellent!

Let's finish this long story.

Yep.

I have a couple of minor "cosmetic" patches, which I'll post very shortly. I'm attaching the first one: it renames 'was_drag' to 'last_msg' (benefits explained in comment:41; moreover, 'was_drag' sounds like a kludge.)

[andrew_b]

Rebased to current master: [0ab87937cd0be9a1084551d7a60ee8489638c26f].

But we have a bug.
Make right panel active. The selected bar is in right panel.
Call menu (by mouse or f9 key).
Close menu (by mouse or (double) Esc key).
The selected bar is in left panel too. Both panels have selected bar.
This bug was introduced in [2d5078c6e072c06bfe0b0ffefd8915a7c4f37bd8].

[mooffie]

Replying to andrew_b:

But we have a bug.
Close menu [...]
[...] Both panels have selected bar.

Ouch ;-) OK, let me handle it.

[mooffie]

Replying to mooffie:

Replying to andrew_b:

But we have a bug.
Close menu [...]
[...] Both panels have selected bar.

Ouch ;-) OK, let me handle it.

Ok, here's the fix.

Explanation:

dlg_set_bottom_widget() and dlg_set_top_widget(), despite the similar names, have very different use cases. When one uses dlg_set_bottom_widget() he naturally isn't interested in focusing the widget (because you bury it). Indeed, this is how the early incarnation of my code behaved. I guess this is why I missed the bug. But at one point I removed this check in order to simplify dlg_set_top_or_bottom_widget() (and to compensate I added the comment "and make it current, albeit typically you'd want to switch [...]").

So the following fix adds the check to prevent the focusing when we're burying the widget.

As for the bug you saw:

It occurred because the menubar, having dlg_set_top_or_bottom_widget() attempting to focus it, refused focusing (in its MSG_FOCUS). This caused do_select_widget() to select the "next" widget, which happens to be the left panel.

Another issue:

We had the odd line "h->current = l" in dlg_set_top_or_bottom_widget(). I flagged this line as suspicious at the patch at comment:55 already. It served nothing. (One could argue that it manages the case when h->state != DLG_ACTIVE, but even if we had such use case (we don't), it'd be wrong to focus a widget without sending it messages because many of our widgets set state in MSG_FOCUS.)

So I removed that line, as well. This line is actually what caused you to see the bug: this line indirectly caused both panels to be active (panel->active) because it artificially moved the focus back to the menubar, and so the left panel, "losing" the focus, didn't get MSG_UNFOCUS and hadn't a chance to deactivate itself.

[mooffie]

BTW, when one exists the menu, the panel (which had the focus) gets sent MSG_FOCUS twice (from do_select_widget()). This problem exists in 'master' as well (that is, it is not the fault of code introduced in this branch). Fixing the focus mechanism is a matter for a separate ticket, of course.

[mooffie]

Here's my next cleanup patch.

It rids us of the 'click' variable (rationale explained in comment:41).

Now, I know we want to close this ticket already, but I think this patch is important enough:

Sending ("processing") events, including pseudo ones like MSG_MOUSE_CLICK, is the purpose of mouse_process_event(). The current situation makes mouse_translate_event() share some of this responsibility, by making it calculate the 'click'. This is bad because:

1. It pollutes mouse_translate_event() with a concern not of its own (functions should do one thing only, preferably).
2. In the future, as we add more pseudo events (e.g., for drag-drop), we don't want to follow this awkward pattern of passing even more state variables like 'click' around (as arguments for mouse_process_event()). Fortunately, there's no reason to: the patch shows this.

---

(As for the patch: in case you're wondering where the "if (in_widget)" test (in "if (in_widget) *click = whatever") went to: this test wasn't necessary: since we don't allow interim dragging, the mouse pointer can't wander outside the widget. That test belonged to the days when we did allow interim dragging.)

[mooffie]

There's a "cosmetic" GPM-related bug with dragging/resizing the editor frames.

I'm attaching a patch to fix this.

The problem:

When dragging a frame, the mouse pointer is displayed shifted 1 cell to the left and upward. When resizing a frame, the shift is bigger.

This happen because, when using GPM, tty_get_event() is the function that paints the mouse pointer. It gets the (x,y) from the Gpm_Event pointer passed as argument. Surprising, isn't it? Anyway, the thing is... edit_mouse_move_resize(), out of convenience, does some in-place arithmetic on its Gpm_Event. So when it calls tty_get_event(&gevent), it causes the mouse pointer to be displayed at a slightly off place.

The fix:

At first I thought to use some shadow variables to do the arithmetic in. Seems obvious. But it turns out that the cost, in coding, is bigger than doing a proper job and getting rid of the event pump completely. Plus, not getting rid of this pump still leaves out one cosmetic problem: as the pump finishes, the mouse pointer will be shown momentarily at the position prior to the start of the pump (reflecting the Gpm_Event MC itself uses).

The good news is that the patch makes our code shorter.

Comments for the patch:

1. Use of the forced_capture flag was removed. I don't know why it was used. My guess is that it was some means to make mouse-resizing work in tandem with keyboard-resizing. But it failed to implement the obvious case (pressing ESC to cancel a mouse-resizing) and instead implemented a dubious feature ("allow stop resize/move by mouse click outside window" -- why should we expect the user to try this instead of just pressing ESC? The user, after all, was using the keyboard in this case).

2. The comment "don't use widget_set_size() here to avoid double draw" was removed: it was wrong. The reason many lines in that file skip widget_set_size() is readability.

3. Unneeded use of "edit->force |= REDRAW_COMPLETELY" was retained for the sake of the comment I added there (as it pertains to other lines in that file doing the same).

4. Instead of keeping the code duplicating the actions that had to be carried out after setting edit->drag_state, I resorted to "code reuse" by calling edit_execute_cmd (edit, CK_WindowResize, -1) etc. Why? Because the "duplicated" code was inferior: for example, the way it painted the frame actually painted the whole widget, and it neglected to call edit_save_size() (so pressing ESC to cancel a mouse move didn't work correctly). Imperfectly duplicating code instead of reusing it is a maintenance nightmare. The one downside is that "CK_Enter" looks out of place --but the benefit is greater.

5. I called edit_execute_cmd(), not edit_handle_move_resize() directly, because this looks more elegant to me (as readers of the code don't care about a widget's internal politics. BTW, we should really create widget_command() and use it instead of calling XYZ_exeecute_cmd() in some places; I'll explain the benefits some other time).

6. I placed the code in a separate function, edit_mouse_handle_move_resize(), because I figured that in the future the main function, edit_mouse_callback(), will become bigger (as we introduce drag-drop) and we want to keep it as simple as possible.

7. I added "redraw frame and status" in the implementations for CK_WindowMove and CK_WindowResize as well. All other commands there (in edit_handle_move_resize()) do update the widget and there's no reason to make CK_WindowMove and CK_WindowResize an exception. (If you want me to delve into the subject of why these commands worked even without this update, then ask, but be prepared for a lengthy discussion (and a big waste of time).)

Please don't "squash" the patch: it'd be educational to keep the event pump in the history; maybe we'll want something similar in the future.

[andrew_b]

Replying to mooffie:

1. Use of the forced_capture flag was removed. I don't know why it was used. My guess is that it was some means to make mouse-resizing work in tandem with keyboard-resizing. But it failed to implement the obvious case (pressing ESC to cancel a mouse-resizing) and instead implemented a dubious feature ("allow stop resize/move by mouse click outside window" -- why should we expect the user to try this instead of just pressing ESC? The user, after all, was using the keyboard in this case).

I'm sure that mouse should work in tandem with keyboard correctly in all cases.
Two cases for your patch.

1. Open any file in editor. Toggle fullscreen mode. Call menu Window->Resize. Change window size using keyboard. Then click:
   • outside window. Nothing happens.
   • inside window. Window size is changed to unexpected one, neither previous nor current.

1. Open any file in editor. Toggle fullscreen mode. Resize window to be smaller than screen. Call menu Window->Move. Change window position using keyboard. Then click:
   • outside window. Nothing happens.
   • inside window. Window position is changed to unexpected one, neither previous nor current.

[mooffie]

Replying to andrew_b:

Two cases for your patch.

Ahaaa! Now I understand the intended behavior. (I hope users can divine it --because I certainly wouldn't have managed to ;-)

OK, I'm attaching a patch that fixes my previous patch. I'm doing this so you can see just what I modified. I added only 4 or 5 lines --the rest are comments.

(If you want me to attach the patch anew, let me know.)

(Some day we ought to rename MCEDIT_DRAG_NORMAL to MCEDIT_DRAG_NONE.)

[mooffie]

(I updated the patch to uncapture the mouse in edit_restore_size() as well (that's when the user hits ESC to abort a move/resize). It's not a bug I introduced: it existed before my patch.)

(An idea: we can turn edit_restore_size() into a new command, say CK_WindowCancelDrag, and implement it inside edit_handle_move_resize(). This way all assignments to 'edit->drag_state' will be in one function and we won't need to chase them around. OTOH, there's no end to improvements: it's more important to finish with this ticket.)

[andrew_b]

Replying to mooffie:

OK, I'm attaching a patch that fixes my previous patch.

Applied. Thanks!

(Some day we ought to rename MCEDIT_DRAG_NORMAL to MCEDIT_DRAG_NONE.)

Done.

Rebased to current master.

Branch:3571_high_level_mouse_api
Initial changeset:03daa62e19d6b8ccd475a108adebb008acfb16ba

[mooffie]

Replying to andrew_b:

Done.

Rebased to current master.

Thanks! You're doing herculean work on mc.

Now, there's one line of code that's missing in the branch. I added it to my patch after you already grabbed it. I explained it in comment:80. I'm attaching a patch that adds this line.

[mooffie]

This patch should be squashed with either "WEdit: get rid of mouse event pump" or "WEdit: use the new mouse API".

[andrew_b]

Done.

[andrew_b]

After 10 days of silence...

[andrew_b]

Merged to master: [51d0783bf9c4a3b312cbaba691993d83cc4c943c].

git log --pretty=oneline 50cca69..51d0783

Congratulations!

[mooffie]

Hip hip hooray!

(And thanks for adding me to AUTHORS, although I've done just a tiny fraction of the work here!)

BTW, as for the new Z-order (when sending events): one nice thing in it, I believe, is that it's going to make the implementation of scrollbar widgets a tad easier (as they'll overlap the main widget).

[zaytsev]

Tadam!