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

Proposed by Natalia Bidart
Status: Merged
Approved by: Natalia Bidart
Approved revision: 290
Merged at revision: 287
Proposed branch: lp:~nataliabidart/ubuntuone-control-panel/default-folders
Merge into: lp:ubuntuone-control-panel
Diff against target: 339 lines (+235/-8)
6 files modified
ubuntuone/controlpanel/logger.py (+2/-4)
ubuntuone/controlpanel/utils/__init__.py (+5/-2)
ubuntuone/controlpanel/utils/linux.py (+59/-0)
ubuntuone/controlpanel/utils/tests/test_linux.py (+101/-1)
ubuntuone/controlpanel/utils/tests/test_windows.py (+41/-0)
ubuntuone/controlpanel/utils/windows.py (+27/-1)
To merge this branch: bzr merge lp:~nataliabidart/ubuntuone-control-panel/default-folders
Reviewer Review Type Date Requested Status
dobey (community) Approve
Diego Sarmentero (community) Approve
Brian Curtin (community) Approve
Review via email: mp+97949@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Brian Curtin (brian.curtin) wrote :

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
Revision history for this message
Natalia Bidart (nataliabidart) wrote :

> 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

289. By Natalia Bidart

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

Revision history for this message
dobey (dobey) wrote :

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
Revision history for this message
Brian Curtin (brian.curtin) wrote :

+1

review: Approve
Revision history for this message
Diego Sarmentero (diegosarmentero) wrote :

+1

review: Approve
Revision history for this message
Natalia Bidart (nataliabidart) wrote :

> 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.

290. By Natalia Bidart

Better unicode management, and non existent folders.

