Update way in which column data is retrieved and set

Previously, column data was retrieved on a background thread. That
thread would iterate through each item in the listview, retrieve the
column data then insert it into the listview.

Now, the column data is still retrieved in a background thread, but
items are only queued to the thread as necessary (e.g. as items are
scrolled into view). Additionally, the background thread no longer
interacts with the listview at all.

The updated implementation has a number of advantages:

- As mentioned above, items are only queued when necessary. This means
  that not all the results have to be loaded at once.
- Because the background thread no longer touches the UI, it's
  potentially easier to lock its access to shared data. Previously,
  acquiring a lock in the background thread and then attempting to lock
  the main thread could result in a deadlock. The background thread
  would try to update the UI, which would require work to be done on the
  main thread, but the main thread would be locked waiting on the
  background thread.
  Note that this locking wasn't actually added. It wouldn't have been
  safe to do so, for the above reason.
- Since the background thread no longer touches the UI, it may help to
  cut down the crashes that can occur when navigating to a different
  folder while column results are being filled. Specifically, the first
  thing the previous background thread did for an item was to look that
  item up in the listview. That could fail (if a navigation had occurred
  since the item was queued), potentially causing a crash.
- Because the previous implementation updated the UI directly, it was
  possible for a UI update to be done after a navigation had occurred.
  That is, column data for an item in a previous folder would be
  inserted into the listview for the current folder.
  In the new implementation, this isn't possible. Results are posted
  back to the main thread and the main thread will ignore results for
  previous folders.
- Both the listview item index and column index are looked up at the
  point where the column data is set (in the main thread). Previously,
  the item index and column index were determined before the column text
  was retrieved. This meant it was possible for the indexes to change
  before the text had been set.
- Parts of the implementation are simplified, since a library is used
  for the thread pool functionality (rather than custom code).
- It's trivial to increase the number of threads used in the thread pool
  (though that may not be of too much use in this specific case, as the
  work is IO bound).

However, there is still at least one remaining issue. The background
thread shares state with the main thread (the same object is used for
both). At the moment, there's no locking that's done. That means that if
a navigation occurs while column data is being retrieved, or a file is
modified or removed, the application could still crash.

Adding locks to the appropriate code may be easier now that the
background thread doesn't touch the UI, but ultimately, there's still
too much state that's being shared.

