Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the Disconnect Network Drive Dialog #858

Closed
wants to merge 7 commits into from

Conversation

@JaredSmudde
Copy link
Contributor

JaredSmudde commented Sep 14, 2018

This is the version of the patch by Kamil that had all his fixes. The patch has sat for long enough.

A few minor things:

  • The dialog lacks the dialog to tell a user that there are no drives to disconnect if that's the case. I've tried to work on it but no luck.
  • It doesn't show drives under Server 2003 but that's for a rainy day.

JIRA issues: CORE-13516, CORE-13518.

Copy link
Member

binarymaster left a comment

Indentation problems should be fixed, replace tabs with 4 spaces.

Column name strings should be loaded from resources.

dll/win32/mpr/wnet.c Outdated Show resolved Hide resolved
dll/win32/mpr/wnet.c Outdated Show resolved Hide resolved
dll/win32/mpr/wnet.c Outdated Show resolved Hide resolved
return;

column.mask = LVCF_WIDTH | LVCF_TEXT;
column.pszText = L"Drive Letter";

This comment has been minimized.

Copy link
@binarymaster

binarymaster Sep 14, 2018

Member

This string should be localized.

ListView_InsertColumn(hListView, 0, &column);

column.mask = LVCF_WIDTH | LVCF_TEXT;
column.pszText = L"Network Path";

This comment has been minimized.

Copy link
@binarymaster

binarymaster Sep 14, 2018

Member

And this one too.

lpRes = pBuff;
dAllocSize = dSize;

continue;

This comment has been minimized.

Copy link
@HeisSpiter

HeisSpiter Sep 14, 2018

Member

This looks rather suspicious and might explain why it doesn't work on W2K3.

Can you try something like: https://git.reactos.org/?p=reactos.git;a=blob;f=base/applications/network/net/cmdUse.c;hb=HEAD ?

I'd be interested to know whether it work better on W2K3.

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 15, 2018

Author Contributor

If I remember correctly, this is where I got my code from.

This comment has been minimized.

Copy link
@HeisSpiter

HeisSpiter Sep 15, 2018

Member

Likely not. They absolutely don't match. Could you rework a bit that loop? This continue is the best way to loose data.

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 15, 2018

Author Contributor

Alright. I tried the code from net use and it still didn't work under 2003.

This comment has been minimized.

Copy link
@HeisSpiter

HeisSpiter Sep 15, 2018

Member

Well, it's at least better than current code ;-).

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 15, 2018

Author Contributor

The only downside is that the listview items don't display properly :)

}
else
{
ERR("WNetEnumResource failed with error: 0x%08x\n", dRet);

This comment has been minimized.

Copy link
@HeisSpiter

HeisSpiter Sep 14, 2018

Member

Please don't make this case noisy. It is expected that WNetEnumResources fails (for instance when there's nothing - left - to enumerate).

memset(&item, 0, sizeof(item));
item.mask = LVIF_TEXT | LVIF_IMAGE;
item.iImage = 0;
item.pszText = lpCur->lpLocalName;

This comment has been minimized.

Copy link
@HeisSpiter

HeisSpiter Sep 14, 2018

Member

For the record, there are cases where lpLocalName can be NULL. Perhaps you want not to copy it to your list, then?

disconn_dlg.cbStructure = sizeof(disconn_dlg);
disconn_dlg.hwndOwner = hwnd;

return WNetDisconnectDialog1W(&disconn_dlg);

This comment has been minimized.

Copy link
@HeisSpiter

HeisSpiter Sep 14, 2018

Member

Please surround these changes with #ifdef REACTOS, that's Wine code.

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 14, 2018

Author Contributor

Hopefully my lastest commit for MPR addresses it.

else
ret = ERROR_EXTENDED_ERROR;
TRACE("WNetDisconnectDialog1W Returned %d\n", ret);
return ret;

This comment has been minimized.

Copy link
@HeisSpiter

HeisSpiter Sep 14, 2018

Member

Please surround these changes with #ifdef REACTOS, that's Wine code.

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 14, 2018

Author Contributor

Same as above.

else if (hr == ERROR_NOT_ENOUGH_MEMORY)
ret = ERROR_NOT_ENOUGH_MEMORY;
else
ret = ERROR_EXTENDED_ERROR;

This comment has been minimized.

Copy link
@HeisSpiter