Revision history for this message
dobey (dobey) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
1# -*- coding: utf-8 -*-1# -*- coding: utf-8 -*-
2
3# Authors: Natalia B Bidart <natalia.bidart@canonical.com>
4#2#
5# Copyright 2010 Canonical Ltd.3# Copyright 2010-2012 Canonical Ltd.
6#4#
7# This program is free software: you can redistribute it and/or modify it5# This program is free software: you can redistribute it and/or modify it
8# under the terms of the GNU General Public License version 3, as published6# under the terms of the GNU General Public License version 3, as published
@@ -30,7 +28,7 @@
30from ubuntuone.platform.xdg_base_directory import ubuntuone_log_dir28from ubuntuone.platform.xdg_base_directory import ubuntuone_log_dir
3129
3230
33if os.environ.get('DEBUG'):31if os.environ.get('U1_DEBUG'):
34 LOG_LEVEL = logging.DEBUG32 LOG_LEVEL = logging.DEBUG
35else:33else:
36 # Only log this level and above34 # Only log this level and above
3735
=== 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 @@
1# -*- coding: utf-8 -*-1# -*- coding: utf-8 -*-
2
3#2#
4# Copyright 2010 Canonical Ltd.3# Copyright 2010-2012 Canonical Ltd.
5#4#
6# This program is free software: you can redistribute it and/or modify it5# This program is free software: you can redistribute it and/or modify it
7# under the terms of the GNU General Public License version 3, as published6# under the terms of the GNU General Public License version 3, as published
@@ -37,10 +36,14 @@
37if sys.platform == 'win32':36if sys.platform == 'win32':
38 from ubuntuone.controlpanel.utils import windows37 from ubuntuone.controlpanel.utils import windows
39 are_updates_present = windows.are_updates_present38 are_updates_present = windows.are_updates_present
39 default_folders = windows.default_folders
40 perform_update = windows.perform_update40 perform_update = windows.perform_update
41else:41else:
42 from ubuntuone.controlpanel.utils import linux
42 are_updates_present = lambda *args, **kwargs: False43 are_updates_present = lambda *args, **kwargs: False
44 default_folders = linux.default_folders
43 perform_update = lambda *args, **kwargs: None45 perform_update = lambda *args, **kwargs: None
46
44# pylint: enable=C010347# pylint: enable=C0103
4548
4649
4750
=== 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 @@
1# -*- coding: utf-8 -*-
2#
3# Copyright 2012 Canonical Ltd.
4#
5# This program is free software: you can redistribute it and/or modify it
6# under the terms of the GNU General Public License version 3, as published
7# by the Free Software Foundation.
8#
9# This program is distributed in the hope that it will be useful, but
10# WITHOUT ANY WARRANTY; without even the implied warranties of
11# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
12# PURPOSE. See the GNU General Public License for more details.
13#
14# You should have received a copy of the GNU General Public License along
15# with this program. If not, see <http://www.gnu.org/licenses/>.
16
17"""Miscelaneous functions and constants for linux."""
18
19import codecs
20import os
21
22from dirspec.basedir import xdg_config_home
23from ubuntuone.controlpanel.logger import setup_logging
24
25
26logger = setup_logging('utils.linux')
27
28
29def default_folders(user_home='', dirs_path=None):
30 """Return a list of the folders to add by default."""
31 result = []
32
33 if dirs_path is None:
34 dirs_path = os.path.join(xdg_config_home, u'user-dirs.dirs')
35
36 if not os.path.exists(dirs_path):
37 logger.warning('default_folders: dirs_path %r does not exist.',
38 dirs_path)
39 return result
40
41 # pylint: disable=W0702
42
43 try:
44 with codecs.open(dirs_path, encoding='utf-8') as f:
45 for line in f:
46 if line.startswith(u'#'):
47 continue
48
49 try:
50 _, value = line.strip().split(u'=')
51 value = value.strip(u'"').replace(u'$HOME', user_home)
52 except:
53 logger.exception('default_folders: can not row %r:', line)
54 else:
55 result.append(value)
56 except:
57 logger.exception('default_folders: can not load file %r:', dirs_path)
58
59 return result
060
=== 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 @@
1717
18"""Test the linux utils functions."""18"""Test the linux utils functions."""
1919
20import codecs
21import os
2022
21from twisted.internet import defer23from twisted.internet import defer
2224
23from ubuntuone.controlpanel import utils25from ubuntuone.controlpanel import utils
24from ubuntuone.controlpanel.tests import TestCase26from ubuntuone.controlpanel.tests import TestCase, USER_HOME
27
28REAL_CONTENT = u"""
29XDG_DESKTOP_DIR="$HOME/Desktop"
30XDG_DOWNLOAD_DIR="$HOME/Downloads"
31XDG_PUBLICSHARE_DIR="$HOME/Public"
32XDG_DOCUMENTS_DIR="$HOME/Documents"
33XDG_MUSIC_DIR="$HOME/Music"
34XDG_PICTURES_DIR="$HOME/Pictures"
35XDG_VIDEOS_DIR="$HOME/Videos"
36"""
37
38NON_ASCII = u"""
39XDG_DESKTOP_DIR="$HOME/桌面"
40XDG_DOWNLOAD_DIR="$HOME/下载"
41XDG_DOCUMENTS_DIR="$HOME/文档"
42"""
43
44LATIN_1 = u"""
45XDG_DESKTOP_DIR="$HOME/Súbeme"
46XDG_DOWNLOAD_DIR="$HOME/Bájame"
47XDG_DOCUMENTS_DIR="$HOME/Mis ñoños Documentos"
48"""
2549
2650
27class AutoupdaterTestCase(TestCase):51class AutoupdaterTestCase(TestCase):
@@ -36,3 +60,79 @@
36 def test_perform_update(self):60 def test_perform_update(self):
37 """Test the method that performs the update."""61 """Test the method that performs the update."""
38 self.assertEqual(None, utils.perform_update())62 self.assertEqual(None, utils.perform_update())
63
64
65class DefaultFoldersTestCase(TestCase):
66 """Test suite for the default_folders retriever."""
67
68 def _create_faked_file(self, content='', encoding=None):
69 """Create a faked file to be parsed and clean it up afterwards."""
70 filename = './dirs-parser-test.dirs'
71 if os.path.exists(filename):
72 os.remove(filename)
73
74 if encoding is None:
75 encoding = 'utf-8'
76
77 with codecs.open(filename, 'w', encoding=encoding) as f:
78 f.write(content)
79
80 self.addCleanup(os.remove, filename)
81
82 return filename
83
84 def test_default_folders_not_file(self):
85 """Default folders are empty if the file does not exist."""
86 path = 'foo/bar/baz/yadda/yo'
87 assert not os.path.exists(path)
88 actual = utils.default_folders(dirs_path=path)
89
90 self.assertEqual(actual, [])
91
92 def test_default_folders_empty_file(self):
93 """Default folders are empty if the file empty."""
94 filename = self._create_faked_file(content='')
95 actual = utils.default_folders(dirs_path=filename)
96
97 self.assertEqual(actual, [])
98
99 def test_default_folders_only_comments(self):
100 """Default folders are empty if only comments in the file."""
101 content = u"# yadda yadda\n# foo bar baz\n#puipuipui"
102 filename = self._create_faked_file(content=content)
103 actual = utils.default_folders(dirs_path=filename)
104
105 self.assertEqual(actual, [])
106
107 def test_default_folders_syntax_error(self):
108 """Default folders do not explode on syntax error."""
109 content = u"foo ~ bar\nYADDA=DOO"
110 filename = self._create_faked_file(content=content)
111 actual = utils.default_folders(dirs_path=filename)
112
113 self.assertEqual(actual, [u'DOO'])
114
115 def test_default_folders_parsed(self):
116 """Default folders parses the content of the file."""
117 filename = self._create_faked_file(content=REAL_CONTENT)
118 actual = utils.default_folders(user_home=USER_HOME, dirs_path=filename)
119
120 expected = (i.split(u'=')[1].strip(u'"') for i in REAL_CONTENT.split())
121 expected = [i.replace(u'$HOME', USER_HOME) for i in expected]
122 self.assertEqual(actual, expected)
123
124 def test_default_folders_non_ascii(self):
125 """Default folders parses the content if it's non-ascii."""
126 filename = self._create_faked_file(content=NON_ASCII)
127 actual = utils.default_folders(user_home=USER_HOME, dirs_path=filename)
128
129 expected = (i.split(u'=')[1].strip(u'"') for i in NON_ASCII.split())
130 expected = [i.replace(u'$HOME', USER_HOME) for i in expected]
131 self.assertEqual(actual, expected)
132
133 def test_default_folders_bad_encoding(self):
134 """Default folders parses the content if it's non-ascii."""
135 filename = self._create_faked_file(content=LATIN_1, encoding='latin-1')
136 actual = utils.default_folders(user_home=USER_HOME, dirs_path=filename)
137
138 self.assertEqual(actual, [])
39139
=== 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 @@
87 self.assertEqual(0, self._called[0][5])87 self.assertEqual(0, self._called[0][5])
8888
8989
90class DefaultFoldersTestCase(TestCase):
91 """Test the default_folders method."""
92
93 def test_special_folders(self, names=None):
94 """Test that default_folders does the right thing."""
95 folders = utils.default_folders()
96
97 if names is None:
98 names = ('PERSONAL', 'MYMUSIC', 'MYPICTURES', 'MYVIDEO')
99
100 self.assertEqual(len(folders), len(names))
101 self.assertEqual(folders, [os.path.abspath(f) for f in folders])
102
103 expected = []
104 for name in names:
105 name = getattr(utils.windows.shellcon, 'CSIDL_%s' % name)
106 folder = utils.windows.shell.SHGetFolderPath(0, name, None, 0)
107 expected.append(folder.encode('utf8'))
108
109 self.assertEqual(sorted(folders), sorted(expected))
110
111 def test_special_folders_with_error(self):
112 """Test that default_folders handles errors."""
113 failing_name = 'MYVIDEO'
114 original_func = utils.windows.shell.SHGetFolderPath
115
116 class SomeError(Exception):
117 """For test only."""
118
119 def fail_for_some(code, name, parent, flag):
120 """Custom failer."""
121 fail = getattr(utils.windows.shellcon, 'CSIDL_%s' % failing_name)
122 if name == fail:
123 raise SomeError('The parameter is incorrect.')
124 else:
125 return original_func(code, name, parent, flag)
126
127 self.patch(utils.windows.shell, 'SHGetFolderPath', fail_for_some)
128 self.test_special_folders(names=('PERSONAL', 'MYMUSIC', 'MYPICTURES'))
129
130
90class GetPathTestCase(TestCase):131class GetPathTestCase(TestCase):
91 """Test the code that is used for the auto update process."""132 """Test the code that is used for the auto update process."""
92133
93134
=== 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 @@
23# Avoid pylint error on Linux23# Avoid pylint error on Linux
24# pylint: disable=F040124# pylint: disable=F0401
25import win32api25import win32api
26
27from win32com.shell import shell, shellcon
26# pylint: enable=F040128# pylint: enable=F0401
27from twisted.internet import defer29from twisted.internet import defer
28from twisted.internet.utils import getProcessValue30from twisted.internet.utils import getProcessValue
2931
30from ubuntuone.controlpanel.logger import setup_logging32from ubuntuone.controlpanel.logger import setup_logging
3133
32logger = setup_logging('utils')34logger = setup_logging('utils.windows')
33AUTOUPDATE_EXE_NAME = 'autoupdate-windows.exe'35AUTOUPDATE_EXE_NAME = 'autoupdate-windows.exe'
3436
3537
@@ -62,6 +64,30 @@
62 defer.returnValue(result)64 defer.returnValue(result)
6365
6466
67def default_folders(user_home=None):
68 """Return a list of the folders to add by default."""
69 # as per http://msdn.microsoft.com/en-us/library/windows/desktop/bb762181,
70 # SHGetFolderPath is deprecated, we should migrate to SHGetKnownFolderPath
71 # (http://msdn.microsoft.com/en-us/library/windows/desktop/bb762188)
72 # but the latter does not support XP
73 # (Minimum supported client: Windows Vista)
74 get_path = lambda name: shell.SHGetFolderPath(
75 0, getattr(shellcon, name), None, 0).encode('utf8')
76
77 folders = []
78 # More information on these constants at
79 # http://msdn.microsoft.com/en-us/library/bb762494
80 for name in (u'PERSONAL', u'MYMUSIC', u'MYPICTURES', u'MYVIDEO'):
81 name = u'CSIDL_%s' % name
82 try:
83 folders.append(get_path(name))
84 except: # pylint: disable=W0702
85 logger.exception('default_folders: could not retrieve folder info '
86 'for name %r.', name)
87
88 return folders
89
90
65def perform_update():91def perform_update():
66 """Spawn the autoupdate process and call the stop function."""92 """Spawn the autoupdate process and call the stop function."""
67 update_path = _get_update_path()93 update_path = _get_update_path()

Subscribers

People subscribed via source and target branches