Skip to content

Commit

Permalink
a ton of small, but important changes
Browse files Browse the repository at this point in the history 
I've inserted the output of a `#define DUMP_MEMUSAGE` build into the
structure definitions so that I can quickly see what needs to be
rewritten/rearranged to improve memory layout.

Now, I'm manually calling a bunch of the graphics APIs, as MFC doesn't
actually check the return values, I'd like to at least wrap them in
`VERIFY( )`, so I can catch pathological behavior (i.e. not go insane
debugging silent failures). God I hate MFC.

Also tweaked the const-correctness here and there, and replaced a few
initializations with brace-initializers.

I refactored `CSelectDrivesDlg::OnBnClickedBrowsefolder` slightly, so
that it's less hairy.

`ownerdrawnlistcontrol.h` has seen the **most**, and the **largest**
changes.

When I unrolled/separated the loops, `COwnerDrawnListCtrl::DrawItem` got
extremely ugly, unreadable, and generally too fucking big! Neither I nor
anyone else likes that sort of code, so I broke it up into far more
comprehensible chunks. Because of that I finally have a proper
understanding of what is going on in that function!

Remaining work to be done:
Inline the newly factored-out functions where appropriate.
  • Loading branch information
@ariccio
ariccio committed Jan 28, 2015
1 parent 091ea54 commit 2c0a187
Show file tree
Hide file tree
Showing 20 changed files with 574 additions and 244 deletions.
2 changes: 1 addition & 1 deletion WinDirStat/windirstat/PageGeneral.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class CPageGeneral final : public CPropertyPage {
BOOL m_listGrid;
BOOL m_listStripes;
BOOL m_listFullRowSelection;

//C4820: 'CPageGeneral' : '4' bytes padding added after data member 'CPageGeneral::m_showTimeSpent'
BOOL m_showTimeSpent;

CButton m_ctlFollowMountPoints;
Expand Down
1 change: 1 addition & 0 deletions WinDirStat/windirstat/PageTreemap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ class CPageTreemap final : public CPropertyPage {
_Field_range_( 0, 100 ) INT m_nBrightness;
_Field_range_( 0, 100 ) INT m_nCushionShading;
_Field_range_( 0, 100 ) INT m_nHeight;
//C4820: 'CPageTreemap' : '4' bytes padding added after data member 'CPageTreemap::m_nScaleFactor'
_Field_range_( 0, 100 ) INT m_nScaleFactor;

DECLARE_MESSAGE_MAP()
Expand Down
81 changes: 67 additions & 14 deletions WinDirStat/windirstat/SelectDrivesDlg.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,27 @@ namespace {
}
}

void log_GetDriveInformation_result( _In_ const CDriveInformationThread* const thread, _In_z_ PCWSTR name, _In_ const std::uint64_t total, _In_ const std::uint64_t free, _In_ const bool success ) {
const rsize_t buffer_size = 256;
wchar_t buffer_debug_out[ buffer_size ] = { 0 };
if ( success ) {
TRACE( _T( "thread (%p)->GetDriveInformation succeeded!, name: %s, total: %I64u, free: %I64u\r\n" ), thread, name, total, free );

const HRESULT pf_res_1 = StringCchPrintfW( buffer_debug_out, buffer_size, L"WDS: thread (%p)->GetDriveInformation succeeded!, name: %s, total: %I64u, free: %I64u\r\n", thread, name, total, free );
ASSERT( SUCCEEDED( pf_res_1 ) );
if ( SUCCEEDED( pf_res_1 ) ) {
OutputDebugStringW( buffer_debug_out );
}
}
else {
TRACE( _T( "thread (%p)->GetDriveInformation failed!, name: %s, total: %I64u, free: %I64u\r\n" ), thread, name, total, free );
const HRESULT pf_res_1 = StringCchPrintfW( buffer_debug_out, buffer_size, L"WDS: thread (%p)->GetDriveInformation failed!, name: %s, total: %I64u, free: %I64u\r\n", thread, name, total, free );
ASSERT( SUCCEEDED( pf_res_1 ) );
if ( SUCCEEDED( pf_res_1 ) ) {
OutputDebugStringW( buffer_debug_out );
}
}
}

}

