Opened 7 years ago

Closed 6 years ago

Last modified 4 years ago

#9484 closed enhancement (fixed)

Patch Adding Report View to wxListbook

Reported by: crjjrc Owned by:
Priority: normal Milestone:
Component: GUI-generic Version: stable-latest
Keywords: wxListbook Cc:
Blocked By: Blocking:
Patch: yes

Description

wxListbook currently uses a wxLC_ICON style unconditionally for the underlying wxListCtrl. However, if no icons are attached to the page labels, this style seems inappropriate. Until ticket 9472, iconless items were displayed with excessive whitespace. The patch on that ticket eliminated the whitespace if the control's imageList was NULL, but labels were still centered due to the wxLC_ICON style.

This patch goes a step further and modifies wxListbook to use a wxLC_REPORT style by default. If wxListbook::SetImageList is called with a non-NULL imageList, the style of list control is modified to use a wxLC_ICON style.

Users can effectively toggle between a report view and an icon view by calling SetImageList with respectively a NULL or non-NULL argument. In report view, the labels are left-aligned and displayed compactly in one column. In icon view, the labels are centered under their icons in one column (as per usual).

Since changing the wxListCtrl style is done by calling SetWindowStyleFlag, which deletes all its items, the items must be re-added. The labels and image ids are stored temporarily and then used for repopulation.

To get the image id, I had to implement wxListbook::GetPageImage(). I don't know why this was left unimplemented.

This modification seems to perform well with the notebook sample. (Under wxGTK 2,9, this sample occasionally gives me an assertion failure regarding the cursor when I navigate between pages. The failure occurred before my modifications.)

  • Chris

Attachments (5)

report_view.patch download (2.8 KB) - added by crjjrc 7 years ago.
Add report view option to wxListbook
report_view2.patch download (3.6 KB) - added by crjjrc 7 years ago.
Previous patch amended according to comments
listctrl.patch download (446 bytes) - added by crjjrc 6 years ago.
Removes unnecessary icon view assertion in GetViewRect
listbkg_icon_mac.patch download (1.2 KB) - added by crjjrc 6 years ago.
Listbook in wxMac always in icon view
msw_listctrl.patch download (1.9 KB) - added by crjjrc 6 years ago.

Download all attachments as: .zip

Change History (19)

Changed 7 years ago by crjjrc

Add report view option to wxListbook

comment:1 follow-up: Changed 7 years ago by vadz

  • Status changed from new to infoneeded_new

A question about the patch: the comment says "if image list presence changed" but the code tests whether image list itself changed. Shouldn't it test do something like

if ( (imageList != NULL) != (GetImageList() != NULL) )
{
  ...
}

?

Also, if you redo the patch to correct the above problem could you please also put the braces on their own lines as in my example instead of putting the opening brace on the same line?

Finally, I don't know where to document this behaviour but as it might be somewhat not obvious I think it would be nice to mention it somewhere in interface/listbook.h.

TIA!

comment:2 in reply to: ↑ 1 Changed 7 years ago by crjjrc

Replying to vadz:

A question about the patch: the comment says "if image list presence changed"
but the code tests whether image list itself changed.

[...]

Also, if you redo the patch to correct the above problem could you please
also put the braces on their own lines as in my example instead of putting
the opening brace on the same line?

Right on both accounts. Bad. Fixed.

Finally, I don't know where to document this behaviour but as it might be
somewhat not obvious I think it would be nice to mention it somewhere in
interface/listbook.h.

I put a short paragraph in, which is included in the modified patch.

  • Chris

Changed 7 years ago by crjjrc

Previous patch amended according to comments

comment:3 Changed 7 years ago by crjjrc

  • Status changed from infoneeded_new to new

comment:4 follow-up: Changed 6 years ago by vadz

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

Applied as r54001, thanks!

comment:5 follow-up: Changed 6 years ago by crjjrc

  • Resolution fixed deleted
  • Status changed from closed to reopened

One of my changes didn't make it into my patch -- sorry. wxListbook::GetControllerSize() calls wxListCtrl::GetViewRect(), which asserts that we are in icon view. The current patched state of src/generic/{listbkg.cpp,listctrl.cpp} fails this assertion when the listbook uses report view. I don't immediately see why the generic implementation of wxListCtrl must have an icon view for GetViewRect() to work correctly. Indeed, I comment out the assertion and the sizing seems okay. I've added a patch that removes this assertion. If someone can see why this assertion is necessary, we'll have to find a workaround.

One other problem is that the patch to listbkg.cpp doesn't work with the native OS X data browser, and I cannot figure out why. In the notebook sample, I appear to enter an infinite loop after turning off images and switching to a listbook. Also, since the native control is used for report view, GetViewRect() in listctrl_mac.cpp needs to be fleshed out a bit more. I tried using the code from the generic implementation, but GetCount() never returns. I'm in over my head on this and would appreciate any help.

  • Chris

Changed 6 years ago by crjjrc

Removes unnecessary icon view assertion in GetViewRect

