Ticket #3716 (closed defect: fixed)

Opened 7 years ago

Last modified 7 years ago

Options: Disabled status not toggled immediately

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

Description

Go to Options -> Configuration. Toggle "Single press" (either with keyboard or with mouse).

Expected: "Timeout" immediately becomes enabled/disabled accordingly.

Actual: "Timeout" becomes properly enabled/disabled only after the focus leaves and then re-enters "Single press".

The same goes for other potentially disabled fields as well, i.e. Layout -> Equal split, and Virtual FS -> Always use ftp proxy.

Attachments

3716-various-checkboxes-not-responding.patch (2.2 KB) - added by mooffie 7 years ago.

Change History

comment:1 follow-up: ↓ 3 Changed 7 years ago by mooffie

This is because of a newfangled change. The checkbox does:

send_message (w->owner, w, MSG_NOTIFY, (int) MSG_KEY, NULL);

but the dialog checks:

case MSG_NOTIFY:
    /* message from "Single press" checkbutton */
    if (sender != NULL && sender->id == configure_old_esc_mode_id && parm == (int) MSG_FOCUS)

It checks for MSG_FOCUS, not MSG_KEY. (There are more places like this.)

@Andrew: I see that focusing a widget sends the dialog a MSG_NOTIFY. Is this really needed?

Version 0, edited 7 years ago by mooffie (next)

comment:2 Changed 7 years ago by mooffie

Here's a patch.

You need to apply this after the patch at #3718 because that patch (#3718) fixes the radios to emit MSG_NOTIFY[MSG_KEY], which this patch relies on for the "Listing mode..." dialog.

Changed 7 years ago by mooffie

comment:3 in reply to: ↑ 1 Changed 7 years ago by andrew_b

Replying to mooffie:

@Andrew: I see that focusing a widget sends the dialog a MSG_NOTIFY. Is this really needed?

Really needed what? Send some message to dialog? Definitely yes. Send a MSG_NOTIFY? Let's discuss.

When I wrote c37f9b770a7af246d59e01e0746a83132a1a16ab I had a choice between MSG_NOTIFY and MSG_FOCUS/MSG_UNFOCUS. MSG_(UN)FOCUS have the only sense: these messages are sent to change widget's focus state. In this logic, if you send MSG_(UN)FOCUS to a dialog, it should change it's focus state. Currently dialog can't be (un)focused because it's unused and not implemented, but it is not a reason to send MSG_(UN)FOCUS to a dialog. Therefore I applied MSG_NOTIFY to notify widget's owner about changing focus of a widget.

I can't agree with usage of MSG_KEY instead of MSG_FOCUS as parameter of MSG_NOTIFY because key is just one of two cases of (un)focusing. The second key is mouse. Perhaps we should create a special message (say, MSG_CHANGE_FOCUS).

comment:4 follow-up: ↓ 5 Changed 7 years ago by mooffie

Ideally I should respond after studying the recent API changes, but since I may be away from the computer for a day or two I'll give, for the meantime, my quick uninformed reading of the situation (so don't let it raise your blood pressure!):

  • MSG_NOTIFY was meant to be like the extremely useful on_change from other GUIs. It was meant to inform the dialog (or related widgets) that a widget has changed its value. Or possibly that some useful event peculiar to it has occurred ("I've finished loading the file!").
  • Then, if I understand correctly, because you wanted to inform the dialog of a focus change, you split MSG_NOTIFY into two. The old meaning (that of on_change) has parm==MSG_KEY (even when caused by the mouse) and the new meaning has parm==MSG_FOCUS.
  • In other words, you carried MSG_NOTIFY's meaning farther than I initially expected. You use it to notify the dialog of changes to w->state. Judging by the result, this makes the code less pretty. To me this looks like the sad situation we had with MSG_ACTION, which used to serve two purposes.


Perhaps we should create a special message (say, MSG_CHANGE_FOCUS).


Certainly! You mean, instead of MSG_NOTIFY[MSG_FOCUS], right? No need to bastardize MSG_NOTIFY. Creating a new message has a huge benefit: we can immediately see if we use it at all, where we use it, and why the message needs to exist.

