Editor frame overwrites button bar

[egmont]

Open mcedit (possibly with a file).

Leave fullscreen, e.g. F9 -> Window -> Toggle fullscreen.

Grab the top of the frame with the mouse, and move it downwards by 1 row.

Notice that when you release the mouse, the bottom of the frame overwrites the "1Help 2Save..." button bar. This is later repaired as you continue using mcedit normally.

If you move the frame further down, it's only its two vertical frame characters at the sides that overwrite the button bar, displaying the file contents is properly limited to its designated area.

Interestingly, the bug does not occur if the frame is moved using the keyboard (F9 -> Window -> Move, then arrow keys and Enter).

[egmont]

Oh, wait... It's not just a visual glitch.

Those buttons don't work in this case!

Their functionality is still available via keyboard, e.g. F7 for search, F10 to quit etc. However, clicking on those buttons with the mouse does not do anything (or I believe tries to position the editor cursor) if there's a file in a window behind these buttons.

[egmont]

The visual glitch, as per the original bugreport, appeared in 4.8.17 (I haven't done a git bisect but presumably the big widget refactoring introduced it).

The behavioral problem (as per the followup comment), the mouse clicks not getting delivered to the bottom buttons is an older one, at least 4.8.16 is also buggy.

[mooffie]

These are two unrelated issues. The visual glitch is a trivial one.

As for the behavioral problem: That's because a WEdit has the WOP_TOP_SELECT flag, so it's above the buttonbar and "steals" its events (the reason the buttonsbar is nevertheless drawn on top of the WEdit is only because edit_update_screen() explicitly draws it).

I see two (or three) solutions for this behavioral problem:

1. Don't use WOP_TOP_SELECT. Replace any widget_select(edit) with a edit_bring_to_top(edit) which makes sure the buttonbar is on top.
2. Make WEdit's mouse handler abort a mouse-down event occurring on the last line of the screen.
3. (Hypothetical solution) Make the dialog's mouse handler tell the system to channel a mouse-down event occurring on the last line of the screen to the buttonbar. But we don't currently have such mechanism (we may need one when we work on scrollbars).

I'll be implementing the 2'nd solution.

[mooffie]

branch: 3760_wedit_over_buttonbar
Initial changeset:c9952020b32a75237881f6a18e36082903fd96fb

Notes:

1. There are two commits in this branch. The 1st is a patch that ​was left out of #3571 (I was aware of this but didn't have the will to start a campaign about it ;-).
2. The unrelated "visual glitch" isn't handled here. Lets first settle the more tricky issue.

[egmont]

Thanks a lot mooffie for looking into these!

[egmont]

Just wondering, wouldn't the first (out of the three) be a cleaner solution? I mean, now it's a one-off hack for the bottom row because the z-indexes are incorrect. Maybe fixing the z-indexes would be a better approach. (Disclaimer: You know the widget system much better than I do! :))

[zaytsev]

Andrew, could you please express your widget system expert opinion? I'm too stupid to be in a position to assess the relative merits of approaches (1) and (2)... I have the same concern as Egmont, the code fix itself looks small and clean, but Mooffie's comments make me feel that it's a hack.

[mooffie]

Replying to egmont:

Just wondering, wouldn't the first (out of the three) [...]