HeisSpiter Sep 14, 2018

Member

I'm not sure what you're trying to achieve here?

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 14, 2018

Author Contributor

I've removed it all and made it so simple in the latest commit.

case DLL_PROCESS_ATTACH:
iccx.dwSize = sizeof(INITCOMMONCONTROLSEX);
iccx.dwICC = ICC_STANDARD_CLASSES | ICC_LISTVIEW_CLASSES;
InitCommonControlsEx(&iccx);

This comment has been minimized.

Copy link
@ThFabba

ThFabba Sep 14, 2018

Member

Calling a comctl32 function from DllMain seems problematic. Is there an alternative place you could move it to?
(and yes, I know we have other dlls/cpls that do this. Doesn't mean it's a good idea)

This comment has been minimized.

Copy link
@learn-more

learn-more Sep 14, 2018

Member

Generally, yes.
But for this specific case (InitCommonControls[Ex]) it is just an empty function, used to force linking to comctl32.

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 15, 2018

Author Contributor

Should I leave it in place or did you all want me to move it?

dll/shellext/netplwiz/SHDisconnectNetDrives.c Outdated Show resolved Hide resolved
dll/shellext/netplwiz/netplwiz.c Outdated Show resolved Hide resolved
dll/shellext/netplwiz/SHDisconnectNetDrives.c Outdated Show resolved Hide resolved
HANDLE hEnum;
LPNETRESOURCE lpRes;
DWORD dAllocSize = 0x1000;
DWORD dCount;

This comment has been minimized.

Copy link
@learn-more

learn-more Sep 14, 2018

Member

d prefix?
Also, move this next to the dSize below.

This comment has been minimized.

Copy link
@HBelusca

HBelusca Sep 14, 2018

Contributor

(so it would be instead "dw" prefix).

dll/shellext/netplwiz/SHDisconnectNetDrives.c Outdated Show resolved Hide resolved

