Robert van Herk wrote:
-LPITEMIDLIST WINAPI SHBrowseForFolderA (LPBROWSEINFOA lpbi) +static INT_PTR BrowseW (LPBROWSEINFOW lpbi, DLGPROC dlgproc, enum browsemode aBrowseMode) +{
INT_PTR ret;browseMode = aBrowseMode;
^^^^^^^^^^
I may miss something here but this looks like a global variable and that would make these function theoretically thread unsafe. As long as only winecfg is going to use this functionality for the unix case it seems not a problem as there is probably not any chance that the native Windows version is called at that moment but this may of course change in the future.
Rolf Kalbermatter
Rolf Kalbermatter wrote:
Robert van Herk wrote:
-LPITEMIDLIST WINAPI SHBrowseForFolderA (LPBROWSEINFOA lpbi) +static INT_PTR BrowseW (LPBROWSEINFOW lpbi, DLGPROC dlgproc, enum browsemode aBrowseMode) +{
INT_PTR ret;browseMode = aBrowseMode;^^^^^^^^^^I may miss something here but this looks like a global variable and that would make these function theoretically thread unsafe. As long as only winecfg is going to use this functionality for the unix case it seems not a problem as there is probably not any chance that the native Windows version is called at that moment but this may of course change in the future.
I guess you are right, but the strange thing is that this was already the case in the original code. The hwnd of the tree view and the pointer to the browseinfo structure are copied into a global variable, so at least my code is not worse here :-).
By the way, isn't it so that a copy of shell32.dll gets loaded for each application anyway? If so, then the variable is at least application local, and, c'mon, who is going to show two modal browsing dialog boxes at the same time :-)?
Robert
Robert van Herk wrote:
I guess you are right, but the strange thing is that this was already the case in the original code. The hwnd of the tree view and the pointer to the browseinfo structure are copied into a global variable, so at least my code is not worse here :-).
Well, existing code doesn't necessarily need to be a nice and clean implementation as it may have been at some point more important to get anything workable into the CVS tree than having nothing at all.
By the way, isn't it so that a copy of shell32.dll gets loaded for each application anyway? If so, then the variable is at least application local, and, c'mon, who is going to show two modal browsing dialog boxes at the same time :-)?
I'm not sure if the shell32 dll is really loaded multiple times in Windows.
However I can definitely see your point here but since the functions are all internal to shell32 at this point and can accept any parameters you may like, I would consider it a cleaner solution to allocate a structure on the stack which contains such helper variables together with the pointer to the user supplied BROWSEINFOA structure and pass its pointer as custom long variable to the dialog procedure instead of lpbi only.
In the end it mist probably wouldn't really matter for practical use since it is a modal dialog, but who knows who is going to extend on that API at some point and stumbling over this.
Rolf Kalbermatter
Rolf Kalbermatter wrote:
Robert van Herk wrote:
I guess you are right, but the strange thing is that this was already the case in the original code. The hwnd of the tree view and the pointer to the browseinfo structure are copied into a global variable, so at least my code is not worse here :-).
Well, existing code doesn't necessarily need to be a nice and clean implementation as it may have been at some point more important to get anything workable into the CVS tree than having nothing at all.
By the way, isn't it so that a copy of shell32.dll gets loaded for each application anyway? If so, then the variable is at least application local, and, c'mon, who is going to show two modal browsing dialog boxes at the same time :-)?
I'm not sure if the shell32 dll is really loaded multiple times in Windows.
However I can definitely see your point here but since the functions are all internal to shell32 at this point and can accept any parameters you may like, I would consider it a cleaner solution to allocate a structure on the stack which contains such helper variables together with the pointer to the user supplied BROWSEINFOA structure and pass its pointer as custom long variable to the dialog procedure instead of lpbi only.
Aaaaaaaaah <<little light bulb>>. You see, I have been spoiled with Java :-) and never thought of that solution. But it is much better, you're right. I will put it in the new version of the patch.
Thanks,
Robert
However I can definitely see your point here but since the functions are all internal to shell32 at this point and can accept any parameters you may like, I would consider it a cleaner solution to allocate a structure on the stack which contains such helper variables together with the pointer to the user supplied BROWSEINFOA structure and pass its pointer as custom long variable to the dialog procedure instead of lpbi only.
There is something I don't understand though.
According to msdn:
INT_PTR DialogBoxParam(
HINSTANCE hInstance, LPCTSTR lpTemplateName, HWND hWndParent, DLGPROC lpDialogFunc, LPARAM dwInitParam );
dwInitParam [in] Specifies the value to pass to the dialog box in the lParam parameter of the WM_INITDIALOG message.
So yes, I could put it all in a structure on the stack and then pass it to the dlgproc, but it will only arrive there on WM_INITDIALOG. However, I will also need the structure when the other messages come in. So where do I save it then, if I cannot put it in a global var? Am I missing something?
Robert
Robert van Herk wrote:
So yes, I could put it all in a structure on the stack and then pass it to the dlgproc, but it will only arrive there on WM_INITDIALOG. However, I will also need the structure when the other messages come in. So where do I save it then, if I cannot put it in a global var? Am I missing something?
Robert
One option is to store it inside the dialog itself. Use "SetWindowsLong" to store the pointer. Just make sure that you pass the right parameter on to WM_INITDIALOG, and that you deallocate the struct (if it's allocated dynamically from the heap) before the window is destroyed (WM_DESTROY is a good bet).
Shachar
-
Robert van Herk -
Rolf Kalbermatter -
Shachar Shemesh