JackTrapper / DelphiBugs

Bug tracker for Delphi
7 stars 2 forks source link

TListColumn.GetWidth uses ListView_GetColumnWidth rather than ListView_GetColumn, causing invalid widths to be stored #14

Open JackTrapper opened 5 years ago

JackTrapper commented 5 years ago

The code of TListColumn.GetWidth mistakenly uses ListView_GetColumnWidth:

function TListColumn.GetWidth: TWidth;
begin
   //...snip...
    FWidth := ListView_GetColumnWidth(LOwner.Handle, FOrderTag);
    Result := FWidth;
end;

This is bad, because if the functions fails, it is documented to to return zero:

Returns the column width if successful, or zero otherwise.
If this message is sent to a list-view control with the LVS_REPORT style and the specified column does not exist, the return value is undefined.

This is poor code all the way around.

Fixed:

function TListColumn.GetWidth: TWidth; var LVColumn : TLVColumn; begin //...snip... LVColumn.mask := LVCF_WIDTH;

if ListView_GetColumn(LOwner.Handle, FOrderTag, LVColumn) then FWidth := LVColumn.cx;

// Original call. Bad because FWidth gets set to zero on error // FWidth := ListView_GetColumnWidth(LOwner.Handle, FOrderTag);

Result := FWidth;

end;


Fixed version
============

Regardless of the other width-related bugs already in the ListView, it is just wrong to use `ListView_GetColumnWidth`. Fixed version of the function:

function TListColumn.GetWidth: TWidth; var IsStreaming: Boolean; LOwner: TCustomListView; LVColumn : TLVColumn; begin LOwner := TListColumns(Collection).Owner; IsStreaming := [csReading, csWriting, csLoading] * LOwner.ComponentState <> [];

// The TCustomListView handles the ReCreateWnd message by streaming the properties out to a memory stream during // DestroyWnd and then reading them back in during CreateWnd. It also sets the Reading flag when doing this. // If someone attempts to read the width while this is happening then FWidth will get overwritten with the // width of the underlying listview column width when it shouldn't be (It shouldn't be because FWidth was just // read from the memory stream in an attempt to restore it). The check for LOwner.Reading will stop // that from happening.

if not LOwner.Reading and ( ((FWidth = 0) and (LOwner.HandleAllocated or not IsStreaming)) or ((not AutoSize) and LOwner.HandleAllocated and (LOwner.ViewStyle = vsReport) and (FWidth <> LVSCW_AUTOSIZE) and (LOwner.ValidHeaderHandle)) ) then begin // Issue: The main listview column will fill the listview width and all sub-item will look hidden. They are not // actually hidden but they appear that way because they all have zero width. You can still use the mouse to resize // them but it's not obvious and it's not at all what we want the user to see.

// How to Re-Create:
// 1. Drop a TListView on a form.
// 2. Add a resize handler to the listview.
// 3. Access the column widths in the resize handler.

// Why It Happens:

 // MSDN Return Code documentation for ListView_GetColumnWidth()
// Returns the column width if successful, or zero otherwise. If this macro is used on a list-view control with the
// LVS_REPORT style and the specified column does not exist, the return value is undefined.

// The problem is that it returns zero on error or zero if the column width is zero. How do we know the difference?
// The 'original call' below sets the member variable to zero in cases where the ListView is returning error.
// That's bad because your TListColumn will now think it has a zero width and when UpdateCols gets called it will
// re-create the column with a zero width. It only happens when you access the column width early in the initialization
// phase but I haven't fully tracked down exactly when. We tried calling UpdateCols in the
// WMParentNotify handler hoping that it would initialize the columns sooner and everything would be fine but
// that doesn't seem to be the issue. If we refuse to access the column width (during a resize) until after
// the form's OnShow has been called then everything works fine but that puts the burden on the developer. The method
// below returns True on success so we only set the FWidth member if it gets valid info from the ListView.
// It's probably a little more expensive but it works. Sept. 10, 2014

// We could FillChar() the structure but it's extra cycles for nothing, mask is the only thing that matters.
// If ListView_GetColumn comes back True then we know LVColumn.cx has been set to the value we need

LVColumn.mask := LVCF_WIDTH;

 if ListView_GetColumn(LOwner.Handle, FOrderTag, LVColumn) then
    FWidth := LVColumn.cx;

 // original call, bad because FWidth gets set to zero on error

// FWidth := ListView_GetColumnWidth(LOwner.Handle, FOrderTag); end;

Result := FWidth; end;