Another potential issue with the new implementation is that the library
being used for the thread pool functionality
(https://github.com/vit-vit/CTPL) hasn't been updated since 2015. It may
not receive any future bug fixes. The library is fairly simple though
and could potentially be replaced if necessary.

Any replacement would need the following functionality:

- The ability to control the number of threads in the pool.
- The ability to push tasks to a queue.
- The ability to clear the queue.
- The ability to retrieve results using std::future.

Author: derceg

Explorer++/Explorer++/Explorer++.vcxproj

@@ -346,6 +346,7 @@
     <ClCompile Include="ShellBrowser\iFolderView.cpp" />
     <ClCompile Include="ShellBrowser\iPathManager.cpp" />
     <ClCompile Include="ShellBrowser\iShellBrowser.cpp" />
+    <ClCompile Include="ShellBrowser\ListView.cpp" />
     <ClCompile Include="ShellBrowser\SortManager.cpp" />
     <ClCompile Include="ShellContextMenuHandler.cpp" />
     <ClCompile Include="SplitFileDialog.cpp" />

Explorer++/Explorer++/Explorer++.vcxproj.filters

@@ -232,6 +232,9 @@
     <ClCompile Include="ShellBrowser\SortManager.cpp">
       <Filter>ShellBrowser</Filter>
     </ClCompile>
+    <ClCompile Include="ShellBrowser\ListView.cpp">
+      <Filter>ShellBrowser</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="ApplicationToolbar.h">

Explorer++/Explorer++/ShellBrowser/BrowsingHandler.cpp

@@ -63,11 +63,13 @@ HRESULT CShellBrowser::BrowseFolder(LPCITEMIDLIST pidlDirectory,UINT wFlags)
 
 	EmptyIconFinderQueue();
 	EmptyThumbnailsQueue();
-	EmptyColumnQueue();
 	EmptyFolderQueue();
 
 	/* TODO: Wait for any background threads to finish processing. */
 
+	m_columnThreadPool.clear_queue();
+	m_columnResults.clear();
+
 	EnterCriticalSection(&m_csDirectoryAltered);
 	m_FilesAdded.clear();
 	m_FileSelectionList.clear();
@@ -268,8 +270,6 @@ void inline CShellBrowser::InsertAwaitingItems(BOOL bInsertIntoGroup)
 		TCHAR szDrive[MAX_PATH];
 		BOOL bNetworkRemovable = FALSE;
 
-		QueueUserAPC(SetAllColumnDataAPC,m_hThread,(ULONG_PTR)this);
-
 		StringCchCopy(szDrive,SIZEOF_ARRAY(szDrive),m_CurDir);
 		PathStripToRoot(szDrive);
 
@@ -567,7 +567,6 @@ HRESULT inline CShellBrowser::AddItemInternal(int iItemIndex,int iItemId,BOOL bP
 
 	m_AwaitingAddList.push_back(AwaitingAdd);
 
-	AddToColumnQueue(AwaitingAdd.iItem);
 	AddToFolderQueue(AwaitingAdd.iItem);
 
 	return S_OK;

Explorer++/Explorer++/ShellBrowser/ColumnManager.cpp

@@ -35,63 +35,6 @@
 
 BOOL GetPrinterStatusDescription(DWORD dwStatus, TCHAR *szStatus, size_t cchMax);
 
-/* Queueing model:
-When first browsing into a folder, all items in queue
-are cleared.
-Then, all new items are added.
-Finally, the APC is queued to the worker thread.
-
-The worker thread will process items, removing items
-from the queue as it does. Interlocking is required
-between queue removal and emptying the queue.
-
-Folder sizes are NOT calculated here. They are done
-within a separate thread called from the main thread. */
-void CShellBrowser::AddToColumnQueue(int iItem)
-{
-	m_pColumnInfoList.push_back(iItem);
-}
-
-void CShellBrowser::EmptyColumnQueue(void)
-{
-	EnterCriticalSection(&m_column_cs);
-
-	m_pColumnInfoList.clear();
-
-	LeaveCriticalSection(&m_column_cs);
-}
-
-BOOL CShellBrowser::RemoveFromColumnQueue(int *iItem)
-{
-	BOOL bQueueNotEmpty;
-
-	EnterCriticalSection(&m_column_cs);
-
-	if(m_pColumnInfoList.empty() == TRUE)
-	{
-		SetEvent(m_hColumnQueueEvent);
-		bQueueNotEmpty = FALSE;
-	}
-	else
-	{
-		std::list<int>::iterator itr;
-
-		itr = m_pColumnInfoList.begin();
-
-		*iItem = *itr;
-
-		ResetEvent(m_hColumnQueueEvent);
-
-		m_pColumnInfoList.erase(itr);
-
-		bQueueNotEmpty = TRUE;
-	}
-
-	LeaveCriticalSection(&m_column_cs);
-
-	return bQueueNotEmpty;
-}
-
 void CShellBrowser::AddToFolderQueue(int iItem)
 {
 	m_pFolderInfoList.push_back(iItem);
@@ -223,33 +166,122 @@ int CShellBrowser::SetAllFolderSizeColumnData(void)
 	return 0;
 }
 
-void CALLBACK SetAllColumnDataAPC(ULONG_PTR dwParam)
+void CShellBrowser::QueueColumnTask(int itemInternalIndex, int columnIndex)
 {
-	CShellBrowser *pShellBrowser = reinterpret_cast<CShellBrowser *>(dwParam);
-	pShellBrowser->SetAllColumnText();
+	auto columnID = GetColumnIdByIndex(columnIndex);
+
+	if (!columnID)
+	{
+		return;
+	}
+
+	int columnResultID = m_columnResultIDCounter++;
+
+	auto result = m_columnThreadPool.push([this, columnResultID, columnID, itemInternalIndex](int id) {
+		UNREFERENCED_PARAMETER(id);
+
+		return this->GetColumnTextAsync(columnResultID, *columnID, itemInternalIndex);
+	});
+
+	// The function call above might finish before this line runs,
+	// but that doesn't matter, as the results won't be processed
+	// until a message posted to the main thread has been handled
+	// (which can only occur after this function has returned).
+	m_columnResults.insert({ columnResultID, std::move(result) });
 }
 
-void CShellBrowser::SetAllColumnText(void)
+CShellBrowser::ColumnResult_t CShellBrowser::GetColumnTextAsync(int columnResultId, unsigned int ColumnID, int InternalIndex) const
 {
-	int ItemIndex;
-	BOOL QueueNotEmpty = RemoveFromColumnQueue(&ItemIndex);
+	std::wstring columnText = GetColumnText(ColumnID, InternalIndex);
 
-	while(QueueNotEmpty)
+	// This message may be delivered before this function has returned.
+	// That doesn't actually matter, since the message handler will
+	// simply wait for the result to be returned.
+	PostMessage(m_hListView, WM_APP_COLUMN_RESULT_READY, columnResultId, 0);
+
+	ColumnResult_t result;
+	result.itemInternalIndex = InternalIndex;
+	result.columnID = ColumnID;
+	result.columnText = columnText;
+
+	return result;
+}
+
+void CShellBrowser::ProcessColumnResult(int columnResultId)
+{
+	auto itr = m_columnResults.find(columnResultId);
+
+	if (itr == m_columnResults.end())
+	{
+		// This result is for a previous folder. It can be ignored.
+		return;
+	}
+
+	auto result = itr->second.get();
+
+	auto index = LocateItemByInternalIndex(result.itemInternalIndex);
+
+	if (!index)
+	{
+		// This is a valid state. The item may simply have been deleted.
+		return;
+	}
+
+	auto columnIndex = GetColumnIndexById(result.columnID);
+
+	if (!columnIndex)
 	{
-		int iColumnIndex = 0;
+		// This is also a valid state. The column may have been removed.
+		return;
+	}
+
+	auto columnText = std::make_unique<TCHAR[]>(result.columnText.size() + 1);
+	StringCchCopy(columnText.get(), result.columnText.size() + 1, result.columnText.c_str());
+	ListView_SetItemText(m_hListView, *index, *columnIndex, columnText.get());
 
-		for(auto itr = m_pActiveColumnList->begin();itr != m_pActiveColumnList->end();itr++)
+	m_columnResults.erase(itr);
+}
+
+boost::optional<int> CShellBrowser::GetColumnIndexById(unsigned int id) const
+{
+	HWND header = ListView_GetHeader(m_hListView);
+
+	int numItems = Header_GetItemCount(header);
+
+	for (int i = 0; i < numItems; i++)
+	{
+		HDITEM hdItem;
+		hdItem.mask = HDI_LPARAM;
+		BOOL res = Header_GetItem(header, i, &hdItem);
+
+		if (!res)
 		{
-			if(itr->bChecked)
-			{
-				SetColumnText(itr->id,ItemIndex,iColumnIndex++);
-			}
+			continue;
 		}
 
-		QueueNotEmpty = RemoveFromColumnQueue(&ItemIndex);
+		if (hdItem.lParam == id)
+		{
+			return i;
+		}
 	}
 
-	ApplyHeaderSortArrow();
+	return boost::none;
+}
+
+boost::optional<unsigned int> CShellBrowser::GetColumnIdByIndex(int index) const
+{
+	HWND hHeader = ListView_GetHeader(m_hListView);
+
+	HDITEM hdItem;
+	hdItem.mask = HDI_LPARAM;
+	BOOL res = Header_GetItem(hHeader, index, &hdItem);
+
+	if (!res)
+	{
+		return boost::none;
+	}
+
+	return hdItem.lParam;
 }
 
 void CShellBrowser::SetColumnText(UINT ColumnID,int ItemIndex,int ColumnIndex)

Explorer++/Explorer++/ShellBrowser/ListView.cpp

@@ -0,0 +1,33 @@
+/******************************************************************
+*
+* Project: Explorer++
+* File: ListView.cpp
+* License: GPL - See LICENSE in the top level directory
+*
+* Written by David Erceg
+* www.explorerplusplus.com
+*
+*****************************************************************/
+
+#include "stdafx.h"
+#include "iShellView.h"
+
+LRESULT CALLBACK CShellBrowser::ListViewProcStub(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam, UINT_PTR uIdSubclass, DWORD_PTR dwRefData)
+{
+	UNREFERENCED_PARAMETER(uIdSubclass);
+
+	CShellBrowser *shellBrowser = reinterpret_cast<CShellBrowser *>(dwRefData);
+	return shellBrowser->ListViewProc(hwnd, uMsg, wParam, lParam);
+}
+
+LRESULT CALLBACK CShellBrowser::ListViewProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
+{
+	switch (uMsg)
+	{
+	case WM_APP_COLUMN_RESULT_READY:
+		ProcessColumnResult(static_cast<int>(wParam));
+		break;
+	}
+
+	return DefSubclassProc(hwnd, uMsg, wParam, lParam);
+}

Explorer++/Explorer++/ShellBrowser/iFolderView.cpp

@@ -77,7 +77,9 @@ m_hOwner(hOwner),
 m_hListView(hListView),
 m_hThread(hIconThread),
 m_hFolderSizeThread(hFolderSizeThread),
-m_itemIDCounter(0)
+m_itemIDCounter(0),
+m_columnThreadPool(1),
+m_columnResultIDCounter(0)
 {
 	m_iRefCount = 1;
 
@@ -126,7 +128,6 @@ m_itemIDCounter(0)
 	m_nAwaitingAdd = 0;
 
 	InitializeCriticalSection(&m_csDirectoryAltered);
-	InitializeCriticalSection(&m_column_cs);
 	InitializeCriticalSection(&m_folder_cs);
 
 	if(!g_bcsThumbnailInitialized)
@@ -148,17 +149,24 @@ m_itemIDCounter(0)
 	}
 
 	m_hIconEvent = CreateEvent(NULL,TRUE,TRUE,NULL);
-	m_hColumnQueueEvent = CreateEvent(NULL,TRUE,TRUE,NULL);
 	m_hFolderQueueEvent = CreateEvent(NULL,TRUE,TRUE,NULL);
+
+	m_ListViewSubclassed = SetWindowSubclass(hListView, ListViewProcStub, LISTVIEW_SUBCLASS_ID, reinterpret_cast<DWORD_PTR>(this));
 }
 
 CShellBrowser::~CShellBrowser()
 {
+	if (m_ListViewSubclassed)
+	{
+		RemoveWindowSubclass(m_hListView, ListViewProcStub, LISTVIEW_SUBCLASS_ID);
+	}
+
 	EmptyIconFinderQueue();
 	EmptyThumbnailsQueue();
-	EmptyColumnQueue();
 	EmptyFolderQueue();
 
+	m_columnThreadPool.clear_queue();
+
 	/* Wait for any current processing to finish. */
 	WaitForSingleObject(m_hIconEvent,INFINITE);
 
@@ -167,7 +175,6 @@ CShellBrowser::~CShellBrowser()
 	m_pDragSourceHelper->Release();
 
 	DeleteCriticalSection(&m_folder_cs);
-	DeleteCriticalSection(&m_column_cs);
 	DeleteCriticalSection(&m_csDirectoryAltered);
 
 	int nItems = ListView_GetItemCount(m_hListView);
@@ -229,13 +236,6 @@ void CShellBrowser::SetCurrentViewMode(UINT ViewMode)
 	{
 		case VM_DETAILS:
 			{
-				int i = 0;
-
-				for(i = 0;i < m_nTotalItems;i++)
-					AddToColumnQueue(i);
-
-				QueueUserAPC(SetAllColumnDataAPC,m_hThread,(ULONG_PTR)this);
-
 				if(m_bShowFolderSizes)
 					QueueUserAPC(SetAllFolderSizeColumnDataAPC,m_hFolderSizeThread,(ULONG_PTR)this);
 			}

Explorer++/Explorer++/ShellBrowser/iShellBrowser.cpp

@@ -265,6 +265,21 @@ int CShellBrowser::LocateFileItemInternalIndex(const TCHAR *szFileName) const
 	return -1;
 }
 
+boost::optional<int> CShellBrowser::LocateItemByInternalIndex(int internalIndex) const
+{
+	LVFINDINFO lvfi;
+	lvfi.flags = LVFI_PARAM;
+	lvfi.lParam = internalIndex;
+	int item = ListView_FindItem(m_hListView, -1, &lvfi);
+
+	if (item == -1)
+	{
+		return boost::none;
+	}
+
+	return item;
+}
+
 DWORD CShellBrowser::QueryFileAttributes(int iItem) const
 {
 	LVITEM lvItem;
@@ -355,6 +370,11 @@ void CShellBrowser::OnListViewGetDisplayInfo(LPARAM lParam)
 		return;
 	}
 
+	if (m_ViewMode == VM_DETAILS && (plvItem->mask & LVIF_TEXT) == LVIF_TEXT)
+	{
+		QueueColumnTask(static_cast<int>(plvItem->lParam), plvItem->iSubItem);
+	}
+
 	if((plvItem->mask & LVIF_IMAGE) == LVIF_IMAGE)
 	{
 		if((m_fileInfoMap.at(static_cast<int>(plvItem->lParam)).dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != FILE_ATTRIBUTE_DIRECTORY)
@@ -1262,7 +1282,6 @@ void CShellBrowser::SetTerminationStatus(void)
 {
 	EmptyIconFinderQueue();
 	EmptyThumbnailsQueue();
-	EmptyColumnQueue();
 	EmptyFolderQueue();
 	m_bNotifiedOfTermination = TRUE;
 }