comment:6 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by vadz

  • Status changed from reopened to infoneeded

Replying to crjjrc:

One of my changes didn't make it into my patch -- sorry. wxListbook::GetControllerSize() calls wxListCtrl::GetViewRect(), which asserts that we are in icon view. The current patched state of src/generic/{listbkg.cpp,listctrl.cpp} fails this assertion when the listbook uses report view. I don't immediately see why the generic implementation of wxListCtrl must have an icon view for GetViewRect() to work correctly.

The generic version probably doesn't but the assert is there because of MSW version limitation, quoting MSDN:

ListView_GetViewRect Macro
--------------------------------------------------------------------------------

Retrieves the bounding rectangle of all items in the list-view control. The list view must be in icon or small icon view. 

I've added a patch that removes this assertion. If someone can see why this assertion is necessary, we'll have to find a workaround.

If this works under Win32 in spite of what MSDN says then we certainly can remove the assert. If it doesn't work there then we'd need a workaround.

One other problem is that the patch to listbkg.cpp doesn't work with the native OS X data browser, and I cannot figure out why.

Sorry, I don't know this code at all and have no time to start hacking it now. Maybe the best would be to disable your changes under OS X unless the generic version is used there for now?

comment:7 in reply to: ↑ 6 Changed 6 years ago by crjjrc

  • Status changed from infoneeded to accepted

Replying to vadz:

One other problem is that the patch to listbkg.cpp doesn't work with the native OS X data browser, and I cannot figure out why.

Sorry, I don't know this code at all and have no time to start hacking it now. Maybe the best would be to disable your changes under OS X unless the generic version is used there for now?

All right, I'm attaching a patch that surrounds my changes to listbkg.cpp with checks for WXMAC. I've tested it, and the listbook control remains in icon view under OS X, and report view for GTK if no image list is present.

As for the GetViewRect() problem, could we have calculate our own wxRect on MSW if not in icon view? Would it a be a matter of just find the maximum label length and the sum of the labels heights?

  • Chris

Changed 6 years ago by crjjrc

Listbook in wxMac always in icon view

comment:8 follow-up: Changed 6 years ago by vadz

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

Applied in a modified form as r54318, thanks.

As for GetViewRect(), I don't know, for all I know it does work even in report view, but I didn't test it.

comment:9 in reply to: ↑ 8 Changed 6 years ago by crjjrc

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to vadz:

As for GetViewRect(), I don't know, for all I know it does work even in report view, but I didn't test it.

Well, I'm submitting a patch in which I've copied over the GetViewRect() code from generic/listctrl.cpp to msw/listctrl.cpp. If we're not in icon view, is this code appropriate? It works fine for me under MinGW, and fixes #9613. Perhaps this generic GetViewRect() belongs at a higher level? Again, I don't really understand the internal workings here. There may be a better way to get the controller size in wxListbook.

  • Chris

Changed 6 years ago by crjjrc

comment:10 Changed 6 years ago by vadz

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

I've provided a simpler implementation for the report view in r54412, it seems to work for me.

comment:11 in reply to: ↑ 4 Changed 6 years ago by R.U.10

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to vadz:

Applied as r54001, thanks!

This patch bring me some problems (I use Windows XP). I add a page in my list book before assigning the image list. But the function SetImageList() now change the list control style (which is supposed to delete items) and add the list book items anew. But the items are finally not deleted at the style change and the page is added twice in the list control.

Here is my code:

m_listbook->AddPage(wnd, wxT("myLabel")), true);
wxBitmap bmp = wxArtProvider::GetBitmap(wxART_QUESTION);
wxImageList *imgList = m_listbook->GetImageList();
if ( imgList == NULL )
{
    imgList = new wxImageList( bmp.GetWidth(), bmp.GetHeight() );
    m_listbook->AssignImageList( imgList );
}
int imgIndex = imgList->Add(bmp);
m_listbook->SetPageImage(m_listbook->GetPageCount()-1, imgIndex );

comment:12 Changed 6 years ago by VZ

(In [60274]) add test checking that switching mode doesn't change the controls contents (see #9484)

comment:13 Changed 6 years ago by vadz

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

It looks that readding the items is simply not necessary (and the unit test in the above mentioned revision should check that it remains so) so I simply removed the code doing it in r60275 and this should fix your problem -- please reopen if it didn't.

Thanks for your testing!

comment:14 Changed 4 years ago by VZ

(In [65559]) Allow use of report mode non-native wxListCtrl in wxListBook under Mac.

The use of wxListBook in report mode was disabled for wxOSX in r54001 and
r54318 (see #9484) because it created problems with the native wxListCtrl
implementation but the report mode can be used if we're using the generic
wxListCtrl version so do allow to use it if the system option governing the
choice of the version to use is set to "generic".

Of course, the real fix would be to correct the bugs in the native wxListCtrl
version and use report mode always but for now this at least restores correct
behaviour with the generic version.

Note: See TracTickets for help on using tickets.