Plugin for FILE_PACK_UNPACK cannot store persistent data

[adamalcorn]

Requests: [] plugin remains in memory till the source file is closed [] the PackFile() function has access to the original path of the source file

The PackFile() parameters include the source file from WinMerge and the desired final destination of the packed file. The file type I am trying to work with requires the original source file that was unpacked to access meta data that must be included in the newly packed file.

I tried to work around the problem by using pSubcode to store an id where the meta data could be stored in memory, but because the plugin is not persistent, InternalFinalConstructRelease() is called before the file is packed and therefore all of the meta data is lost.

My current implementation uses a global variable to store the meta data and just allows for a memory leak. It's not ideal, but it works.

[sdottaka]

I tried to work around the problem by using pSubcode to store an id where the meta data could be stored in memory, but because the plugin is not persistent, InternalFinalConstructRelease() is called before the file is packed and therefore all of the meta data is lost.

As shown below, I tried storing 1234 in the m_data member variable using the UnpackFile function of the DisplayXMLFiles plugin in the WinMerge repository, and referencing it with the PackFile function, but 1234 was stored. In other words, the m_data member variable remained without being released, and the problem could not be reproduced.

--- C:/Users/taka/AppData/Local/Temp/git-blob-a24592/WinMergeScript.h   Sun Jun  2 10:06:59 2024
+++ C:/dev/winmerge/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.h   Sun Jun  2 09:51:32 2024
@@ -22,6 +22,7 @@
    public IDispatchImpl<IWinMergeScript, &IID_IWinMergeScript, &LIBID_DisplayXMLFilesLib, 1, 0, CComTypeInfoHolderFileOnly>
 {
 public:
+   int m_data = 0;
    CWinMergeScript()
    {
    }

--- C:/Users/taka/AppData/Local/Temp/git-blob-a16228/WinMergeScript.cpp Sun Jun  2 10:05:58 2024
+++ C:/dev/winmerge/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.cpp Sun Jun  2 10:03:11 2024
@@ -336,11 +336,15 @@

    *pbChanged = VARIANT_TRUE;
    *pbSuccess = VARIANT_TRUE;
+   m_data = 1234;
    return S_OK;
 }

 STDMETHODIMP CWinMergeScript::PackFile(BSTR fileSrc, BSTR fileDst, VARIANT_BOOL *pbChanged, INT pSubcode, VARIANT_BOOL *pbSuccess)
 {
+   wchar_t buf[256];
+   wsprintfW(buf, L"m_data=%d", m_data);
+   MessageBox(nullptr, buf, nullptr, MB_OK);
    // always return error so the users knows we can not repack
    *pbChanged = VARIANT_FALSE;
    *pbSuccess = VARIANT_FALSE;

[adamalcorn]

@sdottaka I agree that storing a single value works correctly whether as a member variable as you demonstrated or through the pSubcode parameter. The issue I am having is: how to determine the original source file of the file being packed.

In my example below, the source file name is saved but when the Left file is changed the source file name has already been replaced by that of the Right file.

diff --git a/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.cpp b/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.cpp
index d7d8cc4e1..2a2635a2d 100644
--- a/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.cpp
+++ b/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.cpp
@@ -335,4 +335,8 @@ STDMETHODIMP CWinMergeScript::UnpackFile(BSTR fileSrc, BSTR fileDst, VARIANT_BOO
        fclose(oData.pOutput);

+       // pretend to fill m_metaData from the fileSrc by using the file source path as the "meta data"
+       (*pSubcode) = 1234;
+       wsprintfW(this->m_metaData, L"unpack_subcode=\"%d\" meta_data=\"%s\"", (*pSubcode), fileSrc);
+
        *pbChanged = VARIANT_TRUE;
        *pbSuccess = VARIANT_TRUE;
@@ -342,4 +346,8 @@ STDMETHODIMP CWinMergeScript::UnpackFile(BSTR fileSrc, BSTR fileDst, VARIANT_BOO
 STDMETHODIMP CWinMergeScript::PackFile(BSTR fileSrc, BSTR fileDst, VARIANT_BOOL *pbChanged, INT pSubcode, VARIANT_BOOL *pbSuccess)
 {
+       // pretend to pack m_metaData from the original file source into the fileDst by displaying a message
+       wchar_t packedData[2048] = { 0 };
+       wsprintfW(packedData, L"pack_subcode=\"%d\" %s", pSubcode, this->m_metaData);
+       MessageBox(nullptr, packedData, nullptr, MB_OK);
        // always return error so the users knows we can not repack
        *pbChanged = VARIANT_FALSE;
diff --git a/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.h b/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.h
index b3f19cd39..e0fb71628 100644
--- a/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.h
+++ b/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.h
@@ -23,6 +23,8 @@ class ATL_NO_VTABLE CWinMergeScript :
 {
 public:
+       wchar_t m_metaData[1024];
        CWinMergeScript()
        {
+               memset(this->m_metaData, 0, sizeof(this->m_metaData));
        }

In my first comment I mentioned using pSubcode as an id, which works to uniquely identify the correct meta data when packing. Your code helped me understand that the CWinMergeScript is persistent, so I added a destructor to see if I could reference count the objects...

My original code used InternalFinalConstructAddRef() and InternalFinalConstructRelease() to perform reference counting, but I think using the CWinMergeScript constructor and destructor could work! There are 2 objects being instantiated, 1 is destroyed when I close the comparison window and 1 is destroyed when I close the Select Files dialog. I'll do some more testing to make sure the objects are never destroyed while in-use but I think this has solved my problem.

I do suggest providing access to the original source file path in PackFile() but it looks like I have a workaround :)

Thank you for your help! Arigato gozaimasu -Adam

[adamalcorn]

Ultimately, the workaround still has an issue. I'm not sure why, but WinMerge always uses the first instance of the plugin object no matter how many times I open and close tabs within the application. Here is a brief log to demonstrate what I mean:

Plugin UID=1 Created
Plugin UID=2 Created
Plugin UID=1 attempt to Unpack 0x000001e2d191fb98 L"Z:\\work\\fj_caro_selection_script.ws6"
Plugin UID=1 attempt to Unpack 0x000001e2d191ed98 L"Z:\\work\\button_visibilty_linking_to_variable.ws6"
Plugin UID=1 attempt to Pack 0x000001e2d38ecd78 L"Z:\\work\\fj_caro_selection_script.ws6"
Plugin UID=1 attempt to Pack 0x000001e2d391ecb8 L"Z:\\work\\button_visibilty_linking_to_variable.ws6"
Plugin UID=2 Deleted
Plugin UID=3 Created
Plugin UID=1 attempt to Unpack 0x000001e2d394a358 L"Z:\\work\\dwt_ssc.ws6"
Plugin UID=1 attempt to Unpack 0x000001e2d6884a78 L"Z:\\work\\test.ws6"
Plugin UID=1 attempt to Unpack 0x000001e2d65fc2e8 L"Z:\\work\\DW & LO - 240206.wsl"
Plugin UID=1 attempt to Unpack 0x000001e2d394a358 L"Z:\\work\\DW & LO - 240206.ws6"
Plugin UID=1 attempt to Pack 0x000001e2d66f6d38 L"Z:\\work\\DW & LO - 240206.wsl"
Plugin UID=3 Deleted
Plugin UID=1 Unpacked 6 Packed 3
Plugin UID=1 Deleted

I'm glad I can just use the object destructor to cleanup memory I allocate for storing the original unpacked paths, but it does cause some limitations on when I can free memory that is no longer in use. I wish I had time to fork the repo and look into how the plugin is invoked. I suggest closing this issue for now and maybe just plan to enhance plugin support in the future.

[sdottaka]

An instance of the plugin is created for each WinMerge thread, and is reused in the same thread.

You can store meta data as follows:

--- C:/Users/sawan/AppData/Local/Temp/git-blob-a68048/WinMergeScript.h  Tue Jun  4 20:44:15 2024
+++ E:/dev/winmerge/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.h   Tue Jun  4 20:35:53 2024
@@ -9,2 +9,4 @@
 #include "typeinfoex.h"
+#include <vector>
+#include <string>

@@ -24,2 +26,3 @@
 public:
+   std::vector<std::wstring> m_metaDataSet;
    CWinMergeScript()

--- C:/Users/sawan/AppData/Local/Temp/git-blob-a66768/WinMergeScript.cpp    Tue Jun  4 20:44:15 2024
+++ E:/dev/winmerge/Plugins/src_VCPP/DisplayXMLFiles/WinMergeScript.cpp Tue Jun  4 20:41:20 2024
@@ -334,6 +334,16 @@
    fclose(pInput);
    fclose(oData.pOutput);

+   wchar_t buf2[1024];
+   wsprintfW(buf2, L"meta_data=\"%s\"", fileSrc);
+   std::wstring metaData(buf2);
+   auto it = std::find(m_metaDataSet.begin(), m_metaDataSet.end(), metaData);
+   if (it == m_metaDataSet.end())
+   {
+       m_metaDataSet.push_back(metaData);
+       it = m_metaDataSet.end() - 1;
+   }
+   (*pSubcode) = static_cast<int>(std::distance(m_metaDataSet.begin(), it));
    *pbChanged = VARIANT_TRUE;
    *pbSuccess = VARIANT_TRUE;
    return S_OK;
@@ -341,6 +351,7 @@

 STDMETHODIMP CWinMergeScript::PackFile(BSTR fileSrc, BSTR fileDst, VARIANT_BOOL *pbChanged, INT pSubcode, VARIANT_BOOL *pbSuccess)
 {
+   MessageBoxW(nullptr, m_metaDataSet[pSubcode].c_str(), nullptr, MB_OK);
    // always return error so the users knows we can not repack
    *pbChanged = VARIANT_FALSE;
    *pbSuccess = VARIANT_FALSE;

[adamalcorn]

That is essentially what I came up with as well 😄