Zipfldr #451

learn-more · 2018-03-27T18:03:34Z

Initial implementation

This pr aims to cleanup this code, and get it in the tree.
Further improvements will be made later on, like fixing SHCreateFileExtractIconW, implementing a toolbar that will handle IExplorerCommand etc. CORE-14175

learn-more · 2018-03-27T18:04:56Z

dll/shellext/zipfldr/CMakeLists.txt

+set_cpp(WITH_RUNTIME WITH_EXCEPTIONS)
+if(NOT MSVC)
+    # HACK: this should be enabled globally!
+    add_compile_flags_language("-std=c++11" "CXX")


sorry thomas ;)

Isn't it? AFAIK rapps builds with this flag. It must be defined somewhere else.

This doesn't seem to indicate so: https://git.reactos.org/?p=reactos.git&a=search&h=HEAD&st=grep&s=c%2B%2B11

No, some stuff doesnt build like this so we keep pushing it back.

learn-more · 2018-03-27T18:06:26Z

dll/shellext/zipfldr/precomp.h

+#include <assert.h>
+
+EXTERN_C
+ULONG DbgPrint(PCSTR Format,...);


We should think about moving this somewhere shared...

Isn't it already in debug.h ?

boot/freeldr/freeldr/include/debug.h
That doesn't really help outside freeldr ;)

Ah, I thought it was also in sdk/include/reactos/debug.h.

gonzoMD · 2018-03-28T04:51:59Z

dll/shellext/zipfldr/precomp.h

+    return uRet;
+}
+
+#include "zlib_contrib\unzip.h"


Slash instead of backslash?

gonzoMD · 2018-03-28T04:55:04Z

dll/shellext/zipfldr/zipfldr.cpp

+CZipFldrModule gModule;
+
+
+#include "zlib_contrib\ioapi.h"


HBelusca

Any plan to put the zlib_contrib into sdk/lib/3rdparty (under the name "minizip")?

HBelusca · 2018-03-28T12:06:24Z

modules/rostests/apitests/shell32/IShellFolderViewCB.cpp

+#define SFVM_GET_WEBVIEW_TASKS        84 /* undocumented */
+#define SFVM_GET_WEBVIEW_THEME        86 /* undocumented */
+#define SFVM_GETDEFERREDVIEWSETTINGS  92 /* undocumented */
+#endif


Using some sdk/include/reactos/undocshell.h for these would be nice.

I prefer apitests to be stand-alone.
When we will use this in the code, I'll move this somewhere shared.

HBelusca · 2018-03-28T12:10:11Z

dll/shellext/zipfldr/CExplorerCommand.cpp

+            return E_POINTER;
+
+        if (pceltFetched)
+            *pceltFetched = 0;


Is it some kind of "FALSE"?

0 items fetched

HBelusca · 2018-03-28T12:11:38Z

dll/shellext/zipfldr/CZipEnumerator.hpp

+private:
+    CComPtr<IZip> spZip;
+    bool first;
+    CAtlList<CStringA> _returned;


You don't use the usual "m_Blah;" notation for class members here?

I will rename those to be consistent.

HBelusca · 2018-03-28T12:12:35Z

dll/shellext/zipfldr/CZipExtract.cpp

