[MERGE] add win32utils.get_local_appdata_location()

[Mark Hammond]

> > === modified file 'bzrlib/tests/__init__.py'
> > --- bzrlib/tests/__init__.py	2008-09-04 20:32:04 +0000
> > +++ bzrlib/tests/__init__.py	2008-09-19 00:50:24 +0000
> > @@ -1209,7 +1209,8 @@
> >          new_env = {
> >              'BZR_HOME': None, # Don't inherit BZR_HOME to all the
> tests.
> >              'HOME': os.getcwd(),
> > -            'APPDATA': None,  # bzr now use Win32 API and don't rely
> on APPDATA
> > +            # bzr now uses the Win32 API and doesn't rely on
> APPDATA, but the
> > +            # tests do check our impls match APPDATA
> 
> ^^^ This comment looks pretty mysterious.  Do we still need an APPDATA:
> None?  If not, do we need the comment at all?

Agreed - the comment is there more to make the patch understandable than
anything else.  We now want APPDATA left alone like all other variables not
in that list.  The comment should go.

> > +    def test_appdata_matches_environment(self):
> > +        # Typically the APPDATA environment variable will match
> > +        # get_appdata_location
> > +        # XXX - See bug 262874, which asserts the correct encoding
> is 'mbcs',
> > +        encoding = osutils.get_user_encoding()
> > +        env_val = os.environ.get("APPDATA", None)
> > +        if not env_val:
> > +            raise TestSkipped("No APPDATA environment variable
> exists")
> > +        self.assertPathsEqual(win32utils.get_appdata_location(),
> > +                              env_val.decode(encoding))
> 
> ^^^ This seems like a missing feature, not a generic TestSkipped.

*shrug* - if I manually remove that environment variable from my environment
then run the test, I'd be somewhat surprised if bzr asserted my operating
system/bzr installation/whatever was suddenly missing a "feature" (note the
feature of windows having 2 "app data" directories does exist - just the
variable doesn't).  OTOH, having the test reported as being "skipped" *does*
sound reasonable to me.  Maybe I don't understand the exact semantics of
"feature" that bzrlib imposes?  Either way, please indicate if you want me
to resubmit this with a different exception used in place of that (but
please do keep in mind that in these cases I'm actually adding new tests for
existing behaviour in an effort to be helpful, not because I enjoy playing
patch-ping-pong)

> > +    def test_local_appdata_not_using_environment(self):
> > +        # Test that we aren't falling back to the environment
> > +        first = win32utils.get_local_appdata_location()
> > +        self._captureVar("LOCALAPPDATA", None)
> > +        self.assertPathsEqual(first,
> win32utils.get_local_appdata_location())
> > +
> > +    def test_local_appdata_matches_environment(self):
> > +        # LOCALAPPDATA typically only exists on Vista, so we only
> attempt to
> > +        # compare when it exists.
> > +        lad = win32utils.get_local_appdata_location()
> > +        env = os.environ.get("LOCALAPPDATA")
> > +        if env:
> > +            # XXX - See bug 262874, which asserts the correct
> encoding is 'mbcs'
> > +            encoding = osutils.get_user_encoding()
> > +            self.assertPathsEqual(lad, env.decode(encoding))
> 
> ^^^ This shouldn't pass if there's no LOCALAPPDATA-- it's a missing
> feature on pre-Vista Windows.

By "not pass", do you mean "fail"?  Or you just want a specific
FeatureSkipped exception raised instead (but see above - its not clear to me
what "feature" is missing other than an environment variable)?  Something
else?
 
> > === modified file 'bzrlib/win32utils.py'
> > --- bzrlib/win32utils.py	2008-08-19 19:44:10 +0000
> > +++ bzrlib/win32utils.py	2008-09-04 11:39:30 +0000
> > @@ -75,6 +75,10 @@
> >  except ImportError:
> >      has_win32api = False
> >
> > +# pulling in win32com.shell is a bit of overhead, and normally we
> don't need
> > +# it as ctypes is preferred and common.  lazy_imports and "optional"
> > +# modules don't work well, so we do our own lazy thing...
> > +has_win32com_shell = None # Set to True or False once we know for
> sure...
> >
> >  # Special Win32 API constants
> >  # Handles of std streams
> > @@ -84,6 +88,7 @@
> >
> >  # CSIDL constants (from MSDN 2003)
> >  CSIDL_APPDATA = 0x001A      # Application Data folder
> > +CSIDL_LOCAL_APPDATA = 0x001c# <user name>\Local Settings\Applicaiton
> Data (non roaming)
> 
> ^^^^ "Application" is misspelled.

I believe you will find it mis-spelt in the MSDN header too.  Should I add
"(sic)"?  Should I correct the spelling, remove the "from MSDN ..." comment?

> > +def _get_sh_special_folder_path(csidl):
> > +    """Call SHGetSpecialFolderPathW if available, or return None.
> > +
> > +    Result is always unicode (or None).
> > +    """
> > +    if has_ctypes:
> > +        try:
> > +            SHGetSpecialFolderPath = \
> > +                ctypes.windll.shell32.SHGetSpecialFolderPathW
> > +        except AttributeError:
> > +            pass
> > +        else:
> > +            buf = ctypes.create_unicode_buffer(MAX_PATH)
> > +            if SHGetSpecialFolderPath(None,buf,csidl,0):
> > +                return buf.value
> > +
> > +    global has_win32com_shell
> > +    if has_win32com_shell is None:
> > +        try:
> > +            from win32com.shell import shell
> > +            has_win32com_shell = True
> > +        except ImportError:
> > +            has_win32com_shell = False
> 
> ^^^ This seems like it should be split out into a function.

Which part?  There are 7 new lines related to win32com.shell, but I wouldn't
expect that to cross a threshold of when a new function should be created.
No code is duplicated (indeed, the patch reduces duplication).  If this is a
purely a personal style issue, please be very specific about what you want
changed and I'd be happy to make it rather than discuss the finer points of
that style.

Obviously I'd also be happy for you to help me out and submit it with
whatever tweaks you felt were necessary - I hope the intent is quite clear.

> > +    if has_win32com_shell:
> > +        # still need to bind the name locally, but this is fast.
> > +        from win32com.shell import shell
> 
> ^^^ if has_win32com_shell is True, won't shell already be imported?

I believe that as the comment says, 'shell' will be an unbound local without
that block every time the function is called after the first.  Is the
comment unclear?  If so, how should I adjust it?