Merge lp:~nataliabidart/ubuntuone-control-panel/default-folders into lp:ubuntuone-control-panel

[Natalia Bidart]

- Implemented a method to list the user's default folders in every platform (part of LP: #933697).

[Brian Curtin (brian.curtin)]

94 + with open(dirs_path) as f:
95 + while True:
96 + line = f.readline()

The file object you get back (f) is already iterable, so you could replace the "while True" with "for line in f"

Other than this it looks alright.

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

> 94 + with open(dirs_path) as f:
> 95 + while True:
> 96 + line = f.readline()
>
> The file object you get back (f) is already iterable, so you could replace the
> "while True" with "for line in f"
>
> Other than this it looks alright.

Fixed and pushed, great catch! thanks

[lp:~nataliabidart/ubuntuone-control-panel/default-folders]

289. By Natalia Bidart on 2012-03-16

- Inprove default_folders file content iteration (thanks brian.curtin).

[dobey (dobey)]

86 + if dirs_path is None:
87 + dirs_path = os.path.join(user_home, '.config', 'user-dirs.dirs')

We probably want to use xdg_config_home from dirspec.basedir here, instead of assuming ~/.config/.

With the suggestion from Brian, you can also drop the if line is None: break check.

Do we need to check that translated names for directories get handled correctly as well here, and ensure that unicode handling doesn't break again? I don't see a test for that case in your tests.

Also, on Windows, for the special folders you check for, which aren't supported on XP for example, how is that error handled? I suppose there should be a test which causes and checks for the error when that happens, to ensure we handle it properly? The linked MSDN page seems to suggest that _MYVIDEO doesn't exist on 5.0 (XP) for example. It might also be worth noting in the comment that if/when we drop XP support, we probably want to switch to _MYDOCUMENTS instead of _PERSONAL for that folder; as the documentation seems to suggest we should do.

review: Needs Fixing

[Brian Curtin (brian.curtin)]

+1

review: Approve

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[Natalia Bidart (nataliabidart)]

> 86 + if dirs_path is None:
> 87 + dirs_path = os.path.join(user_home, '.config', 'user-dirs.dirs')
>
> We probably want to use xdg_config_home from dirspec.basedir here, instead of
> assuming ~/.config/.

Added.

> With the suggestion from Brian, you can also drop the if line is None: break
> check.

Done already.

> Do we need to check that translated names for directories get handled
> correctly as well here, and ensure that unicode handling doesn't break again?
> I don't see a test for that case in your tests.

Added tests for this.

> Also, on Windows, for the special folders you check for, which aren't
> supported on XP for example, how is that error handled? I suppose there should
> be a test which causes and checks for the error when that happens, to ensure
> we handle it properly? The linked MSDN page seems to suggest that _MYVIDEO
> doesn't exist on 5.0 (XP) for example. It might also be worth noting in the
> comment that if/when we drop XP support, we probably want to switch to
> _MYDOCUMENTS instead of _PERSONAL for that folder; as the documentation seems
> to suggest we should do.

Good catch, made the code more robust and added tests for it.

[lp:~nataliabidart/ubuntuone-control-panel/default-folders]

290. By Natalia Bidart on 2012-03-16

Better unicode management, and non existent folders.

[dobey (dobey)]

review: Approve

=== modified file 'ubuntuone/controlpanel/logger.py'
--- ubuntuone/controlpanel/logger.py	2012-02-06 18:09:58 +0000
+++ ubuntuone/controlpanel/logger.py	2012-03-16 20:00:25 +0000
@@ -1,8 +1,6 @@
 # -*- coding: utf-8 -*-
-
-# Authors: Natalia B Bidart <[e-mail]>
 #
-# Copyright 2010 Canonical Ltd.
+# Copyright 2010-2012 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -30,7 +28,7 @@
 from ubuntuone.platform.xdg_base_directory import ubuntuone_log_dir
 
 
