Opened 8 years ago

Closed 6 years ago

#9379 closed enhancement (fixed)

wxDataViewListModel GetValue/SetValue interface

Reported by: troelsk Owned by:
Priority: normal Milestone:
Component: GUI-all Version:
Keywords: wxDataViewListModel Cc: troelsk, frm
Blocked By: Blocking:
Patch: no

Description

  1. Using un-const references is bad C++, and you do this in both GetValue and SetValue (and unfortunately in several other places in wx too):

virtual void GetValue( wxVariant &, ...) const;
virtual bool SetValue( wxVariant &, ...);

Please consider changing this interface to:

virtual void GetValue( wxVariant*, ...) const;
virtual bool SetValue( const wxVariant &, ...);

  1. Why is GetValue void? Please consider making it return bool.
  1. Character data speed optimization:

I propose letting wxDataViewListModel allow for a special relationship with character based data sources (xml, csv, dbf), and provide a supplemental GetValue/SetValue pair:

virtual bool GetValue(wxString*, unsigned int col, unsigned int row );
virtual bool SetValue(const wxString& value, unsigned int col, unsigned int row );

wxDataViewListModel can provide default implementations:

bool wxDataViewListModel::GetValue( wxString* str, unsigned int col, unsigned int row )
{

wxVariant var;
bool ok = GetValue(&var, col, row);
if (ok) str->operator=(var.MakeString());
return ok;

}

bool wxDataViewListModel::SetValue( const wxString& str, unsigned int col, unsigned int row)
{

return SetValue(wxVariant(str));

}

Thanks for your great work RR & FM!

Regards

Attachments (1)

sort.cpp download (1.8 KB) - added by troelsk 8 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 8 years ago by frm

all your points seems very good to me... except maybe the GetValue/SetValue specialization for wxString: wxDataViewCtrl is optimized to only call GetValue() on the items which really needs redrawing, and only when redrawing is effectively necessary, so that there shouldn't be problems of performances (even if the wxDataViewCtrl has, say, 10000 items, only 30-40 at max will be displayed at the same time...).
SetValue() is called very rarely: i.e. only when the user modifies through the wxDataViewCtrl interface the data. Definitively it's not going to be a problem from performance POV.

Last, wxVariant is refcounted and thus shouldn't be very inefficient to copy & use wxVariant objects... ;)

comment:2 Changed 8 years ago by troelsk

wxDataViewListModel can be used without wxDataViewCtrl. You could 'attach' it to wxListBox which is all about strings. Or manipulate the dataset, walk over all records of a huge dataset to fix up some strings. But perhaps what makes the case for the wxString interface:
wxVariant is unfamiliar to most developers, most will have to look up the documentation initially.
wxString most developers knows by heart, most will be able to start using wxDataViewListModel right away, using wxString.
I think wxDataViewListModel is/can be universally useful, together with wxDataViewCtrl or without it, I'd always wanted a class like this!

One more proposal:

  1. Rename wxDataViewListModel -> wxDataModel (or wxDataset or wxDatasetModel)

Changed 8 years ago by troelsk

comment:3 Changed 8 years ago by troelsk

  1. Make it GetValue(row,col) instead of GetValue(col,row)

Ad. 3: The backend in theory could cache the entire dataset in wxVariants, but normally the data will be moved into new wxVariant instances on demand (when scrolling). Creating and destroying wxVariants is hard on memory management (uses new/delete on the data instead of storing it in a union), which especially slows things down if using wxDataViewSortedListModel. (Brillant design choice to make wxDataViewSortedListModel a mix-in class!)
Attachment:
Optimized wxDataViewListModelSortedDefaultCompare, utilizing hash and GetValue(wxString*).

File Added: sort.cpp

comment:4 Changed 8 years ago by frm

wxDataViewListModel can be used without wxDataViewCtrl. You could 'attach'
it to wxListBox which is all about strings. Or manipulate the dataset, walk
over all records of a huge dataset to fix up some strings.

