Ticket #3566 (closed task: fixed)

Opened 8 years ago

Last modified 8 years ago

Split MSG_ACTION into MSG_ACTION and MSG_NOTIFY

Reported by: mooffie Owned by: andrew_b
Priority: major Milestone: 4.8.16
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

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.

Change History

comment:1 follow-up: ↓ 4 Changed 8 years ago by 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.

comment:2 Changed 8 years ago by andrew_b

  • Blocked By 3569 added

comment:3 Changed 8 years ago by andrew_b

  • Blocked By 3569 removed

comment:4 in reply to: ↑ 1 Changed 8 years ago by 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.

comment:5 Changed 8 years ago by mooffie

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

comment:6 Changed 8 years ago by 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.)

Changed 8 years ago by mooffie

comment:7 Changed 8 years ago by mooffie

The 2nd patch:

"Split MSG_ACTION into MSG_ACTION and MSG_NOTIFY."

comment:8 Changed 8 years ago by 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!)

comment:9 Changed 8 years ago by 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).

comment:10 Changed 8 years ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.16

Branch: 3566_msg_notify
Initial changeset:bb903f0e4a2b896a1be7b05032e0211e1bd2e6eb

Version 2, edited 8 years ago by andrew_b (previous) (next) (diff)

comment:11 Changed 8 years ago by andrew_b

Rebased to recent master to resolve conflicts.
Please review.

comment:12 Changed 8 years ago by 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).)

comment:13 Changed 8 years ago by andrew_b

Rebased to current master to resolve conflicts.
Please review.

comment:14 Changed 8 years ago by 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.)

comment:15 Changed 8 years ago by 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 :-/

comment:16 follow-up: ↓ 17 Changed 8 years ago by mooffie

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

comment:17 in reply to: ↑ 16 Changed 8 years ago by 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.)

comment:18 Changed 8 years ago by zaytsev

  • Votes for changeset set to mooffie
  • Branch state changed from on review to approved

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

comment:19 Changed 8 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from mooffie to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [26232583227d37905557853fe70a6be70afbbdb5].

git log --pretty=oneline e213af2..2623258

comment:20 Changed 8 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.