For the record: solution (1) was the first I tried (I'm attaching the patch, for reference) but I believed Andrew wouldn't accept it because it casts doubt about the idea of WOP_TOP_SELECT (and of removing dlg_set_top_widget() in favor of widget_select()).

Replying to egmont:

Just wondering, wouldn't the first (out of the three) be a cleaner solution? I mean, now it's a one-off hack for the bottom row because the z-indexes are incorrect. Maybe fixing the z-indexes would be a better approach.

Some random notes:

• Choosing (1) may mean the death of WOP_TOP_SELECT, which may entail a minor redesign of the API.

• "fixing the z-indexes"? One could argue that (1) doesn't fix this; that a real fix would be to put the MDI client area in a special container widget (which would clip the WEdits and thereby solve the issue). BTW, I wonder how Borland's TVision handled this.

• While our widget system is not perfect, I don't think it'd be wise to spend much resources in improving it because I don't think our needs currently demand this. Which brings us specifically to:

• The editor's MDI interface: this type of interface has long fallen out of favor. So there may not be merit in re-working the system to come up with a "clean" solution for a situation that has lost its desirability.

• One could argue, against (1), that the widget_select(edit) calls looks natural and that we should therefore keep them.

(If solution (3) were practical, I'd have chosen it, as it lets us skirt the problem. I'll try to think of a variation of this solution (in the past we tried to do something similar with the menu, but failed).)

[mooffie]

(but this patch is blocked by #3763)

[zaytsev]

I have to admit that now that I can see what Solution 1 actually entails, I'm rather inclined in favor of taking Solution 2 ;-)

[andrew_b]

Replying to zaytsev:

Andrew, could you please express your widget system expert opinion?

Sorry for the long silence, I'll take a look at this tomorrow.

[mooffie]

Replying to zaytsev:

I have to admit that now that I can see what Solution 1 actually entails, I'm rather inclined in favor of taking Solution 2 ;-)

Yeah, that's the "may entail a minor redesign of the API" I meant.

Replying to andrew_b:

Replying to zaytsev:

Andrew, could you please express your widget system expert opinion?

Sorry for the long silence, I'll take a look at this tomorrow.

(Note: solution (3), or a variation thereof, is about altering the dialog's mouse handler. Sort of like what we do with the menu. You'll remember the woes we had there (which led us to introduce Z-order), and why I think that without a proper mechanism to do this "channelling" we shouldn't give it a second try.)

[mooffie]

Replying to andrew_b:

Sorry for the long silence, I'll take a look at this tomorrow.

(And remember that the proposed code intends to fix only the mouse's "behavioral problem", not the "visual glitch", which is unrelated.)

[andrew_b]

Replying to mooffie:

1. There are two commits in this branch. The 1st is a patch that ​was left out of #3571 (I was aware of this but didn't have the will to start a campaign about it ;-).

I think, the reference to the #3571 should be removed from the comment. #3571 is closed and forgotten, now are in the branch of another ticket.

2. The unrelated "visual glitch" isn't handled here. Lets first settle the more tricky issue.

For me, this is hack. The bottom line (the button line) is owned by editor dialog not by WEdit window. Therefore mouse events on bottom line should handled in edit_dialog_mouse_callback() not in edit_mouse_callback().

[andrew_b]

Replying to mooffie:

• "fixing the z-indexes"? One could argue that (1) doesn't fix this; that a real fix would be to put the MDI client area in a special container widget (which would clip the WEdits and thereby solve the issue). BTW, I wonder how Borland's TVision handled this.

TurboVision? splits the screen area into three parts: the menu line (top line), the status line (bottom line) and the desktop (space between menu and status line). Desktop is the group which is container for other widgets and groups.

• While our widget system is not perfect,

Yep. We have only 2 levels in widget model: Widget->WDialog. In this model, we cannot have complex widgets (containers) that can have other widgets inside. TurboVision? have the power TGroup object which allows to have theoretically infinite widget chain: TView->TGroup->TGroup->...TGroup.

I don't think it'd be wise to spend much resources in improving it because I don't think our needs currently demand this. Which brings us specifically to:

The main widget which we have to implement is the group. Getting TVision as a reference design, most part of this work I ​made but not finished yet. The remain part is the routing of key events through group members.

[mooffie]

Replying to andrew_b:

2. The unrelated "visual glitch" isn't handled here. Lets first settle the more tricky issue.

For me, this is hack.

Sure it's a hack.

I gave another look at solution (3). It turns out that the apprehensions I had in comment:11 did not materialize: all works fine just by bringing the buttonbar to the top.

I'm attaching a new patch.

(BTW, would it have been ok if I had pushed it as "3760_wedit_over_buttonbar_v2"?)

[mooffie]

(As for TVision: I'll have a look at it later to see how they implement scrollbars. Are there any other "compound widgets" you were thinking of, or were you otherwise interested in TGroup mainly for layout purpose?)

[andrew_b]

I'd like to make something simple like following:

diff --git a/src/editor/editwidget.c b/src/editor/editwidget.c
index 515a586..adf892f 100644
--- a/src/editor/editwidget.c
+++ b/src/editor/editwidget.c
@@ -920,6 +920,15 @@ edit_dialog_mouse_callback (Widget * w, mouse_msg_t msg, mouse_event_t * event)
                 menubar_activate (b, drop_menus, -1);
         }
     }
+    else if (msg == MSG_MOUSE_DOWN && event->y == w->y + w->lines - 1)
+    {
+        /* buttonbar */
+        Widget *b;
+
+        b = WIDGET (find_buttonbar (DIALOG (w)));
+        b->mouse_callback (b, msg, event);
+        unhandled = FALSE;
+    }
 
     /* Continue handling of unhandled event in window or menu */
     event->result.abort = unhandled;

but it doesn't work: MSG_MOUSE_CLICK isn't sent to button bar.

[mooffie]

Replying to andrew_b:

I'd like to make something simple like following:

b->mouse_callback (b, msg, event);

but it doesn't work: MSG_MOUSE_CLICK isn't sent to button bar.

Calling the callback directly, as in w->mouse_callback (w, msg, event), is wrong for several reasons:

• Since it bypasses mouse_translate_event(), the state (w->mouse.capture) doesn't get updated, so the widget won't even see a MSG_MOUSE_UP.

• Since it bypasses mouse_process_event(), pseudo events (like MSG_MOUSE_CLICK) aren't generated. (Although in this case there won't be a problem because the system itself will call mouse_process_event() for the next event, MSG_MOUSE_UP, if you fix the previous problem.)

• The event you pass on is already "translated". I.e., the (x, y) coordinates are that of the 1st widget, not of the widget you're forwarding the event to. (Although specifically in this case, of forwarding to WButtonBar, this happens not to make a difference because a WButtonBar looks at the x-coordinate only, which happens to be that of the dialog.)

In other words, our API doesn't have a foolproof way (as opposed to an ad-hoc way) to forward mouse events (see comment:3, "to channel"). It's not hard to invent such mechanism (e.g., "event->result.forward_to = w"), it's just that we currently don't have one. But having introduced Z-order processing (in #3571) we can solve this issue simply by floating the buttonbar, as I did in my patch.

---

Now, a little excursion: The reason I asked (comment:16) if you're interested in TGroup only for the sake of scrollbars is because it's possible to implement scrollbars even without TGroup: by having the mouse API work on "regions" instead of widgets (widgets would be "regions" too). Add to this something like "event->result.forward_to = the_scrollbar_region" and all is set.

[andrew_b]

Replying to mooffie:

branch: 3760_wedit_over_buttonbar
Initial changeset:c9952020b32a75237881f6a18e36082903fd96fb

I vote for this, but don't forget ​indent 2nd commit.

[mooffie]

Replying to andrew_b:

branch: 3760_wedit_over_buttonbar
Initial changeset:c9952020b32a75237881f6a18e36082903fd96fb

I vote for this

Ok, but are you sure? We all agree that patch is a hack (for the reason you gave in comment:13).

Don't you think "SOLUTION-NUMBER-3.patch" is better? It handles the issue not in WEdit's callback but in the dialog's callback (where a very similar issue, of the menu, is also handled). The "price" is to have one more API function, widget_set_top().

(I'll soon post a small patch to fix the issue of the "visual glitch" (we have two unrelated issues in this ticket).)

[andrew_b]

Replying to mooffie:

We all agree that patch is a hack

While we have MenuBar? and ButtonBar? as common widgets in dialog, we have to apply some hacks to handle overlapped widgets.

[mooffie]

Replying to andrew_b:

Replying to mooffie:

We all agree that patch is a hack

While [...]

Ok, I'll commit that branch.

I had to rebase it (as it outdated master already, and I needed to fix the indent + minor rewording of commit message):

branch: 3760_wedit_over_buttonbar
Initial changeset:f0d4cf79d84d0beb8c0270b2830decc3808fb30a

Now, I'm attaching the patch to fix the visual glitch. Its commit message explains everything. Let me know if you're fine with it and I'll commit it in that same branch.

[mooffie]

(So, right now all I need is for you, Andrew, to review/approve the patch I've just attached.)

[andrew_b]

Replying to mooffie:

review/approve the patch I've just attached.

Looks ok. I agree.

[mooffie]

Rebase:

branch: 3760_wedit_over_buttonbar
Initial changeset:5ea5c5deb66f2e81e49ef44b9791f0d643748f6c

[mooffie]

Merged to master: [9ccd22310af65aef18e09d623147535df7e3438f]

[mooffie]

NEWS-4.8.19 updated.