I don't think many people will ever use wxDataViewListModel outside the wxDataViewCtrl area... and in any case, it's easy to derive a class from it which deals with a specific API with wxString...

But perhaps what
makes the case for the wxString interface:
wxVariant is unfamiliar to most developers, most will have to look up the
documentation initially.
wxString most developers knows by heart, most will be able to start using
wxDataViewListModel right away, using wxString.

I think that an example like:

virtual void GetValue( ... ) const

{

variant = wxT("you can assign strings to wxVariant just as if it was a wxString!");

}

will make clear how easy is to use wxVariant... so in conclusion I don't think a specific wxString API should be added to wxDataViewListModel, nor to wxDataViewModel, sorry.

Ad. 3: The backend in theory could cache the entire dataset in wxVariants,
but normally the data will be moved into new wxVariant instances on demand
(when scrolling). Creating and destroying wxVariants is hard on memory
management (uses new/delete on the data instead of storing it in a union),
which especially slows things down if using wxDataViewSortedListModel.

isn't wxVariant refcounted? in that case passing around wxVariant objects should be efficient enough as the shared data wxVariantData won't be copied/re-allocated but just shared by light-weight objects (i.e. the wxVariant objects)...

Optimized wxDataViewListModelSortedDefaultCompare, utilizing hash and
GetValue(wxString*).

I'll include it in my next patch if not applied before, thanks!
It looks very good except maybe for that "auto" keyword (after googling a bit for it, I've found it's old and "deprecated"...) ;)

  1. Rename wxDataViewListModel -> wxDataModel (or wxDataset or

wxDatasetModel)

for reasons above I'd keep the "wxDataView" prefix; also the "List" word is important because the next big feature which must be added to wxDataView* classes is wxDataViewTreeModel: i.e. there must be a clear difference between the list-models and the tree-models, emphasized by the name of the base classes.

  1. Make it GetValue(row,col) instead of GetValue(col,row)

i.e. you suggest to change it to the "matrix" convention row;col instead of the currently used "x;y" convention (i.e. "col;row")... I don't see any big advantage in any of the two notations: the x;y one is widely used in computer programming and thus is the most familiar to the programmer but OTOH wxDataViewListModel arranges data in a "tabular" form, i.e. resemble the matrix concepts...
what do others think?

comment:5 Changed 8 years ago by troelsk

isn't wxVariant refcounted?

The wxVariants are *temporary* stack objects here (unless your database backend is built around wxVariant and stores all data in memory, in wxVariants; not likely), goes out of scope instantly.
So being refcounted is absolutely not a benefit in this kind of scenario. It's flexible but rather wasteful.
Hence the additional wxString interface, to make it possible to avoid having text strings take this detour around wxVariant (new wxVariantData, delete wxVariantData).
This is being flexible, from a performance POV.

WxDataViewCtrl is optimized to only call GetValue() on the items
which really needs redrawing, and only when redrawing is effectively
necessary, so that there shouldn't be problems of performances

Think huge database + wxDataViewSortedListModel
(walks over entire database before displaying, could take quite a while)

comment:6 Changed 8 years ago by troelsk

Another strings-only scenario: Printing and print previewing. Your wxPrintout class will want wxStrings only out of wxDataViewListModel, never anything else, AFAICS. wxVariant is not desirable here.

comment:7 Changed 6 years ago by wojdyr

  • Component set to GUI-all
  • Keywords wxDataViewListModel added

comment:8 Changed 6 years ago by troelsk

  • Resolution set to fixed
  • Status changed from new to closed
  1. Make it GetValue(row,col) instead of GetValue(col,row)

Fixed with r47545, wxDataViewVirtualListModel class:

virtual void GetValue( wxVariant &variant, unsigned int row, unsigned int col ) const = 0;
virtual bool SetValue( const wxVariant &variant, unsigned int row, unsigned int col ) = 0;

Note: See TracTickets for help on using tickets.