+
+                if (err < 0)
+                {
+                    __debugbreak();


Are these debugbreaks meant to be removed in the near future?

yeah, I probably should replace them with DPRINT1 before committing.

HBelusca · 2018-03-28T12:13:17Z

dll/shellext/zipfldr/CZipFolder.hpp

+{
+    { IDS_COL_NAME,     SHCOLSTATE_TYPE_STR | SHCOLSTATE_ONBYDEFAULT,   25, LVCFMT_LEFT },
+    { IDS_COL_TYPE,     SHCOLSTATE_TYPE_STR | SHCOLSTATE_ONBYDEFAULT,   20, LVCFMT_LEFT },
+    { IDS_COL_COMPRSIZE,SHCOLSTATE_TYPE_STR | SHCOLSTATE_ONBYDEFAULT,   10, LVCFMT_RIGHT },


/me suggests space after the first comma.

HBelusca · 2018-03-28T12:13:51Z

dll/shellext/zipfldr/CZipFolder.hpp

+        {
+            SHFILEINFOA shfi;
+            DWORD dwAttributes = isDir ? FILE_ATTRIBUTE_DIRECTORY : FILE_ATTRIBUTE_NORMAL;
+            ULONG_PTR firet = SHGetFileInfoA(zipEntry->Name, dwAttributes, &shfi, sizeof(shfi), SHGFI_USEFILEATTRIBUTES | SHGFI_TYPENAME);


FileInfo RETurn I guess, probably copied that from somwhere.

FileInfo-RET

HBelusca · 2018-03-28T12:14:37Z

dll/shellext/zipfldr/CZipFolder.hpp

+    STDMETHODIMP CreateViewObject(HWND hwndOwner, REFIID riid, LPVOID *ppvOut)
+    {
+        static const GUID UnknownIID = // {93F81976-6A0D-42C3-94DD-AA258A155470}
+        {0x93F81976, 0x6A0D, 0x42C3, {0x94, 0xDD, 0xAA, 0x25, 0x8A, 0x15, 0x54, 0x70}};


There's no existing standard name for this one?

Couldn't find it.

Apparently not. I did a quick googling and no one has a name for it.

sanchaez · 2018-03-28T20:52:51Z

dll/shellext/zipfldr/CFolderViewCB.cpp

+{
+public:
+
+    CFolderViewCB()


This is created for you.

Yeah I had some code in those that is gone now.

sanchaez · 2018-03-28T20:54:26Z

dll/shellext/zipfldr/CZipEnumerator.hpp

+{
+private:
+    CComPtr<IZip> spZip;
+    bool first;


bIsFirst
... but you don't use the usual MS style in this file, so whatever.

m_Zip, m_First, m_Returned

sanchaez · 2018-03-28T20:59:28Z

dll/shellext/zipfldr/CZipExtract.cpp

+        :uf(NULL)
+    {
+        m_Filename = Filename;
+        m_Directory = m_Filename;


Initializing those two members in initalizer list would be cleaner IMO.

m_Filename maybe, m_Directory no because I also modify it.

sanchaez · 2018-03-28T21:01:55Z

dll/shellext/zipfldr/CZipExtract.cpp

+        m_Filename = Filename;
+        m_Directory = m_Filename;
+        PWSTR Dir = m_Directory.GetBuffer();
+        PathRemoveExtensionW(Dir);


Using rvalue from GetBuffer() would be cleaner IMO.

I don't agree :)

sanchaez · 2018-03-28T21:02:32Z

dll/shellext/zipfldr/CZipExtract.cpp

+        m_Directory.ReleaseBuffer();
+    }
+
+    ~CZipExtract()


This is created for you. Maybe you wanted virtual dtor?

No, I should call Close() here, and warn that it wasnt closed.

sanchaez · 2018-03-28T21:03:11Z

dll/shellext/zipfldr/CZipExtract.cpp

+    {
+        if (uf)
+            unzClose(uf);
+        uf = NULL;


This would be better IMO.

if (uf) { unzClose(uf); uf = NULL; }

sanchaez · 2018-03-28T21:05:13Z

dll/shellext/zipfldr/CZipExtract.cpp

+            {
+            case IDYES: return Yes;
+            case IDYESALL: return YesToAll;
+            default:


Seems weird IMO but would work OK.
I would reorder the labels:

case IDCANCEL: return Cancel; case IDNO: //return No; default: return No;

sanchaez · 2018-03-28T21:11:45Z

dll/shellext/zipfldr/CZipExtract.cpp

+            HICON hIcon = LoadIcon(NULL, IDI_EXCLAMATION);
+            SendDlgItemMessage(IDC_EXCLAMATION_ICON, STM_SETICON, (WPARAM)hIcon);
+
+            /* Our CString does not support FormatMessage yet */


Consider implementing it then 😺

If I were to implement every feature that shit depends on, I can commit this in about 3 year.

sanchaez · 2018-03-28T21:16:32Z

dll/shellext/zipfldr/CZipExtract.cpp

+                    }
+                }
+
+


Double newline.

sanchaez · 2018-03-28T21:24:18Z

dll/shellext/zipfldr/zlib_contrib/MiniZip64_info.txt

@@ -0,0 +1,74 @@
+MiniZip - Copyright (c) 1998-2010 - by Gilles Vollant - version 1.1 64 bits from Mathias Svensson


I think zlib_contrib should be moved to zlib.

You mean, inside our existing zlib?

Being close to linux may not be bad. Some it is more easy if it's an bit more equivalent in direction linux as the "original", because later it's more easy to use with.. :)

@blackcrack: I have no idea what you are saying here.

@sanchaez why, is there something else that needs it?

Since they come from the same source, keeping them together makes it easier to sync.

@learn-more
Mark, do not mob around here otherwise I'll report you to Github.

@blackcrack : I don't understand your first message either...

ah this is a other musik :) Okey maybe a bit Awkwardly expressed :)
i have meant , because Linux use the zlib also , i have mentored to use the zlib too.
Makes may later more usable if we use it directly.. {less complications or so.. (it was just an assumption)} ;)
And GonzoMD, it must not be bad if you it not understand ;)

sanchaez · 2018-03-28T21:24:53Z

dll/win32/shell32/CDefView.cpp

@@ -400,7 +402,6 @@ HRESULT WINAPI CDefView::Initialize(IShellFolder *shellFolder)
 {
    m_pSFParent = shellFolder;
    shellFolder->QueryInterface(IID_PPV_ARG(IShellFolder2, &m_pSF2Parent));
-


Unrelated change.

Not really, I was experimenting with calls to the IShellFolderViewCB ;)

sanchaez · 2018-03-28T21:29:20Z

modules/rostests/apitests/shell32/SHCreateFileExtractIconW.cpp

                {
-                    ok(!memcmp(colorData, colorDataRef, colorSize), "Expected equal colorData for %S(%lx)\n", cur.Name, cur.dwFlags);
+                    /* Incase requested filetype does not match, the exe icon is not used! */


In case 😄

Justin Case 😆

sanchaez · 2018-03-28T21:30:15Z

modules/rostests/apitests/shell32/SHCreateFileExtractIconW.cpp

+        }
+
+        {
+            CComPtr<IUnknown> spUnkown, spUnknown2;


spUnknown

sanchaez · 2018-03-28T21:30:34Z

modules/rostests/apitests/shell32/SHCreateFileExtractIconW.cpp

+    for (size_t n = 0; n < _countof(InterfaceTests); ++n)
+    {
+        {
+            CComPtr<IUnknown> spUnkown;


spUnknown

sanchaez · 2018-03-28T21:46:05Z

dll/shellext/zipfldr/CExplorerCommand.cpp

+        if (m_bFirst && celt)
+        {
+            m_bFirst = false;
+            celt--;


Seems redundant.

Yeah Initially I wrote this so that it could handle multiple items, to be consistent i'll leave it in.

HBelusca · 2018-04-02T13:29:49Z

dll/win32/shell32/CDefView.cpp

@@ -402,6 +402,7 @@ HRESULT WINAPI CDefView::Initialize(IShellFolder *shellFolder)
 {
    m_pSFParent = shellFolder;
    shellFolder->QueryInterface(IID_PPV_ARG(IShellFolder2, &m_pSF2Parent));
+


What does this achieve? :P

It negates the removal of this line in an earlier commit.

learn-more · 2018-04-03T20:27:08Z

Moved minizip to zlib/contrib, and into a static library.

sanchaez · 2018-04-03T21:35:22Z

sdk/lib/3rdparty/zlib/CMakeLists.txt

@@ -21,9 +21,22 @@ list(APPEND SOURCE
    infback.c
    uncompr.c)

+list(APPEND MINIZIP_SOURCE


Maybe MiniZip deserves it's own CMakeLists.txt with a minizip target that could be included here via add_subdirectory()?
EDIT: refer to MiniZip Makefile or Automake/Autoconf.

Adding it inside an additional add_subdirectory would just be a pointless indirection.

What is there to see in the Makefile?

learn-more · 2018-04-04T07:40:56Z

sdk/lib/3rdparty/zlib/CMakeLists.txt

 if(CMAKE_CROSSCOMPILING)
    add_library(zlib ${SOURCE} ${SOLO_SOURCE})
    add_library(zlib_solo ${SOLO_SOURCE})
+    add_library(minizip ${MINIZIP_SOURCE})


missing dependency on xdk

CORE-14082

…ncorrect. CORE-14082

CORE-7684

Icon by Jared Smudde CORE-7684

CORE-7684

learn-more commented Mar 27, 2018

View reviewed changes

learn-more force-pushed the zipfldr branch from 2e763ee to 34cea89 Compare March 27, 2018 18:21

gonzoMD reviewed Mar 28, 2018

View reviewed changes

sanchaez self-requested a review March 28, 2018 10:27

HBelusca reviewed Mar 28, 2018

View reviewed changes

learn-more force-pushed the zipfldr branch from 34cea89 to 9068edc Compare March 28, 2018 18:23

sanchaez reviewed Mar 28, 2018

View reviewed changes

sanchaez approved these changes Mar 29, 2018

View reviewed changes

yagoulas approved these changes Mar 31, 2018

View reviewed changes

HBelusca reviewed Apr 2, 2018

View reviewed changes

HBelusca approved these changes Apr 2, 2018

View reviewed changes

learn-more force-pushed the zipfldr branch from dc3397a to 1949503 Compare April 3, 2018 20:22

sanchaez reviewed Apr 3, 2018

View reviewed changes

learn-more commented Apr 4, 2018

View reviewed changes

learn-more force-pushed the zipfldr branch from 1949503 to 006cc18 Compare April 4, 2018 17:50

sanchaez added the enhancement For PRs with an enhancement/new feature. label Apr 7, 2018

learn-more self-assigned this Apr 7, 2018

learn-more added 6 commits April 7, 2018 15:29

[SHELL32] Initial partial IShellFolderViewCB implementation.

0d07967

[SHELL32] Partially implement SHCreateFileExtractIconW

a693932

CORE-14082

[SHELL32_APITEST] Show that our current SHCreateFileExtractIconW is i…

7fe78f2

…ncorrect. CORE-14082

[ZLIB] Add minizip as library.

5100859

CORE-7684

[ZIPFLDR] Initial implementation.

c7e6a9d

Icon by Jared Smudde CORE-7684

[ZIPFLDR] Register the shell extension.

da6a46c

CORE-7684

learn-more force-pushed the zipfldr branch from 006cc18 to da6a46c Compare April 7, 2018 13:30

learn-more merged commit da6a46c into reactos:master Apr 7, 2018

learn-more deleted the zipfldr branch April 7, 2018 16:04

		@@ -0,0 +1,74 @@
		MiniZip - Copyright (c) 1998-2010 - by Gilles Vollant - version 1.1 64 bits from Mathias Svensson

		CZipFldrModule gModule;


		#include "zlib_contrib\ioapi.h"

Zipfldr #451

Zipfldr #451

Conversation

learn-more commented Mar 27, 2018 • edited

Choose a reason for hiding this comment

sanchaez Mar 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HBelusca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanchaez Mar 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanchaez Mar 28, 2018 • edited

Choose a reason for hiding this comment

sanchaez Mar 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackcrack Apr 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blackcrack Apr 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

learn-more commented Apr 3, 2018

sanchaez Apr 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

learn-more commented Mar 27, 2018 •

edited

sanchaez Mar 28, 2018 •

edited

sanchaez Mar 28, 2018 •

edited

sanchaez Mar 28, 2018 •

edited

sanchaez Mar 28, 2018 •

edited

blackcrack Apr 1, 2018 •

edited

blackcrack Apr 1, 2018 •

edited

sanchaez Apr 3, 2018 •

edited