Opened 2 years ago

Last modified 17 months ago

#14163 confirmed enhancement

Remove wxDataViewCtrl::m_value and forward to the model in its GetValue() instead

Reported by: Hanmac Owned by:
Priority: low Milestone:
Component: GUI-generic Version: stable-latest
Keywords: simple Cc:
Blocked By: Blocking:
Patch: no

Description

i have a patch that adds the Value for wxEVT_COMMAND_DATAVIEW_ITEM_VALUE_CHANGED events

Attachments (1)

dataview.patch download (1.6 KB) - added by Hanmac 2 years ago.

Download all attachments as: .zip

Change History (5)

Changed 2 years ago by Hanmac

comment:1 Changed 2 years ago by vadz

  • Milestone set to 3.0
  • Priority changed from normal to low
  • Status changed from new to confirmed
  • Summary changed from DataViewCtrl should set the Value when processing an VALUE_CHANGED Event to wxDataViewCtrl should set the Value when processing an VALUE_CHANGED Event

We could apply this but looking at the code it seems that the value is almost never set actually. The generic version does it only for wxEVT_COMMAND_DATAVIEW_ITEM_CONTEXT_MENU (of all things...) while the GTK and OS X ones never do it at all.

If it were up to me, I'd remove wxDataViewEvent::GetValue() entirely. It just doesn't make much sense to have it because you can always get the value from the control (or the model) itself while preemptively adding it to the event when nobody needs it just seems useless.

Alternatively, and more backwards compatibly, we could keep GetValue() but make it fill m_value on demand when it's called by getting it from m_model.

What do you think?

P.S. In any case, we do need consistency here so if we do nothing else we ought to remove SetValue() from the generic code generating wxEVT_COMMAND_DATAVIEW_ITEM_CONTEXT_MENU event before 3.0.

comment:2 Changed 2 years ago by Hanmac

another idea is to remove m_value but keep Get/SetValue as shortcut for GetModel->Get/SetValue.
this would be backwards too. (but your fill on demand is also ok)

imo calling GetModel->GetValue(val,GetItem(),GetColumn()) looks stupid, and only GetValue() looks better

i dont know if we need an SetValue in the Event ...

comment:3 Changed 17 months ago by vadz

  • Keywords simple added
  • Milestone 3.0 deleted
  • Patch unset
  • Summary changed from wxDataViewCtrl should set the Value when processing an VALUE_CHANGED Event to Remove wxDataViewCtrl::m_value and forward to the model in its GetValue() instead

Removing m_value would be even better, it's indeed not public in wxDataViewEvent so we can do this.

For now I'll just remove setting it.

comment:4 Changed 17 months ago by VZ

(In [73623]) Don't set cell value in wxDataViewEvent in one place only.

We should either set the cell value in the event object everywhere or not do
it anywhere and as currently the native GTK and OS X versions don't do it at
all and the generic version only does it for ITEM_CONTEXT_MENU events, it's
easier to not do it at all.

See #14163.

Note: See TracTickets for help on using tickets.