I'm sending this to the list, so others might comment as well, in case
anyone is interested.
I will resend the patch to the list as well in a separate email, so information
will be complete.
On 14.08.2009 01:46, Steve Borho wrote (in private email):
> On Thu, Aug 13, 2009 at 5:03 PM, Adrian Buehlmann<adr...@cadifra.com> wrote:
>> # HG changeset patch
>> # User Adrian Buehlmann <adr...@cadifra.com>
>> # Date 1250200892 -7200
>> # Branch stable
>> # Node ID 560870a155be106daf92e06abf4c9fda80fa9ddc
>> # Parent 943019bebd82519756ac34111ce8759f81997b48
>> shellext: use system PathIsDirectory in IsDirectory
>>
>> diff --git a/win32/shellext/TortoiseUtils.cpp
>> b/win32/shellext/TortoiseUtils.cpp
>> --- a/win32/shellext/TortoiseUtils.cpp
>> +++ b/win32/shellext/TortoiseUtils.cpp
>> @@ -8,6 +8,8 @@
>> #include <io.h>
>> #include "FCNTL.H"
>>
>> +#include "shlwapi.h"
>> +
>>
>> LPWSTR hf_mbtowc(LPWSTR lpw, LPCSTR lpa, int nChars)
>> {
>> @@ -150,11 +152,7 @@ std::string GetTemporaryFile(LPCTSTR pre
>>
>> bool IsDirectory(const std::string& filename)
>> {
>> - DWORD attributes = GetFileAttributesA(filename.c_str());
>> - if (attributes == INVALID_FILE_ATTRIBUTES)
>> - return false;
>> -
>> - return (attributes & FILE_ATTRIBUTE_DIRECTORY) != 0;
>> + return ::PathIsDirectory(filename.c_str()) != 0;
>> }
>
> Is this in response to anything in particular, or just a simplification?
Ok.
Honestly, I'm a bit surprised that this needs an explanation before
it is pushed to the official repo.
But asking questions is always a good thing (If you don't mind getting
another lengthy cliff note by me :)
So I will try to write a bit about that patch and other things below:
There is a slight chance that using a Windows API function instead of
doing it "by hand" (as the current implementation of IsDirectory does)
might be able to do an optimization we don't know of.
Given the fact that we have a problem report about slow context menu (488).
As can be seen, IsDirectory is called by the context menu code (before
opening the menu). This can be slow over the network. It is unlikely
that my patch will be faster, but who knows? Why not use the windows API
function PathIsDirectory? If nothing else, using that higher level
windows API function instead of our lower level self made implementation
would turn IsDirectory into a one liner.
I admit I don't have the time (or to be honest, don't want to invest it)
to test this proposed change on all the Windows System variations TortoiseHg
wants to support (Which, BTW, is quite an impressive list if spelled out:
Windows XP 32 bit, Windows XP 64 bit, Vista 32 bit, Vista 64 bit,
Windows 7 RC 32 bit, Windows 7 RC 64 bit). Oh and there is now even
Windows 7 RTM (so two more).
I have only tested the patch on Windows XP 32 bit and haven't noticed
any speed change. For the network access, I have used a samba share,
running on a FreeBSD server, since I don't have any full blown MS Windows
servers here and I didn't want to bother to configure a network share on
another Windows XP workstation box here for this tiny patch.
If anyone else would like to test the patch in a different setup, feel
free to do so.
But given the fact that we already have PathIsDirectory calls in the overlay
handling code paths, I thought this patch would be relatively safe to apply
to the cmenu code path as well (and to the rest of the code paths in the
overlay code that use it through GetHgRepoRoot).
It stumbled over this while I was pondering a bit about issue 488.
Checking if GetHgRepoRoot is done optimal.
If anyone else has good ideas how to improve issue 488, I will gladly
step back another two steps from hacking on the C++ shell extension.
I haven't done that much on the cmenu part anyway.
My main focus has been on the overlay stuff, doing just some
smaller tweaks here an there on the original cmenu code provided by
TK (like fixing some implementations of COM interfaces, or for example,
eliminating a GDI resource leak caused by the icon handling code).
My interest in the cmenu part is generally a bit lower than it was
for the overlay part. The latter being the main reason why the
pre 0.8 TortoiseHg shell integration was far from optimal for me
(as a user of TortoiseHg).
I'm especially not interested to invest time in improving the
icon drawing bug(s) we still have on the cmenu (see the list of issue, I'm
just too lazy to dig out the number right now). I personally simply don't need
icons in cmenus, but since I know lots of users are keen on having them,
I'll keep mostly silent on that front. Plus the icons seem to work fine on
my current home turf (Windows XP 32 bit). After having fixed the GDI
resource leak :) (which my brother Frank reported to me, thanks!).
As an idea regarding issue 488: maybe TortoiseHg should consider not
trying to search for a mercurial repository in the cmenu code, if the
path is on a network share, as doing some network accesses to search
for hg repos can be quite time consuming to do in a ui thread, which
fits squarely with the task to present a menu.
There is already a reader comment in the respective MSDN article
hinting in that direction (it even proposes to do it in background
thread, which would be quite complicated to implement):
http://msdn.microsoft.com/en-us/library/bb773621(VS.85).aspx
Although that would mean having to settle on a static cmenu for
network paths (which wouldn't be such a bad thing, IMHO).
As a general note, I'm currently trying to step down a bit on the
C++ shellext, as it starts to get a bit boring and I have probably
hacked a bit too much on it in the last couple of months anyway.
Time to move on.
Hopefully, someone else can step forward a bit on that front.
Apologies for the long post again. I hope it helps in one way or
another.
I will send the patch to the list now, as promised.
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
_______________________________________________
Tortoisehg-develop mailing list
Tortoisehg-develop@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tortoisehg-develop