-if os.environ.get('DEBUG'):
+if os.environ.get('U1_DEBUG'):
     LOG_LEVEL = logging.DEBUG
 else:
     # Only log this level and above

=== modified file 'ubuntuone/controlpanel/utils/__init__.py'
--- ubuntuone/controlpanel/utils/__init__.py	2012-03-09 13:44:53 +0000
+++ ubuntuone/controlpanel/utils/__init__.py	2012-03-16 20:00:25 +0000
@@ -1,7 +1,6 @@
 # -*- coding: utf-8 -*-
-
 #
-# Copyright 2010 Canonical Ltd.
+# Copyright 2010-2012 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -37,10 +36,14 @@
 if sys.platform == 'win32':
     from ubuntuone.controlpanel.utils import windows
     are_updates_present = windows.are_updates_present
+    default_folders = windows.default_folders
     perform_update = windows.perform_update
 else:
+    from ubuntuone.controlpanel.utils import linux
     are_updates_present = lambda *args, **kwargs: False
+    default_folders = linux.default_folders
     perform_update = lambda *args, **kwargs: None
+
 # pylint: enable=C0103
 
 

=== added file 'ubuntuone/controlpanel/utils/linux.py'
--- ubuntuone/controlpanel/utils/linux.py	1970-01-01 00:00:00 +0000
+++ ubuntuone/controlpanel/utils/linux.py	2012-03-16 20:00:25 +0000
@@ -0,0 +1,59 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright 2012 Canonical Ltd.
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Miscelaneous functions and constants for linux."""
+
+import codecs
+import os
+
+from dirspec.basedir import xdg_config_home
+from ubuntuone.controlpanel.logger import setup_logging
+
+
+logger = setup_logging('utils.linux')
+
+
+def default_folders(user_home='', dirs_path=None):
+    """Return a list of the folders to add by default."""
+    result = []
+
+    if dirs_path is None:
+        dirs_path = os.path.join(xdg_config_home, u'user-dirs.dirs')
+
+    if not os.path.exists(dirs_path):
+        logger.warning('default_folders: dirs_path %r does not exist.',
+                       dirs_path)
+        return result
+
+    # pylint: disable=W0702
+
+    try:
+        with codecs.open(dirs_path, encoding='utf-8') as f:
+            for line in f:
+                if line.startswith(u'#'):
+                    continue
+
+                try:
+                    _, value = line.strip().split(u'=')
+                    value = value.strip(u'"').replace(u'$HOME', user_home)
+                except:
+                    logger.exception('default_folders: can not row %r:', line)
+                else:
+                    result.append(value)
+    except:
+        logger.exception('default_folders: can not load file %r:', dirs_path)
+
+    return result

=== modified file 'ubuntuone/controlpanel/utils/tests/test_linux.py'
--- ubuntuone/controlpanel/utils/tests/test_linux.py	2011-10-26 10:45:54 +0000
+++ ubuntuone/controlpanel/utils/tests/test_linux.py	2012-03-16 20:00:25 +0000
@@ -17,11 +17,35 @@
 
 """Test the linux utils functions."""
 
+import codecs
+import os
 
 from twisted.internet import defer
 
 from ubuntuone.controlpanel import utils
-from ubuntuone.controlpanel.tests import TestCase
+from ubuntuone.controlpanel.tests import TestCase, USER_HOME
+
+REAL_CONTENT = u"""
+XDG_DESKTOP_DIR="$HOME/Desktop"
+XDG_DOWNLOAD_DIR="$HOME/Downloads"
+XDG_PUBLICSHARE_DIR="$HOME/Public"
+XDG_DOCUMENTS_DIR="$HOME/Documents"
+XDG_MUSIC_DIR="$HOME/Music"
+XDG_PICTURES_DIR="$HOME/Pictures"
+XDG_VIDEOS_DIR="$HOME/Videos"
+"""
+
+NON_ASCII = u"""
+XDG_DESKTOP_DIR="$HOME/桌面"
+XDG_DOWNLOAD_DIR="$HOME/下载"
+XDG_DOCUMENTS_DIR="$HOME/文档"
+"""
+
+LATIN_1 = u"""
+XDG_DESKTOP_DIR="$HOME/Súbeme"
+XDG_DOWNLOAD_DIR="$HOME/Bájame"
+XDG_DOCUMENTS_DIR="$HOME/Mis ñoños Documentos"
+"""
 
 
 class AutoupdaterTestCase(TestCase):
@@ -36,3 +60,79 @@
     def test_perform_update(self):
         """Test the method that performs the update."""
         self.assertEqual(None, utils.perform_update())
+
+
+class DefaultFoldersTestCase(TestCase):
+    """Test suite for the default_folders retriever."""
+
+    def _create_faked_file(self, content='', encoding=None):
+        """Create a faked file to be parsed and clean it up afterwards."""
+        filename = './dirs-parser-test.dirs'
+        if os.path.exists(filename):
+            os.remove(filename)
+
+        if encoding is None:
+            encoding = 'utf-8'
+
+        with codecs.open(filename, 'w', encoding=encoding) as f:
+            f.write(content)
+
+        self.addCleanup(os.remove, filename)
+
+        return filename
+
+    def test_default_folders_not_file(self):
+        """Default folders are empty if the file does not exist."""
+        path = 'foo/bar/baz/yadda/yo'
+        assert not os.path.exists(path)
+        actual = utils.default_folders(dirs_path=path)
+
+        self.assertEqual(actual, [])
+
+    def test_default_folders_empty_file(self):
+        """Default folders are empty if the file empty."""
+        filename = self._create_faked_file(content='')
+        actual = utils.default_folders(dirs_path=filename)
+
+        self.assertEqual(actual, [])
+
+    def test_default_folders_only_comments(self):
+        """Default folders are empty if only comments in the file."""
+        content = u"# yadda yadda\n# foo bar baz\n#puipuipui"
+        filename = self._create_faked_file(content=content)
+        actual = utils.default_folders(dirs_path=filename)
+
+        self.assertEqual(actual, [])
+
+    def test_default_folders_syntax_error(self):
+        """Default folders do not explode on syntax error."""
+        content = u"foo ~ bar\nYADDA=DOO"
+        filename = self._create_faked_file(content=content)
+        actual = utils.default_folders(dirs_path=filename)
+
+        self.assertEqual(actual, [u'DOO'])
+
+    def test_default_folders_parsed(self):
+        """Default folders parses the content of the file."""
+        filename = self._create_faked_file(content=REAL_CONTENT)
+        actual = utils.default_folders(user_home=USER_HOME, dirs_path=filename)
+
+        expected = (i.split(u'=')[1].strip(u'"') for i in REAL_CONTENT.split())
+        expected = [i.replace(u'$HOME', USER_HOME) for i in expected]
+        self.assertEqual(actual, expected)
+
+    def test_default_folders_non_ascii(self):
+        """Default folders parses the content if it's non-ascii."""
+        filename = self._create_faked_file(content=NON_ASCII)
+        actual = utils.default_folders(user_home=USER_HOME, dirs_path=filename)
+
+        expected = (i.split(u'=')[1].strip(u'"') for i in NON_ASCII.split())
+        expected = [i.replace(u'$HOME', USER_HOME) for i in expected]
+        self.assertEqual(actual, expected)
+
+    def test_default_folders_bad_encoding(self):
+        """Default folders parses the content if it's non-ascii."""
+        filename = self._create_faked_file(content=LATIN_1, encoding='latin-1')
+        actual = utils.default_folders(user_home=USER_HOME, dirs_path=filename)
+
+        self.assertEqual(actual, [])

