Ticket #3632 (closed task: fixed)
Refactoring of widget flags
Reported by: | andrew_b | Owned by: | andrew_b |
---|---|---|---|
Priority: | major | Milestone: | 4.8.18 |
Component: | mc-core | Version: | master |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Branch state: | merged | Votes for changeset: | committed-master |
Description
Currently, Widget::options is the mix of options and states and it should be split to:
- Widget::options. These flags are set once, when the widget is constructed, and usually stay constant (if widget is input line, it is input line always, for example);
- Widget::state. This field contains information on the state of the widget. Unlike option flags, state flags often change during the lifetime of a widget as the state of the widget changes.
- WDialog::state should be merged into Widget::state as they are defines dialog state;
- WDialog::flags should be merged into Widget::options as they are defines dialog options.
Change History
comment:2 follow-up: ↓ 3 Changed 9 years ago by mooffie
Just some random notes:
- Have you considered having MSG_STATE_CHANGED instead of MSG_DISABLE/MSG_ENABLE? (Interestingly, this could solve changeset:ebf57a81 too, though I'm not sure we'd want this as the official solution.)
- Was it wise to rename WANT_IDLE to IDLE? The line "widget_get_state(w, WST_IDLE)" reads as "the widget is idle", which is wrong because there's no such thing "idle widget".
- For the same reason (code clarity), perhaps it's better to return the "DLG" to WST_ACTIVE et al, because WST_ACTIVE doesn't mean "active widget"?
OTOH, I now see that there are going to be a bunch of other "DLG" flags (DLG_CENTER, DLG_TRYUP), and I wonder if having to keep this "DLG" is a code smell.
(It's a bit disappointing to lose dlg->state. The code was quite clear then. But perhaps it's just psychological attachment on my part, which is why I write this in parenthesis. Did it bother you too losing it?)
- As for WOP_TOP_SELECT: I was wondering if it'd be better to postpone this till after we fix the MSG_FOCUS mechanism (and, later, think of how to implement scrollbars) and know more fully what our needs are.
- Out of curiosity: what drove you to introduce this change?
comment:3 in reply to: ↑ 2 Changed 9 years ago by andrew_b
Replying to mooffie:
Just some random notes:
- Have you considered having MSG_STATE_CHANGED instead of MSG_DISABLE/MSG_ENABLE?
Change state to what one? Widget state is a set is states: at the same time widget is active, enabled, focused, etc.
- Was it wise to rename WANT_IDLE to IDLE? The line "widget_get_state(w, WST_IDLE)" reads as "the widget is idle", which is wrong because there's no such thing "idle widget".
WANT_IDLE looks like WANT_CURSOR and WAND HOTKEY, i.e. option. Current IDLE is state. I agree, this name is not quite good and can be changed to some more appropriate.
- For the same reason (code clarity), perhaps it's better to return the "DLG" to WST_ACTIVE et al, because WST_ACTIVE doesn't mean "active widget"?
Dialog is inherited from widget and therefore dialog is widget. Every visible object should be widget. This change will be used in the future.
OTOH, I now see that there are going to be a bunch of other "DLG" flags (DLG_CENTER, DLG_TRYUP), and I wonder if having to keep this "DLG" is a code smell.
See [1] below.
(It's a bit disappointing to lose dlg->state. The code was quite clear then. But perhaps it's just psychological attachment on my part, which is why I write this in parenthesis. Did it bother you too losing it?)
See above. Dialog is widget. No reason to have independent state definitions for one widget.
- As for WOP_TOP_SELECT: I was wondering if it'd be better to postpone this till after we fix the MSG_FOCUS mechanism
It does not matter what will be first. The implementation of WOP_TOP_SELECT is simple and simplify the code (get rid of special dlg_set_top_widget() function).
(and, later, think of how to implement scrollbars) and know more fully what our needs are.
See [1] below.
- Out of curiosity: what drove you to introduce this change?
[1] This is one step to #2919. I was not probably create this ticket but continue #2919. The following should be written there but I write it here.
In general: current widget subsystem in mc is very simple: there are only two layers: Widget and WDialog. It is obvious those two objects are not enough to create non-trivial complicated objects. Look at the current implementation of screens (#1490). This is a lot of hacks. In order to implement #2156 we should add more and more hacks.
We want flexible and power hierarchy of widgets. In simplest case this is Widget->WGroup->WDialog.
WGroup represents a set of widgets and it visible from outside as solid widget.
If you wand implement scrollbar within current widget subsystem, you have some problems with it positioning, sizing, drawing, communication with other widget. The scrollable widget should be group, i.e. WEditWinow = WEdit + 2 WScrollbars, WFilePanel = WFileList + WScrollbar(s), WScreen = WMenu + WButtonBar + (WFileManader or WEditor or WViewer or etc).
I'm currently slow working on implementation of WGroup, but this branch is very dirty and far from finish. I decided to split this work to several small steps to refacror current widget subsystem before implementation of WGroup.
comment:4 Changed 9 years ago by andrew_b
- Branch state changed from no branch to on review
- Milestone changed from Future Releases to 4.8.18
Branch: 3632_widget_flags
Initial changeset:619480c68b75a4120848a4b8ca46f670d5385cb0
comment:5 Changed 9 years ago by andrew_b
- Votes for changeset set to andrew_b
- Branch state changed from on review to approved
comment:6 Changed 9 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
Merged to master: [61c379b96491ded352c918b519802cf9faf050aa].
git log --pretty=oneline 5251045..61c379b
Branch: 3632_widget_flags
Initial changeset:a64ba3eb2ddb9a0555a4b5ff7ba76b76cbe9c8fd