Expand All @@ -127,10 +148,10 @@ INT CDriveItem::Compare( _In_ const COwnerDrawnListItem* const baseOther, RANGE_
return signum( m_path.compare( other->m_path ) );

case column::COL_TOTAL:
return signum( m_totalBytes - other->m_totalBytes );
return signum( static_cast<std::int64_t>( m_totalBytes ) - static_cast<std::int64_t>( other->m_totalBytes ) );

case column::COL_FREE:
return signum( m_freeBytes - other->m_freeBytes );
return signum( static_cast<std::int64_t>( m_freeBytes ) - static_cast<std::int64_t>( other->m_freeBytes ) );

case column::COL_ATTRIBUTES:
case column::COL_BYTES:
Expand Down Expand Up @@ -195,7 +216,12 @@ CDriveInformationThread::CDriveInformationThread( _In_ std::wstring path, LPARAM

BOOL CDriveInformationThread::InitInstance( ) {
const auto drive_tuple = RetrieveDriveInformation( m_path );

auto m_name_previous = m_name.load( );
if ( m_name_previous != std::get<1>( drive_tuple ) ) {
delete[ ] m_name_previous;
m_name.store( nullptr );
m_name_previous = nullptr;
}
m_success .store( std::get<0>( drive_tuple ) );
m_name .store( std::get<1>( drive_tuple ) );
m_totalBytes.store( std::get<2>( drive_tuple ) );
Expand Down Expand Up @@ -241,9 +267,11 @@ void CDrivesList::OnLButtonDown( const UINT /*nFlags*/, const CPoint /*point*/ )

void CDrivesList::OnLvnDeleteitem( NMHDR* pNMHDR, LRESULT* pResult ) {
auto pNMLV = reinterpret_cast< LPNMLISTVIEW >( pNMHDR );
delete GetItem( pNMLV->iItem );
const auto drive_list_item = GetItem( pNMLV->iItem );
TRACE( _T( "Deleting CDriveItem: %p\r\n" ), drive_list_item );
pNMLV->iItem = -1;
pNMLV->iSubItem = 0;
delete drive_list_item;
*pResult = 0;
}

Expand All @@ -255,7 +283,11 @@ void CDrivesList::OnNMDblclk( NMHDR* /*pNMHDR*/, LRESULT* pResult ) {
*pResult = 0;

auto point = GetCurrentMessage( )->pt;
ScreenToClient( &point );
ASSERT( ::IsWindow( m_hWnd ) );
//"Return value: If the function succeeds, the return value is nonzero. If the function fails, the return value is zero."
VERIFY( ::ScreenToClient( m_hWnd, &point ) );
//ScreenToClient( &point );

const auto i = HitTest( point );
if ( i == -1 ) {
return;
Expand Down Expand Up @@ -355,6 +387,9 @@ void CSelectDrivesDlg::setListOptions( ) {

void CSelectDrivesDlg::initWindow( ) {
ShowWindow( SW_SHOWNORMAL );
ASSERT(::IsWindow(m_hWnd));
//"Return value: If the function succeeds, the return value is nonzero. If the function fails, the return value is zero."
VERIFY( ::UpdateWindow( m_hWnd ) );
UpdateWindow( );
BringWindowToTop( );
SetForegroundWindow( );
Expand Down Expand Up @@ -507,9 +542,9 @@ BOOL CSelectDrivesDlg::OnInitDialog( ) {

void CSelectDrivesDlg::OnBnClickedBrowsefolder( ) {
TRACE( _T( "User wants to select a folder to analyze...\r\n" ) );
const wchar_t bobtitle[ ] = L"WinDirStat - Select Folder";

const UINT flags = BIF_RETURNONLYFSDIRS bitor BIF_USENEWUI bitor BIF_NONEWFOLDERBUTTON;
WTL::CFolderDialog bob { NULL, bobtitle, flags};
WTL::CFolderDialog bob { NULL, global_strings::select_folder_dialog_title_text, flags };
//ASSERT( m_folder_name_heap.compare( m_folderName ) == 0 );
bob.SetInitialFolder( m_folder_name_heap.c_str( ) );
auto resDoModal = bob.DoModal( );
Expand Down Expand Up @@ -648,15 +683,33 @@ LRESULT _Function_class_( "GUI_THREAD" ) CSelectDrivesDlg::OnWmuThreadFinished(
const std::uint64_t free = thread->m_freeBytes.load( );
const bool success = thread->m_success.load( );
const std::wstring name = thread->m_name.load( );
delete[ ] thread->m_name.load( );
const auto old_name_value = thread->m_name.load( );
thread->m_name.store( nullptr );
delete[ ] old_name_value;
//delete[ ] thread->m_name.load( );
LeaveCriticalSection( &_csRunningThreads );
if ( success ) {
TRACE( _T( "thread (%p)->GetDriveInformation succeeded!, name: %s, total: %I64u, free: %I64u\r\n" ), thread, name.c_str( ), total, free );
}
else {
TRACE( _T( "thread (%p)->GetDriveInformation failed!, name: %s, total: %I64u, free: %I64u\r\n" ), thread, name.c_str( ), total, free );
}

log_GetDriveInformation_result( thread, name.c_str( ), total, free, success );

//void log_GetDriveInformation_result( _In_ const CDriveInformationThread* const thread, _In_z_ PCWSTR name, _In_ const std::uint64_t total, _In_ const std::uint64_t )
//const rsize_t buffer_size = 256;
//wchar_t buffer_debug_out[ buffer_size ] = { 0 };
//if ( success ) {
// TRACE( _T( "thread (%p)->GetDriveInformation succeeded!, name: %s, total: %I64u, free: %I64u\r\n" ), thread, name.c_str( ), total, free );
// const HRESULT pf_res_1 = StringCchPrintfW( buffer_debug_out, buffer_size, L"WDS: thread (%p)->GetDriveInformation succeeded!, name: %s, total: %I64u, free: %I64u\r\n", thread, name.c_str( ), total, free );
// ASSERT( SUCCEEDED( pf_res_1 ) );
// if ( SUCCEEDED( pf_res_1 ) ) {
// OutputDebugStringW( buffer_debug_out );
// }
// }
//else {
// TRACE( _T( "thread (%p)->GetDriveInformation failed!, name: %s, total: %I64u, free: %I64u\r\n" ), thread, name.c_str( ), total, free );
// const HRESULT pf_res_1 = StringCchPrintfW( buffer_debug_out, buffer_size, L"WDS: thread (%p)->GetDriveInformation failed!, name: %s, total: %I64u, free: %I64u\r\n", thread, name.c_str( ), total, free );
// ASSERT( SUCCEEDED( pf_res_1 ) );
// if ( SUCCEEDED( pf_res_1 ) ) {
// OutputDebugStringW( buffer_debug_out );
// }
// }



Expand Down
3 changes: 3 additions & 0 deletions WinDirStat/windirstat/SelectDrivesDlg.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,14 @@ class CDriveInformationThread final : public CWinThread {
const std::wstring m_path; // Path like "C:\"
const LPARAM m_driveItem; // The list item, we belong to
std::atomic<HWND> m_dialog;
//C4820: 'CDriveInformationThread' : '4' bytes padding added after data member 'CDriveInformationThread::m_serial'
const UINT m_serial; // serial number of m_dialog
const rsize_t m_threadNum;
// "[out]"-parameters
std::atomic<PWSTR> m_name; // Result: name like "BOOT (C:)", valid if m_success
std::atomic<std::uint64_t> m_totalBytes; // Result: capacity of the drive, valid if m_success
std::atomic<std::uint64_t> m_freeBytes; // Result: free space on the drive, valid if m_success
//C4820: 'CDriveInformationThread' : '7' bytes padding added after data member 'CDriveInformationThread::m_success'
std::atomic<bool> m_success; // Result: false, iff drive is unaccessible.

};
Expand Down Expand Up @@ -176,6 +178,7 @@ class CSelectDrivesDlg final : public CDialog {

public:
// Dialog Data
//C4820: 'CSelectDrivesDlg' : '4' bytes padding added after data member 'CSelectDrivesDlg::m_radio'
int m_radio; // out.
//CString m_folderName; // out. Valid if m_radio = RADIO_AFOLDER
std::wstring m_folder_name_heap;
Expand Down
5 changes: 4 additions & 1 deletion WinDirStat/windirstat/TreeListControl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ struct VISIBLEINFO {
FILETIME filetime_cache;
_Field_range_( 0, 4294967295 ) std::uint32_t files_cache;
_Field_range_( 0, 32767 ) std::int16_t indent; // 0 for the root item, 1 for its children, and so on.
//C4820: 'VISIBLEINFO' : '1' bytes padding added after data member 'VISIBLEINFO::isExpanded'
bool isExpanded : 1; // Whether item is expanded.
};

Expand Down Expand Up @@ -126,6 +127,7 @@ class CTreeListItem : public COwnerDrawnListItem {

_Pre_satisfies_( this->m_vi._Myptr != nullptr )
void SortChildren ( );

_Pre_satisfies_( this->m_parent != NULL )
bool HasSiblings ( ) const;

Expand Down Expand Up @@ -157,7 +159,7 @@ class CTreeListItem : public COwnerDrawnListItem {
static CTreeListControl* GetTreeListControl( );

public:
CTreeListItem* m_parent;
const CTreeListItem* m_parent;

// Data needed to display the item.
mutable std::unique_ptr<VISIBLEINFO> m_vi = nullptr;
Expand Down Expand Up @@ -256,6 +258,7 @@ class CTreeListControl final : public COwnerDrawnListCtrl {
CBitmap m_bmNodes0; // The bitmaps needed to draw the treecontrol-like branches
CBitmap m_bmNodes1; // The same bitmaps with stripe-background color
INT m_lButtonDownItem; // Set in OnLButtonDown(). -1 if not item hit.
//C4820: 'CTreeListControl' : '3' bytes padding added after data member 'CTreeListControl::m_lButtonDownOnPlusMinusRect'
bool m_lButtonDownOnPlusMinusRect; // Set in OnLButtonDown(). True, if plus-minus-rect hit.


Expand Down
49 changes: 37 additions & 12 deletions WinDirStat/windirstat/colorbutton.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,60 +40,81 @@
class CColorButton final : public CButton {
public:

CColorButton( ) { }
CColorButton( ) = default;
CColorButton& operator=( const CColorButton& in ) = delete;
CColorButton( const CColorButton& in ) = delete;
// The color preview is an own little child window of the button.
class CPreview final : public CWnd {
public:
COLORREF m_color;

CPreview& operator=( const CPreview& in ) = delete;
CPreview( const CPreview& in ) = delete;

CPreview( ) : m_color( 0 ) { }
CPreview( ) : m_color{ 0u } { }
void SetColor( _In_ const COLORREF color ) {
m_color = color;
if ( IsWindow( m_hWnd ) ) {
InvalidateRect( NULL );
//"Return value: If the function succeeds, the return value is nonzero. If the function fails, the return value is zero."
VERIFY( ::InvalidateRect( m_hWnd, NULL, TRUE ) );
//InvalidateRect( NULL );
}
}
COLORREF m_color;

DECLARE_MESSAGE_MAP()
afx_msg void OnPaint( ) {
CPaintDC dc( this );
RECT rc;
GetClientRect( &rc );
ASSERT( ::IsWindow( m_hWnd ) );

//"Return value: If the function succeeds, the return value is nonzero. If the function fails, the return value is zero. To get extended error information, call GetLastError."
VERIFY( ::GetClientRect( m_hWnd, &rc ) );
//GetClientRect( &rc );

VERIFY( dc.DrawEdge( &rc, EDGE_BUMP, BF_RECT bitor BF_ADJUST ) );

auto color = m_color;
auto color_scope_holder = m_color;
if ( ( GetParent( )->GetStyle( ) bitand WS_DISABLED ) != 0 ) {
color = GetSysColor( COLOR_BTNFACE );
color_scope_holder = GetSysColor( COLOR_BTNFACE );
}
const auto color = color_scope_holder;
dc.FillSolidRect( &rc, color );
}

afx_msg void OnLButtonDown( UINT nFlags, CPoint point ) {
ClientToScreen( &point );
GetParent( )->ScreenToClient( &point );
ASSERT( ::IsWindow( m_hWnd ) );
//"Return value: If the function succeeds, the return value is nonzero. If the function fails, the return value is zero."
VERIFY( ::ClientToScreen( m_hWnd, &point ) );
//ClientToScreen( &point );

const auto this_parent = GetParent( );

//"Return value: If the function succeeds, the return value is nonzero. If the function fails, the return value is zero."
VERIFY( ::ScreenToClient( this_parent->m_hWnd, &point ) );
//this_parent->ScreenToClient( &point );

TRACE( _T( "User clicked x:%ld, y:%ld! Sending WM_LBUTTONDOWN!\r\n" ), point.x, point.y );

/*
_AFXWIN_INLINE CWnd* CWnd::GetParent() const
{ ASSERT(::IsWindow(m_hWnd)); return CWnd::FromHandle(::GetParent(m_hWnd)); }
*/
GetParent( )->SendMessageW( WM_LBUTTONDOWN, nFlags, MAKELPARAM( point.x, point.y ) );
this_parent->SendMessageW( WM_LBUTTONDOWN, nFlags, MAKELPARAM( point.x, point.y ) );
}
};

//C4820: 'CColorButton::CPreview' : '4' bytes padding added after data member 'CColorButton::CPreview::m_color'

CPreview m_preview;

protected:
DECLARE_MESSAGE_MAP()
afx_msg void OnPaint( ) {
if ( m_preview.m_hWnd == NULL ) {
RECT rc;
GetClientRect( &rc );
//"Return value: If the function succeeds, the return value is nonzero. If the function fails, the return value is zero. To get extended error information, call GetLastError."
VERIFY( ::GetClientRect( m_hWnd, &rc ) );
//GetClientRect( &rc );

rc.right = rc.left + ( rc.right - rc.left ) / 3;
//rc.DeflateRect( 4, 4 );
Expand Down Expand Up @@ -128,7 +149,11 @@ class CColorButton final : public CButton {

afx_msg void OnEnable( const BOOL bEnable ) {
if ( IsWindow( m_preview.m_hWnd ) ) {
m_preview.InvalidateRect( NULL );

//"Return value: If the function succeeds, the return value is nonzero. If the function fails, the return value is zero."
VERIFY( ::InvalidateRect( m_preview.m_hWnd, NULL, TRUE ) );
//m_preview.InvalidateRect( NULL );

}
CButton::OnEnable( bEnable );
}
Expand Down

0 comments on commit 2c0a187

Please sign in to comment.