=== modified file 'ubuntuone/controlpanel/utils/tests/test_windows.py'
--- ubuntuone/controlpanel/utils/tests/test_windows.py	2011-11-15 12:36:15 +0000
+++ ubuntuone/controlpanel/utils/tests/test_windows.py	2012-03-16 20:00:25 +0000
@@ -87,6 +87,47 @@
         self.assertEqual(0, self._called[0][5])
 
 
+class DefaultFoldersTestCase(TestCase):
+    """Test the default_folders method."""
+
+    def test_special_folders(self, names=None):
+        """Test that default_folders does the right thing."""
+        folders = utils.default_folders()
+
+        if names is None:
+            names = ('PERSONAL', 'MYMUSIC', 'MYPICTURES', 'MYVIDEO')
+
+        self.assertEqual(len(folders), len(names))
+        self.assertEqual(folders, [os.path.abspath(f) for f in folders])
+
+        expected = []
+        for name in names:
+            name = getattr(utils.windows.shellcon, 'CSIDL_%s' % name)
+            folder = utils.windows.shell.SHGetFolderPath(0, name, None, 0)
+            expected.append(folder.encode('utf8'))
+
+        self.assertEqual(sorted(folders), sorted(expected))
+
+    def test_special_folders_with_error(self):
+        """Test that default_folders handles errors."""
+        failing_name = 'MYVIDEO'
+        original_func = utils.windows.shell.SHGetFolderPath
+
+        class SomeError(Exception):
+            """For test only."""
+
+        def fail_for_some(code, name, parent, flag):
+            """Custom failer."""
+            fail = getattr(utils.windows.shellcon, 'CSIDL_%s' % failing_name)
+            if name == fail:
+                raise SomeError('The parameter is incorrect.')
+            else:
+                return original_func(code, name, parent, flag)
+
+        self.patch(utils.windows.shell, 'SHGetFolderPath', fail_for_some)
+        self.test_special_folders(names=('PERSONAL', 'MYMUSIC', 'MYPICTURES'))
+
+
 class GetPathTestCase(TestCase):
     """Test the code that is used for the auto update process."""
 

