Ticket #3632 (closed task: fixed)

Opened 10 months ago

Last modified 9 months ago

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:1 Changed 10 months ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted

Branch: 3632_widget_flags
Initial changeset:a64ba3eb2ddb9a0555a4b5ff7ba76b76cbe9c8fd

comment:2 follow-up: ↓ 3 Changed 10 months 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 10 months 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 months 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 months ago by andrew_b

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

comment:6 Changed 9 months 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

comment:7 Changed 9 months ago by andrew_b

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