#18724 closed enhancement (fixed)

Make HasValue virtual in wxDataViewModel and have implementations use it.

Reported by: moraleda Owned by: Vadim Zeitlin <vadim@…>
Priority: low Milestone:
Component: GUI-all Version: 3.0.3
Keywords: dataview Cc:
Blocked By: Blocking:
Patch: no

Description

It appears that for the past 11 years the method HasValue in wxDataViewModel has been a place holder for a feature that has not yet been implemented. In particular HasValue it is not used and the implementations use the hard coded equivalent of return col == 0 || !IsContainer(item) || HasContainerColumns(item);

(https://github.com/wxWidgets/wxWidgets/commit/b372f20e7ed8d5477b0b4ffa1449f86b29fa3c5b)

This means that currently Container nodes can choose whether to display values in either the first column only or all columns, and non-container nodes must display values in all columns.

I would like to write and submit a pull request to actually implement this feature. In particular, have HasValue be a virtual method and have the implementations use it, so that models may specify which items have values in which columns.

The default implementation of HasValue would be equivalent to the current hard coded behavior in order to be fully backwards compatible with existing code.

Over the years I have encountered many use cases where some columns do not make sense for some items and I feel that the suggested feature could cleanly handle these.

This ticket is a request for feedback before embarking in this project. Thank you.

Change History (4)

comment:1 Changed 14 months ago by moraleda

  • Cc jorge.moraleda@… added
  • Summary changed from New Feature Make HasValue virtual in DataView Control and have implementations use it. to Make HasValue virtual in wxDataViewModel and have implementations use it.

comment:2 Changed 14 months ago by vadz

  • Priority changed from normal to low
  • Status changed from new to confirmed

Yes, as I noted in the commit message of b372f20e7ed8d5477b0b4ffa1449f86b29fa3c5b, which added HasValue(), it might make sense to allow overriding it, so if you have a use case for it, I definitely see no objections to doing it. Moreover, AFAICS, this should be relatively straightforward.

Good luck and TIA!

comment:3 Changed 14 months ago by moraleda

  • Cc jorge.moraleda@… removed

Thank you. I have made the changes and submitted a pull request.

My design goals were:

1) If HasValue has not been overridden the behavior should be exactly the same as it was before.

2) If HasValue has been overridden, only render cells for which HasValue is true and provide sensible keyboard navigation among those cells.

Implementation wise I have removed all invocations to HasContainerColumns except in the default implementation of HasValue, and all decisions are now made based on what HasValue returns.

The rendering portion was totally straightforward, the keyboard navigation was a little more subtle. I have been playing with the sample tests and all seem to behave correctly, but please take a look at it if you can.

comment:4 Changed 14 months ago by Vadim Zeitlin <vadim@…>

  • Owner set to Vadim Zeitlin <vadim@…>
  • Resolution set to fixed
  • Status changed from confirmed to closed

In e6ab2391c/git-wxWidgets:

Merge branch 'dvc-virtual-has-value'

Allow overriding wxDataViewModel::HasValue() to specify which cells
should, and should not, show anything.

Closes https://github.com/wxWidgets/wxWidgets/pull/1792

Closes #18724.

Note: See TracTickets for help on using tickets.