Opened 2 years ago

Closed 21 months ago

Last modified 21 months ago

#14618 closed enhancement (fixed)

Add wxListCtrl::SetAlternateRowColour()

Reported by: troelsk Owned by:
Priority: low Milestone:
Component: GUI-all Version: stable-latest
Keywords: wxListCtrl Cc:
Blocked By: Blocking:
Patch: yes

Description

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?

Attachments (3)

lc.patch download (10.0 KB) - added by troelsk 2 years ago.
Trunk
lc-exStyle.patch download (12.3 KB) - added by troelsk 2 years ago.
wxLC_EX_ROW_LINES
lc.2.patch download (12.1 KB) - added by troelsk 21 months ago.
EnableAlternateRowColours() version

Download all attachments as: .zip

Change History (19)

Changed 2 years ago by troelsk

Trunk

comment:1 Changed 2 years ago by vadz

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

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?

Changed 2 years ago by troelsk

wxLC_EX_ROW_LINES

comment:2 Changed 2 years ago by troelsk

Patch redone, adding an extra style flag, wxLC_EX_ROW_LINES.

comment:3 Changed 2 years ago by 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.

comment:4 Changed 2 years ago by 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?

comment:5 Changed 2 years ago by 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;
    }

comment:6 Changed 2 years ago by 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.

comment:7 Changed 2 years ago by 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.

comment:8 Changed 21 months ago by 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()"

comment:9 Changed 21 months ago by 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!

comment:10 follow-up: Changed 21 months ago by 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

comment:11 in reply to: ↑ 10 Changed 21 months ago by 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().

comment:12 Changed 21 months ago by 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.

Changed 21 months ago by troelsk

EnableAlternateRowColours() version

comment:13 Changed 21 months ago by 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.

comment:14 Changed 21 months ago by 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.

comment:15 Changed 21 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

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

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

Closes #14618.

comment:16 Changed 21 months ago by troelsk

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

Note: See TracTickets for help on using tickets.