Merge lp:~phill-ridout/openlp/import-depreciations into lp:openlp
- import-depreciations
- Merge into trunk
Proposed by
Phill
Status: | Merged |
---|---|
Merged at revision: | 2738 |
Proposed branch: | lp:~phill-ridout/openlp/import-depreciations |
Merge into: | lp:openlp |
Diff against target: |
405 lines (+152/-68) 10 files modified
openlp/core/common/__init__.py (+45/-1) openlp/core/lib/pluginmanager.py (+3/-28) openlp/core/ui/media/mediacontroller.py (+6/-14) openlp/plugins/presentations/lib/impresscontroller.py (+2/-1) openlp/plugins/presentations/lib/pdfcontroller.py (+1/-1) openlp/plugins/presentations/lib/powerpointcontroller.py (+1/-1) openlp/plugins/presentations/lib/pptviewcontroller.py (+1/-1) openlp/plugins/presentations/lib/presentationtab.py (+1/-1) openlp/plugins/presentations/presentationplugin.py (+6/-15) tests/functional/openlp_core_common/test_common.py (+86/-5) |
To merge this branch: | bzr merge lp:~phill-ridout/openlp/import-depreciations |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Phill | Approve | ||
Tomas Groth | Approve | ||
Tim Bentley | Approve | ||
Review via email: mp+324045@code.launchpad.net |
This proposal supersedes a proposal from 2017-05-14.
Commit message
Description of the change
Fix the depreciated code, and refactor it.
lp:~phill-ridout/openlp/import-depreciations (revision 2736)
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[FAILURE] https:/
Stopping after failure
To post a comment you must log in.
Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal | # |
review:
Needs Fixing
Revision history for this message
Phill (phill-ridout) wrote : Posted in a previous version of this proposal | # |
I had merged with trunk, but forgot to update trunk!
Revision history for this message
Phill (phill-ridout) : Posted in a previous version of this proposal | # |
review:
Needs Fixing
Revision history for this message
Tomas Groth (tomasgroth) : | # |
review:
Approve
Revision history for this message
Phill (phill-ridout) : | # |
review:
Approve
Revision history for this message
Phill (phill-ridout) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'openlp/core/common/__init__.py' |
2 | --- openlp/core/common/__init__.py 2016-12-31 11:01:36 +0000 |
3 | +++ openlp/core/common/__init__.py 2017-05-15 10:31:30 +0000 |
4 | @@ -24,7 +24,7 @@ |
5 | OpenLP work. |
6 | """ |
7 | import hashlib |
8 | - |
9 | +import importlib |
10 | import logging |
11 | import os |
12 | import re |
13 | @@ -32,6 +32,7 @@ |
14 | import traceback |
15 | from chardet.universaldetector import UniversalDetector |
16 | from ipaddress import IPv4Address, IPv6Address, AddressValueError |
17 | +from pathlib import Path |
18 | from shutil import which |
19 | from subprocess import check_output, CalledProcessError, STDOUT |
20 | |
21 | @@ -79,6 +80,49 @@ |
22 | log.exception('failed to check if directory exists or create directory') |
23 | |
24 | |
25 | +def extension_loader(glob_pattern, excluded_files=[]): |
26 | + """ |
27 | + A utility function to find and load OpenLP extensions, such as plugins, presentation and media controllers and |
28 | + importers. |
29 | + |
30 | + :param glob_pattern: A glob pattern used to find the extension(s) to be imported. Should be relative to the |
31 | + application directory. i.e. openlp/plugins/*/*plugin.py |
32 | + :type glob_pattern: str |
33 | + |
34 | + :param excluded_files: A list of file names to exclude that the glob pattern may find. |
35 | + :type excluded_files: list of strings |
36 | + |
37 | + :return: None |
38 | + :rtype: None |
39 | + """ |
40 | + app_dir = Path(AppLocation.get_directory(AppLocation.AppDir)).parent |
41 | + for extension_path in app_dir.glob(glob_pattern): |
42 | + extension_path = extension_path.relative_to(app_dir) |
43 | + if extension_path.name in excluded_files: |
44 | + continue |
45 | + module_name = path_to_module(extension_path) |
46 | + try: |
47 | + importlib.import_module(module_name) |
48 | + except (ImportError, OSError): |
49 | + # On some platforms importing vlc.py might cause OSError exceptions. (e.g. Mac OS X) |
50 | + log.warning('Failed to import {module_name} on path {extension_path}' |
51 | + .format(module_name=module_name, extension_path=str(extension_path))) |
52 | + |
53 | + |
54 | +def path_to_module(path): |
55 | + """ |
56 | + Convert a path to a module name (i.e openlp.core.common) |
57 | + |
58 | + :param path: The path to convert to a module name. |
59 | + :type path: Path |
60 | + |
61 | + :return: The module name. |
62 | + :rtype: str |
63 | + """ |
64 | + module_path = path.with_suffix('') |
65 | + return '.'.join(module_path.parts) |
66 | + |
67 | + |
68 | def get_frozen_path(frozen_option, non_frozen_option): |
69 | """ |
70 | Return a path based on the system status. |
71 | |
72 | === modified file 'openlp/core/lib/pluginmanager.py' |
73 | --- openlp/core/lib/pluginmanager.py 2016-12-31 11:01:36 +0000 |
74 | +++ openlp/core/lib/pluginmanager.py 2017-05-15 10:31:30 +0000 |
75 | @@ -23,10 +23,9 @@ |
76 | Provide plugin management |
77 | """ |
78 | import os |
79 | -import imp |
80 | |
81 | from openlp.core.lib import Plugin, PluginStatus |
82 | -from openlp.core.common import AppLocation, RegistryProperties, OpenLPMixin, RegistryMixin |
83 | +from openlp.core.common import AppLocation, RegistryProperties, OpenLPMixin, RegistryMixin, extension_loader |
84 | |
85 | |
86 | class PluginManager(RegistryMixin, OpenLPMixin, RegistryProperties): |
87 | @@ -70,32 +69,8 @@ |
88 | """ |
89 | Scan a directory for objects inheriting from the ``Plugin`` class. |
90 | """ |
91 | - start_depth = len(os.path.abspath(self.base_path).split(os.sep)) |
92 | - present_plugin_dir = os.path.join(self.base_path, 'presentations') |
93 | - self.log_debug('finding plugins in {path} at depth {depth:d}'.format(path=self.base_path, depth=start_depth)) |
94 | - for root, dirs, files in os.walk(self.base_path): |
95 | - for name in files: |
96 | - if name.endswith('.py') and not name.startswith('__'): |
97 | - path = os.path.abspath(os.path.join(root, name)) |
98 | - this_depth = len(path.split(os.sep)) |
99 | - if this_depth - start_depth > 2: |
100 | - # skip anything lower down |
101 | - break |
102 | - module_name = name[:-3] |
103 | - # import the modules |
104 | - self.log_debug('Importing {name} from {root}. Depth {depth:d}'.format(name=module_name, |
105 | - root=root, |
106 | - depth=this_depth)) |
107 | - try: |
108 | - # Use the "imp" library to try to get around a problem with the PyUNO library which |
109 | - # monkey-patches the __import__ function to do some magic. This causes issues with our tests. |
110 | - # First, try to find the module we want to import, searching the directory in root |
111 | - fp, path_name, description = imp.find_module(module_name, [root]) |
112 | - # Then load the module (do the actual import) using the details from find_module() |
113 | - imp.load_module(module_name, fp, path_name, description) |
114 | - except ImportError as e: |
115 | - self.log_exception('Failed to import module {name} on path {path}: ' |
116 | - '{args}'.format(name=module_name, path=path, args=e.args[0])) |
117 | + glob_pattern = os.path.join('openlp', 'plugins', '*', '*plugin.py') |
118 | + extension_loader(glob_pattern) |
119 | plugin_classes = Plugin.__subclasses__() |
120 | plugin_objects = [] |
121 | for p in plugin_classes: |
122 | |
123 | === modified file 'openlp/core/ui/media/mediacontroller.py' |
124 | --- openlp/core/ui/media/mediacontroller.py 2016-12-31 11:01:36 +0000 |
125 | +++ openlp/core/ui/media/mediacontroller.py 2017-05-15 10:31:30 +0000 |
126 | @@ -28,7 +28,8 @@ |
127 | import datetime |
128 | from PyQt5 import QtCore, QtWidgets |
129 | |
130 | -from openlp.core.common import OpenLPMixin, Registry, RegistryMixin, RegistryProperties, Settings, UiStrings, translate |
131 | +from openlp.core.common import OpenLPMixin, Registry, RegistryMixin, RegistryProperties, Settings, UiStrings, \ |
132 | + extension_loader, translate |
133 | from openlp.core.lib import ItemCapabilities |
134 | from openlp.core.lib.ui import critical_error_message_box |
135 | from openlp.core.common import AppLocation |
136 | @@ -39,6 +40,7 @@ |
137 | parse_optical_path |
138 | from openlp.core.ui.lib.toolbar import OpenLPToolbar |
139 | |
140 | + |
141 | log = logging.getLogger(__name__) |
142 | |
143 | TICK_TIME = 200 |
144 | @@ -172,19 +174,9 @@ |
145 | Check to see if we have any media Player's available. |
146 | """ |
147 | log.debug('_check_available_media_players') |
148 | - controller_dir = os.path.join(AppLocation.get_directory(AppLocation.AppDir), 'core', 'ui', 'media') |
149 | - for filename in os.listdir(controller_dir): |
150 | - if filename.endswith('player.py') and filename != 'mediaplayer.py': |
151 | - path = os.path.join(controller_dir, filename) |
152 | - if os.path.isfile(path): |
153 | - module_name = 'openlp.core.ui.media.' + os.path.splitext(filename)[0] |
154 | - log.debug('Importing controller %s', module_name) |
155 | - try: |
156 | - __import__(module_name, globals(), locals(), []) |
157 | - # On some platforms importing vlc.py might cause |
158 | - # also OSError exceptions. (e.g. Mac OS X) |
159 | - except (ImportError, OSError): |
160 | - log.warning('Failed to import %s on path %s', module_name, path) |
161 | + controller_dir = os.path.join('openlp', 'core', 'ui', 'media') |
162 | + glob_pattern = os.path.join(controller_dir, '*player.py') |
163 | + extension_loader(glob_pattern, ['mediaplayer.py']) |
164 | player_classes = MediaPlayer.__subclasses__() |
165 | for player_class in player_classes: |
166 | self.register_players(player_class(self)) |
167 | |
168 | === modified file 'openlp/plugins/presentations/lib/impresscontroller.py' |
169 | --- openlp/plugins/presentations/lib/impresscontroller.py 2016-12-31 11:01:36 +0000 |
170 | +++ openlp/plugins/presentations/lib/impresscontroller.py 2017-05-15 10:31:30 +0000 |
171 | @@ -58,7 +58,8 @@ |
172 | |
173 | from openlp.core.lib import ScreenList |
174 | from openlp.core.common import get_uno_command, get_uno_instance |
175 | -from .presentationcontroller import PresentationController, PresentationDocument, TextType |
176 | +from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument, \ |
177 | + TextType |
178 | |
179 | |
180 | log = logging.getLogger(__name__) |
181 | |
182 | === modified file 'openlp/plugins/presentations/lib/pdfcontroller.py' |
183 | --- openlp/plugins/presentations/lib/pdfcontroller.py 2016-12-31 11:01:36 +0000 |
184 | +++ openlp/plugins/presentations/lib/pdfcontroller.py 2017-05-15 10:31:30 +0000 |
185 | @@ -29,7 +29,7 @@ |
186 | from openlp.core.common import AppLocation, check_binary_exists |
187 | from openlp.core.common import Settings, is_win |
188 | from openlp.core.lib import ScreenList |
189 | -from .presentationcontroller import PresentationController, PresentationDocument |
190 | +from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument |
191 | |
192 | if is_win(): |
193 | from subprocess import STARTUPINFO, STARTF_USESHOWWINDOW |
194 | |
195 | === modified file 'openlp/plugins/presentations/lib/powerpointcontroller.py' |
196 | --- openlp/plugins/presentations/lib/powerpointcontroller.py 2016-12-31 11:01:36 +0000 |
197 | +++ openlp/plugins/presentations/lib/powerpointcontroller.py 2017-05-15 10:31:30 +0000 |
198 | @@ -43,7 +43,7 @@ |
199 | from openlp.core.lib import ScreenList |
200 | from openlp.core.lib.ui import UiStrings, critical_error_message_box, translate |
201 | from openlp.core.common import trace_error_handler, Registry |
202 | -from .presentationcontroller import PresentationController, PresentationDocument |
203 | +from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument |
204 | |
205 | log = logging.getLogger(__name__) |
206 | |
207 | |
208 | === modified file 'openlp/plugins/presentations/lib/pptviewcontroller.py' |
209 | --- openlp/plugins/presentations/lib/pptviewcontroller.py 2016-12-31 11:01:36 +0000 |
210 | +++ openlp/plugins/presentations/lib/pptviewcontroller.py 2017-05-15 10:31:30 +0000 |
211 | @@ -35,7 +35,7 @@ |
212 | |
213 | from openlp.core.common import AppLocation |
214 | from openlp.core.lib import ScreenList |
215 | -from .presentationcontroller import PresentationController, PresentationDocument |
216 | +from openlp.plugins.presentations.lib.presentationcontroller import PresentationController, PresentationDocument |
217 | |
218 | |
219 | log = logging.getLogger(__name__) |
220 | |
221 | === modified file 'openlp/plugins/presentations/lib/presentationtab.py' |
222 | --- openlp/plugins/presentations/lib/presentationtab.py 2016-12-31 11:01:36 +0000 |
223 | +++ openlp/plugins/presentations/lib/presentationtab.py 2017-05-15 10:31:30 +0000 |
224 | @@ -25,7 +25,7 @@ |
225 | from openlp.core.common import Settings, UiStrings, translate |
226 | from openlp.core.lib import SettingsTab, build_icon |
227 | from openlp.core.lib.ui import critical_error_message_box |
228 | -from .pdfcontroller import PdfController |
229 | +from openlp.plugins.presentations.lib.pdfcontroller import PdfController |
230 | |
231 | |
232 | class PresentationTab(SettingsTab): |
233 | |
234 | === modified file 'openlp/plugins/presentations/presentationplugin.py' |
235 | --- openlp/plugins/presentations/presentationplugin.py 2016-12-31 11:01:36 +0000 |
236 | +++ openlp/plugins/presentations/presentationplugin.py 2017-05-15 10:31:30 +0000 |
237 | @@ -20,19 +20,18 @@ |
238 | # Temple Place, Suite 330, Boston, MA 02111-1307 USA # |
239 | ############################################################################### |
240 | """ |
241 | -The :mod:`presentationplugin` module provides the ability for OpenLP to display presentations from a variety of document |
242 | -formats. |
243 | +The :mod:`openlp.plugins.presentations.presentationplugin` module provides the ability for OpenLP to display |
244 | +presentations from a variety of document formats. |
245 | """ |
246 | import os |
247 | import logging |
248 | |
249 | from PyQt5 import QtCore |
250 | |
251 | -from openlp.core.common import AppLocation, translate |
252 | +from openlp.core.common import AppLocation, extension_loader, translate |
253 | from openlp.core.lib import Plugin, StringContent, build_icon |
254 | from openlp.plugins.presentations.lib import PresentationController, PresentationMediaItem, PresentationTab |
255 | |
256 | - |
257 | log = logging.getLogger(__name__) |
258 | |
259 | |
260 | @@ -122,17 +121,9 @@ |
261 | Check to see if we have any presentation software available. If not do not install the plugin. |
262 | """ |
263 | log.debug('check_pre_conditions') |
264 | - controller_dir = os.path.join(AppLocation.get_directory(AppLocation.PluginsDir), 'presentations', 'lib') |
265 | - for filename in os.listdir(controller_dir): |
266 | - if filename.endswith('controller.py') and filename != 'presentationcontroller.py': |
267 | - path = os.path.join(controller_dir, filename) |
268 | - if os.path.isfile(path): |
269 | - module_name = 'openlp.plugins.presentations.lib.' + os.path.splitext(filename)[0] |
270 | - log.debug('Importing controller {name}'.format(name=module_name)) |
271 | - try: |
272 | - __import__(module_name, globals(), locals(), []) |
273 | - except ImportError: |
274 | - log.warning('Failed to import {name} on path {path}'.format(name=module_name, path=path)) |
275 | + controller_dir = os.path.join('openlp', 'plugins', 'presentations', 'lib') |
276 | + glob_pattern = os.path.join(controller_dir, '*controller.py') |
277 | + extension_loader(glob_pattern, ['presentationcontroller.py']) |
278 | controller_classes = PresentationController.__subclasses__() |
279 | for controller_class in controller_classes: |
280 | controller = controller_class(self) |
281 | |
282 | === modified file 'tests/functional/openlp_core_common/test_common.py' |
283 | --- tests/functional/openlp_core_common/test_common.py 2017-04-24 05:17:55 +0000 |
284 | +++ tests/functional/openlp_core_common/test_common.py 2017-05-15 10:31:30 +0000 |
285 | @@ -22,11 +22,13 @@ |
286 | """ |
287 | Functional tests to test the AppLocation class and related methods. |
288 | """ |
289 | +from pathlib import Path |
290 | from unittest import TestCase |
291 | -from unittest.mock import MagicMock, patch |
292 | +from unittest.mock import MagicMock, call, patch |
293 | |
294 | -from openlp.core.common import check_directory_exists, de_hump, trace_error_handler, translate, is_win, is_macosx, \ |
295 | - is_linux, clean_button_text |
296 | +from openlp.core import common |
297 | +from openlp.core.common import check_directory_exists, clean_button_text, de_hump, extension_loader, is_macosx, \ |
298 | + is_linux, is_win, path_to_module, trace_error_handler, translate |
299 | |
300 | |
301 | class TestCommonFunctions(TestCase): |
302 | @@ -72,6 +74,72 @@ |
303 | mocked_exists.assert_called_with(directory_to_check) |
304 | self.assertRaises(ValueError, check_directory_exists, directory_to_check) |
305 | |
306 | + def test_extension_loader_no_files_found(self): |
307 | + """ |
308 | + Test the `extension_loader` function when no files are found |
309 | + """ |
310 | + # GIVEN: A mocked `Path.glob` method which does not match any files |
311 | + with patch('openlp.core.common.AppLocation.get_directory', return_value='/app/dir/openlp'), \ |
312 | + patch.object(common.Path, 'glob', return_value=[]), \ |
313 | + patch('openlp.core.common.importlib.import_module') as mocked_import_module: |
314 | + |
315 | + # WHEN: Calling `extension_loader` |
316 | + extension_loader('glob', ['file2.py', 'file3.py']) |
317 | + |
318 | + # THEN: `extension_loader` should not try to import any files |
319 | + self.assertFalse(mocked_import_module.called) |
320 | + |
321 | + def test_extension_loader_files_found(self): |
322 | + """ |
323 | + Test the `extension_loader` function when it successfully finds and loads some files |
324 | + """ |
325 | + # GIVEN: A mocked `Path.glob` method which returns a list of files |
326 | + with patch('openlp.core.common.AppLocation.get_directory', return_value='/app/dir/openlp'), \ |
327 | + patch.object(common.Path, 'glob', return_value=[Path('/app/dir/openlp/import_dir/file1.py'), |
328 | + Path('/app/dir/openlp/import_dir/file2.py'), |
329 | + Path('/app/dir/openlp/import_dir/file3.py'), |
330 | + Path('/app/dir/openlp/import_dir/file4.py')]), \ |
331 | + patch('openlp.core.common.importlib.import_module') as mocked_import_module: |
332 | + |
333 | + # WHEN: Calling `extension_loader` with a list of files to exclude |
334 | + extension_loader('glob', ['file2.py', 'file3.py']) |
335 | + |
336 | + # THEN: `extension_loader` should only try to import the files that are matched by the blob, excluding the |
337 | + # files listed in the `excluded_files` argument |
338 | + mocked_import_module.assert_has_calls([call('openlp.import_dir.file1'), call('openlp.import_dir.file4')]) |
339 | + |
340 | + def test_extension_loader_import_error(self): |
341 | + """ |
342 | + Test the `extension_loader` function when `SourceFileLoader` raises a `ImportError` |
343 | + """ |
344 | + # GIVEN: A mocked `import_module` which raises an `ImportError` |
345 | + with patch('openlp.core.common.AppLocation.get_directory', return_value='/app/dir/openlp'), \ |
346 | + patch.object(common.Path, 'glob', return_value=[Path('/app/dir/openlp/import_dir/file1.py')]), \ |
347 | + patch('openlp.core.common.importlib.import_module', side_effect=ImportError()), \ |
348 | + patch('openlp.core.common.log') as mocked_logger: |
349 | + |
350 | + # WHEN: Calling `extension_loader` |
351 | + extension_loader('glob') |
352 | + |
353 | + # THEN: The `ImportError` should be caught and logged |
354 | + self.assertTrue(mocked_logger.warning.called) |
355 | + |
356 | + def test_extension_loader_os_error(self): |
357 | + """ |
358 | + Test the `extension_loader` function when `import_module` raises a `ImportError` |
359 | + """ |
360 | + # GIVEN: A mocked `SourceFileLoader` which raises an `OSError` |
361 | + with patch('openlp.core.common.AppLocation.get_directory', return_value='/app/dir/openlp'), \ |
362 | + patch.object(common.Path, 'glob', return_value=[Path('/app/dir/openlp/import_dir/file1.py')]), \ |
363 | + patch('openlp.core.common.importlib.import_module', side_effect=OSError()), \ |
364 | + patch('openlp.core.common.log') as mocked_logger: |
365 | + |
366 | + # WHEN: Calling `extension_loader` |
367 | + extension_loader('glob') |
368 | + |
369 | + # THEN: The `OSError` should be caught and logged |
370 | + self.assertTrue(mocked_logger.warning.called) |
371 | + |
372 | def test_de_hump_conversion(self): |
373 | """ |
374 | Test the de_hump function with a class name |
375 | @@ -83,7 +151,7 @@ |
376 | new_string = de_hump(string) |
377 | |
378 | # THEN: the new string should be converted to python format |
379 | - self.assertTrue(new_string == "my_class", 'The class name should have been converted') |
380 | + self.assertEqual(new_string, "my_class", 'The class name should have been converted') |
381 | |
382 | def test_de_hump_static(self): |
383 | """ |
384 | @@ -96,7 +164,20 @@ |
385 | new_string = de_hump(string) |
386 | |
387 | # THEN: the new string should be converted to python format |
388 | - self.assertTrue(new_string == "my_class", 'The class name should have been preserved') |
389 | + self.assertEqual(new_string, "my_class", 'The class name should have been preserved') |
390 | + |
391 | + def test_path_to_module(self): |
392 | + """ |
393 | + Test `path_to_module` when supplied with a `Path` object |
394 | + """ |
395 | + # GIVEN: A `Path` object |
396 | + path = Path('openlp/core/ui/media/webkitplayer.py') |
397 | + |
398 | + # WHEN: Calling path_to_module with the `Path` object |
399 | + result = path_to_module(path) |
400 | + |
401 | + # THEN: path_to_module should return the module name |
402 | + self.assertEqual(result, 'openlp.core.ui.media.webkitplayer') |
403 | |
404 | def test_trace_error_handler(self): |
405 | """ |
Looks good but has issues with trunk!