Add wxListCtrl::SetAlternateRowColour()

[troelsk]

This patch
- removes the need to override OnGetItemAttr() to achieve this
- brings the wxListCtrl and wxDataViewListCtrl interfaces a little closer *)
- moves OnGetItemAttr() to the base class
- includes documentation
- updates the sample
- tested on MSW only

*)
Or, maybe add a style like wxDV_ROW_LINES to make the interface yet closer to that of wxDataViewListCtrl,

   #define wxLC_SORT_DESCENDING  0x8000
+  #define wxLC_ROW_LINES       0x10000 // going beyond 16 bits should be okay?

[troelsk]

Trunk

[vadz]

As much as I dislike them in general, I think it should indeed be a style for consistency. However 0x10000 == wxTRANSPARENT_WINDOW and I'd prefer to avoid this clash. Perhaps we could reuse something like wxLC_AUTOARRANGE which doesn't make sense with wxLC_VIRTUAL anyhow?

[troelsk]

wxLC_EX_ROW_LINES

[troelsk]

Patch redone, adding an extra style flag, wxLC_EX_ROW_LINES.

[vadz]

Err, extra style is inconsistent with wxDVC neither. Why did you decide not to reuse one of the existing ones? I dislike extended styles even more than the normal ones, it's a hack on top of a hack...

If anything, I prefer the original patch then.

[troelsk]

Why did you decide not to reuse one of the existing ones?

Because it is confusing with two flags of the same value, doing different things. And because of the native MSW implementation, there might be some version of commctrl.dll out there that has problems with this style in combination with LVS_REPORT. Not worth the risk I think.

    if ( style & wxLC_AUTOARRANGE /* aka wxLC_ROW_LINES */ )
        wstyle |= LVS_AUTOARRANGE;

Err, extra style is inconsistent with wxDVC neither.

Fairly close?

[troelsk]

Two more compatibility methods for wxListCtrl, long overdue really.

    long GetSelectedRow(wxString* str = NULL) const
    {
       long row = InReportView() ? GetFirstSelected() : GetFocusedItem();

       if (str && (wxNOT_FOUND != row)) *str = GetItemText(row);
       return row;
    }

    bool SelectRow(long row, bool focus = true)
    {
        bool ok = SetItemState(row, wxLIST_STATE_SELECTED, wxLIST_STATE_SELECTED);

        if (ok)
        {
            if (focus)
            {
                SetItemState(row, wxLIST_STATE_FOCUSED, wxLIST_STATE_FOCUSED);
            }
            EnsureVisible(row);
        }
        return ok;
    }

[vadz]

I'm not sure how are these methods better than the existing wxListView::Select() and Focus() and either they should work in report view only (then no need for InReportView()) or they shouldn't have "Row" in their names.

But in any case, I think it's completely unrelated to the original patch, could you please move those to a separate ticket?

Concerning the original patch, I think we should apply the first one, adding the new functions, but I also wonder if they shouldn't go into wxListView only and not wxListCtrl. The reasoning is the same: they don't make any sense in non-virtual, and hence non-report, view.

[troelsk]

wxListCtrl or wxListView, it is probably more convenient for users to have most functionality in wxListCtrl, as this is the class most are familiar with. Ie DialogBlocks has a "Add wxListCtrl" menu item, no "wxListView" menu item; provided that you know the class exists you'll need to type in "wxListView" in the class field in DB, to get to this functionality.

About the other (unrelated) methods, thanks for feedback, I'll think it over.

[troelsk]

Please apply. Will be very convenient to have this in wx instead of everybody coding it in user code, and, it will slightly ease the upgrade to wxDVC

See also ticket #14617 "Document wxDataViewCtrl::SetAlternateRowColour()"

[vadz]

Looking at the patch more carefully, I'd like to change it a bit:

1. The (empty) OnGetItemAttr() in all the derived classes now seem totally useless. Why not just remove them?
2. Having overloaded SetAlternateRowColour() versions taking a colour and not taking it seems counterintuitive. What about calling the void one UseAlternateRowColours() instead? We should also document that Use() should be preferred because if we ever have a native version for OS X it would be the only one that we could implement there.
3. Do we really need GetAlternateRowColour()? Granted, it's trivial in the generic version but under OS X it doesn't seem possible to retrieve it at all. And it just doesn't seem to be very useful.

Could you please update the patch along these lines? TIA!

[troelsk]

The (empty) OnGetItemAttr() in all the derived classes now seem totally useless. Why not just remove them?

Indeed, will remove

Having overloaded SetAlternateRowColour() versions taking a colour and not taking it seems counterintuitive. What about calling the void one UseAlternateRowColours() instead

Ok

We should also document that Use() should be preferred because if we ever have a native version for OS X it would be the only one that we could implement there.

But why a OSX native version, I believed that this widget is going nowhere, is superseeded by (native on OSX) wxDataViewListCtrl, right?

Do we really need GetAlternateRowColour()? Granted, it's trivial in the generic version but under OS X it doesn't seem possible to retrieve it at all. And it just doesn't seem to be very useful.

It is a bit weird not having it

[vadz]

Replying to troelsk:

We should also document that Use() should be preferred because if we ever have a native version for OS X it would be the only one that we could implement there.

But why a OSX native version, I believed that this widget is going nowhere, is superseeded by (native on OSX) wxDataViewListCtrl, right?

I think it makes more sense to use wxDVC directly rather than use wxDVLC myself but OS X was just an example I wanted to use to show that it was possible to have support for zebra colouring but without being able to get the colour used.

Do we really need GetAlternateRowColour()? Granted, it's trivial in the generic version but under OS X it doesn't seem possible to retrieve it at all. And it just doesn't seem to be very useful.

It is a bit weird not having it

If you think about Set(), perhaps. But for me the primary function is/should be Use() and then I really don't see anything wrong with lack of Get().

[vadz]

P.S. Perhaps we could have IsUsingAlternateRowColours() and then we could have GetAlternateRowColour() which might conceivably return invalid colour if we can't retriever it. Again, this seems a bit too complicated to me for something as trivial as this but at least it would be logically consistent.

[troelsk]

EnableAlternateRowColours() version

[troelsk]

Many thanks for feedback. Ok, let's try without the getter. Patch updated. I called it "Enable" instead of "Use" and added a boolean argument, so it can be disabled.

[VZ]

(In [73238]) Don't use "Cancel" button in the about dialog of the listctrl sample.

No real changes, just remove the unnecessary button.

See #14618.

[VZ]

(In [73239]) Add wxListCtrl::EnableAlternateRowColours() and SetAlternateRowColour().

Add methods to simply enable alternative row background colours in wxListCtrl.

Closes #14618.

[troelsk]

This should be useful to most wxListCtrl users. You cleaned up the sample changes good, and the doc, thanks!