MSG_(UN)FOCUS have the only sense: these messages are sent to change widget's focus state.


I didn't know that's the new meaning of MSG_(UN)FOCUS.

Are you sure your wording is exact? I thought its meaning now would be to tell the widget: "You've just (lost)received the focus!". The widget, I thought, has already got its state (WST_FOCUSED) changed by this point. It'd be followed by MSG_DRAW. Again: I'll need to study the code.

In the past I suggested to break MSG_FOCUS into MSG_QUERY_FOCUS ("tell me, are you able to receive the focus?", to which it should answer yes/no) and MSG_GOT_FOCUS ("You've just received the focus!", to which it won't need to answer). (This MSG_GOT_FOCUS could be renamed "MSG_FOCUS", as before.) Similarly with MSG_UNFOCUS.

I can't agree with usage of MSG_KEY instead of MSG_FOCUS as parameter of MSG_NOTIFY because key is just one of two cases of (un)focusing. The second key is mouse.


My opinion is that MSG_NOTIFY should have one purpose, and therefore should need no parameter (at least not one that changes the meaning).

My patches weren't meant to be committed. They were just a way to quickly show what caused the bug.

I can't agree with usage of MSG_KEY [...] because key is just one of two cases of (un)focusing. The second key is mouse.


I hope this is a typo. Why should we care how a widget got/lost focus? Or whether its data change (MSG_NOTIFY) was caused by mouse or keyboard?

comment:5 in reply to: ↑ 4 Changed 7 years ago by andrew_b

Replying to mooffie:

Ideally I should respond after studying the recent API changes

You can start from 5ac1c5a3e0131f49cd78bad69a5bb0068b9f02f2.

I can't agree with usage of MSG_KEY [...] because key is just one of two cases of (un)focusing. The second key is mouse.


I hope this is a typo.

Yes, this is a typo. s/second key/second case

Why should we care how a widget got/lost focus? Or whether its data change (MSG_NOTIFY) was caused by mouse or keyboard?

Exactly. MSG_NOTIGY is the general message, it doesn't depend upon event source. The name MSG_KEY means the keyboard as source.

comment:6 Changed 7 years ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Votes for changeset set to andrew_b
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.19

comment:7 Changed 7 years ago by andrew_b

  • Branch state changed from on review to approved

comment:8 Changed 7 years ago by andrew_b

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

comment:9 Changed 7 years ago by andrew_b

  • Status changed from testing to closed

comment:10 follow-up: ↓ 12 Changed 7 years ago by mooffie

Great. I'm relieved to see the new MSG_CHANGED_FOCUS.

What I still don't like is the parm == MSG_KEY / MSG_WHATEVER accompanying MSG_NOTIFY, and here's why:

I see MSG_NOTIFY as a "high level" event. Or, using other words, "application level". Or, if you will, "semantic event". Most other events are low-level: they have to do with the implementation of the widget system.

Users of MSG_NOTIFY are supposed to be interested in semantic stuff like "value has changed", "data is ready", etc. I don't see what low-level stuff like MSG_KEY (or, previously, MSG_FOCUS) has to do with this. Seems like mixing oranges with East Siberian bears. MSG_NOTIFY was supposed, I believed, to free users from having to work with exactly this sort of low level mechanics; to make them able to concentrate on just what they want.

comment:11 Changed 7 years ago by mooffie

Why is it necessary for these boxes.c dialogs to check that their radios/checboxes were modified by the keyboard and not, say, by the mouse?

comment:12 in reply to: ↑ 10 Changed 7 years ago by andrew_b

Replying to mooffie:

Great. I'm relieved to see the new MSG_CHANGED_FOCUS.

What I still don't like is the parm == MSG_KEY / MSG_WHATEVER accompanying MSG_NOTIFY:

I applied patch as is to fix bug in short time.

In general, I agree. WCheck and WRadio send MSG_NOTIFY in the only one case: when state is changed. Therefore we can omit parm == MSG_KEY / MSG_WHATEVER. I'll make it in the cleanup branch.

Note: See TracTickets for help on using tickets.