=== modified file 'ubuntuone/controlpanel/utils/windows.py'
--- ubuntuone/controlpanel/utils/windows.py	2012-03-09 00:47:47 +0000
+++ ubuntuone/controlpanel/utils/windows.py	2012-03-16 20:00:25 +0000
@@ -23,13 +23,15 @@
 # Avoid pylint error on Linux
 # pylint: disable=F0401
 import win32api
+
+from win32com.shell import shell, shellcon
 # pylint: enable=F0401
 from twisted.internet import defer
 from twisted.internet.utils import getProcessValue
 
 from ubuntuone.controlpanel.logger import setup_logging
 
-logger = setup_logging('utils')
+logger = setup_logging('utils.windows')
 AUTOUPDATE_EXE_NAME = 'autoupdate-windows.exe'
 
 
@@ -62,6 +64,30 @@
     defer.returnValue(result)
 
 
+def default_folders(user_home=None):
+    """Return a list of the folders to add by default."""
+    # as per http://msdn.microsoft.com/en-us/library/windows/desktop/bb762181,
+    # SHGetFolderPath is deprecated, we should migrate to SHGetKnownFolderPath
+    # (http://msdn.microsoft.com/en-us/library/windows/desktop/bb762188)
+    # but the latter does not support XP
+    # (Minimum supported client: Windows Vista)
+    get_path = lambda name: shell.SHGetFolderPath(
+        0, getattr(shellcon, name), None, 0).encode('utf8')
+
+    folders = []
+    # More information on these constants at
+    # http://msdn.microsoft.com/en-us/library/bb762494
+    for name in (u'PERSONAL', u'MYMUSIC', u'MYPICTURES', u'MYVIDEO'):
+        name = u'CSIDL_%s' % name
+        try:
+            folders.append(get_path(name))
+        except:  # pylint: disable=W0702
+            logger.exception('default_folders: could not retrieve folder info '
+                             'for name %r.', name)
+
+    return folders
+
+
 def perform_update():
     """Spawn the autoupdate process and call the stop function."""
     update_path = _get_update_path()