Split MSG_ACTION into MSG_ACTION and MSG_NOTIFY

[mooffie]

MSG_ACTION is used to ask a widget to execute an action.

But it has also been hijacked to serve as a notification mechanism:

• WListbox uses it to signal the dialog that the selected item has changed.
• WCheckbox uses it similarly.
• WButtonbar uses it to ask a dialog to execute some action or hand it to some other widget.
• (Maybe there are more cases; see note below about 'grep'.)

So right now MSG_ACTION serves two different purposes. I suggest to create a new message, MSG_NOTIFY, and use it for the three cases above.

Advantages:

• It will make our code simpler. Currently, there's an ugly "if (sender != NULL)" in many MSG_ACTION handlers. (And writing a proper handler is tad tricky: programmers might return MSG_HANDLED, or the opposite, for any MSG_ACTION.)

• We'll be able to 'grep' the source for such notifications. Currently we can't: it's all mixed up.

[mooffie]

Let me know if you like the idea and I'll provide a patch. But this should have to wait till the other listbox-related patches get committed because we don't want to invalidate them.

[andrew_b]

Replying to mooffie:

Let me know if you like the idea

Yes, I like it.

and I'll provide a patch.

Please.

But this should have to wait till the other listbox-related patches get committed because we don't want to invalidate them.

Done in #3569.

[mooffie]

Ok, it's ready. The ticket is solved by two patches, as follows:

[mooffie]

The 1st patch:

"Simplify buttonbar and menu handling."

Explanation:

Out dialogs' MSG_ACTION handlers often contain three "if" statements (if sender == NULL, sender == menu_bar, sender == button_bar). For the most part we can get rid of this complexity because MSG_ACTION will be used just for one purpose (executing commands). Recall that the introduction for this ticket says that making the code simpler is one of our objectives. We also make WButtonBar send the message directly to the target widget.

This simplification doesn't introduce bugs because the involved dialogs happen to not contain radio buttons or listboxes (which fire MSG_ACTION for reasons other than executing commands).

(The reason this patch is here, and not in a separate ticket, is because as long as we don't introduce MSG_NOTIFY the aforementioned handlers could develop bugs in the future.)

[mooffie]

The 2nd patch:

"Split MSG_ACTION into MSG_ACTION and MSG_NOTIFY."

[mooffie]

Now,

Here's a little demonstration of how these patches make our lives easier:

Suppose we find something that looks like a bug in the notification mechanism of WRadio (it inexplicably sends notification in MSG_CURSOR). So we 'grep' for all the places that use "MSG_NOTIFY" to see if our fix could affect existing code:

​http://www.typo.co.il/~mooffie/tmp/mc/source/R/MSG_NOTIFY.html

...without these patches, we'd have had to go through all the places that handle "MSG_ACTION" and wade out all the places that don't use it as a notification mechanism:

​http://www.typo.co.il/~mooffie/tmp/mc/source/R/MSG_ACTION.html
(this list was longer before the patches were applied!)

[mooffie]

Random comments:

• Two places in the 1st patch have the comment "Note: the buttonbar sends messages directly to the the XYZ, not to here, which is why we can pass NULL [...]". If you want to, we can add there g_assert(sender != WIDGET (find_buttonbar (h))).

• We have "case MSG_NOTIFY: if (sender != NULL && ...)" in a few places. If we agree that the sender is mandatory (it can't be otherwise, really) we can remove the NULL check. We can add this sanity check to send_message() (it already has one; but we should use g_assert() instead).

[andrew_b]

Branch: 3566_msg_notify
Initial changeset:7ea7239e1861c44a11fb23741506d937d4e74d48

[andrew_b]

Rebased to recent master to resolve conflicts.
Please review.

[mooffie]

I re-tested the code. Everything seems to work fine.

(BTW, the patch intentionally leaves untouched one line that uses MSG_ACTION: it's in dead code that's removed in the patch for ticket:3587. I didn't want to invalidate that patch (plus, it's dead code anyway).)

[andrew_b]

Rebased to current master to resolve conflicts.
Please review.

[mooffie]

I tested the code again. Works fine. (Yes, I know my own testing isn't the 2'nd party code review you're asking for, but it's better than nothing.)

[zaytsev]

@mooffie, are you going to have time to have a further look at it this week? I'm asking because this ticket still has 4.8.16 as a milestone, and I think it would be really awesome if I could prepare test tarballs over the weekend. However, I don't think I'm in a position to provide a useful review for this one :-/

[mooffie]

@zaytsev: yes, I'll give it a another look-over tomorrow.

[mooffie]

Replying to mooffie:

@zaytsev: yes, I'll give it another look-over tomorrow.

Okay, I've just finished inspecting the code again and didn't see something amiss in it.

Basically, what I'm doing is looking at ​all the places where MSG_ACTION is used with spacial attention to "case MSG_ACTION:" lines: these are sections that potentially handle the buttonbar or the menu and those of them that do mumbo jumbo are simplified. When MSG_ACTION is used to send/receive notifications it's changed to MSG_NOTIFY.

As for the buttonbar simplification:

The code that handled the buttonbar in 'src/diffviewer/ydiff.c' and 'src/viewer/actions_cmd.c' used find_widget_type() to search for the target widget. But that code never got executed: the buttonbar's buttons, for the Viewer and Diffviewer (and all other places actually), ​already specify the target widget (or use NULL for the dialog) --that's the last argument to buttonbar_set_label(). (BTW, the "[1] Help" button in the Diffviewer doesn't work (when you click on it with the mouse), but this has nothing to do with the patch. It's a matter for a separate ticket.)

[zaytsev]

I understand that the branch is now ready to be merged.

[andrew_b]

Merged to master: [26232583227d37905557853fe70a6be70afbbdb5].

git log --pretty=oneline e213af2..2623258