static INT_PTR CALLBACK DisconnectDlgProc(HWND hDlg, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
HICON hIcon = NULL, hIconSm = NULL;

This comment has been minimized.

Copy link
@learn-more

learn-more Sep 14, 2018

Member

No need to initialize the icons.

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 14, 2018

Author Contributor

Once I removed the initialization, GCC complains about how hIcon and hIconSm is not declared.

This comment has been minimized.

Copy link
@binarymaster

binarymaster Sep 14, 2018

Member

Are you removing entire line?

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 14, 2018

Author Contributor

Yes. But I probably only need to remove = NULL?

This comment has been minimized.

Copy link
@binarymaster

binarymaster Sep 14, 2018

Member

Variable is initialized with NULL, so obviously.

static INT_PTR CALLBACK DisconnectDlgProc(HWND hDlg, UINT uMsg, WPARAM wParam, LPARAM lParam)
{
HICON hIcon = NULL, hIconSm = NULL;
HWND hOkbutton = GetDlgItem(hDlg, ID_OK);

This comment has been minimized.

Copy link
@learn-more

learn-more Sep 14, 2018

Member

Please move initialization of this to the WM_INITDIALOG case, that is the only place where it is used.

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 14, 2018

Author Contributor

Same here plus I get a warning about mixing case and declarations.

dll/shellext/netplwiz/netplwiz.spec Outdated Show resolved Hide resolved
dll/win32/mpr/CMakeLists.txt Show resolved Hide resolved
dll/shellext/netplwiz/netplwiz.c Outdated Show resolved Hide resolved
dll/shellext/netplwiz/netplwiz.c Outdated Show resolved Hide resolved
dll/shellext/netplwiz/resource.h Outdated Show resolved Hide resolved
#ifdef __REACTOS__
HRESULT WINAPI SHDisconnectNetDrives(PVOID Unused);

return SHDisconnectNetDrives(NULL);

This comment has been minimized.

Copy link
@HBelusca

HBelusca Sep 14, 2018

Contributor

What should be done here instead is a LoadLibrary of the dll that exports this API, followed by a GetProcAddress to get the function pointer. At the end, release the loaded dll with FreeLibrary.

This comment has been minimized.

Copy link
@JaredSmudde

JaredSmudde Sep 15, 2018

Author Contributor

Before I make another commit, does this look like what you wanted?

#ifdef __REACTOS__
    HMODULE hDll = LoadLibraryW(L"netplwiz.dll");
    static BOOL (WINAPI *pSHDisconnectNetDrives)(PVOID);
    pSHDisconnectNetDrives = GetProcAddress(hDll, "SHDisconnectNetDrives");
    
    return pSHDisconnectNetDrives(NULL);
    
    FreeLibrary(hDll);
 #endif

This comment has been minimized.

Copy link
@binarymaster

binarymaster Sep 15, 2018

Member

#ifdef __REACTOS__

Use markdown properly.

This comment has been minimized.

Copy link
@HBelusca

HBelusca Sep 15, 2018

Contributor

@JaredSmudde : almost: a more correct code would be (note that the FreeLibrary is before the 'return' instruction):

#ifdef __REACTOS__
    DWORD dwRet;
    HMODULE hDll = LoadLibraryW(L"netplwiz.dll");
    static BOOL (WINAPI *pSHDisconnectNetDrives)(PVOID);
    pSHDisconnectNetDrives = GetProcAddress(hDll, "SHDisconnectNetDrives");
    
    dwRet = pSHDisconnectNetDrives(NULL);
    
    FreeLibrary(hDll);
    return dwRet;
 #endif

This may be further improved by loading only once the dll and caching the imported procedure, etc...

This comment has been minimized.

Copy link
@ThFabba

ThFabba Sep 15, 2018

Member

Is delay load an option here?

This comment has been minimized.

Copy link
@learn-more

learn-more Sep 15, 2018

Member

Not until binutils is fixed :(

lpRes = pBuff;
dAllocSize = dSize;

continue;

This comment has been minimized.

Copy link
@HeisSpiter

HeisSpiter Sep 15, 2018

Member

Likely not. They absolutely don't match. Could you rework a bit that loop? This continue is the best way to loose data.

@tkreuzer tkreuzer added this to New PRs in ReactOS PRs Dec 11, 2018
@tkreuzer tkreuzer moved this from New PRs to Changes requested or WIP in ReactOS PRs Dec 16, 2018
Copy link
Contributor

SergeGautherie left a comment

Should this PR be upstreamed, before/after?


SetLastError(WN_NO_NETWORK);
return WN_NO_NETWORK;
#ifdef __REACTOS__

This comment has been minimized.

Copy link
@SergeGautherie

SergeGautherie Apr 2, 2019

Contributor

You should add an #else and leave WineSync code as is. At other places too.

This comment has been minimized.

Copy link
@SergeGautherie

SergeGautherie Apr 25, 2019

Contributor

Done in #1522.

@HBelusca

This comment has been minimized.

Copy link
Contributor

HBelusca commented Apr 21, 2019

@JaredSmudde : status of this PR?

@JaredSmudde

This comment has been minimized.

Copy link
Contributor Author

JaredSmudde commented Apr 21, 2019

It needs a rewrite. Unfortunately, time is something I've been short on.

@HBelusca HBelusca added the WIP label Apr 22, 2019
ReactOS PRs automation moved this from WIP / Waiting on contributor to Done Apr 22, 2019
@JaredSmudde

This comment has been minimized.

Copy link
Contributor Author

JaredSmudde commented Apr 22, 2019

I'm going to have to make a new PR. I've lost access to the branch I was using.

@learn-more

This comment has been minimized.

Copy link
Member

learn-more commented Apr 22, 2019

You can just check it out again from github?

@JaredSmudde

This comment has been minimized.

Copy link
Contributor Author

JaredSmudde commented Apr 22, 2019

My repo got deleted and I had to refork.

@HBelusca

This comment has been minimized.

Copy link
Contributor

HBelusca commented Apr 22, 2019

Lol, bad manipulation?

@JaredSmudde

This comment has been minimized.

Copy link
Contributor Author

JaredSmudde commented Apr 22, 2019

I somehow had messed up the repo so much I couldn't do anything with it. I couldn't pull or push so I found it easier to start over. On a side note, I'm on my 4th repo, I messed the other 3 up lol.

@SergeGautherie

This comment has been minimized.

Copy link
Contributor

SergeGautherie commented Apr 23, 2019

This PR reads 'from unknown repository' :-<
At least, commits are still there, so you should be able to reuse "them" in some way to create a new branch ..

@JaredSmudde

This comment has been minimized.

Copy link
Contributor Author

JaredSmudde commented Apr 23, 2019

I've got everything building now and tested. It's now live in #1522.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.