Merge lp:~knightrider0xd/openlp/non-text-item-height-default into lp:openlp

[Ian Knight]

Adds automatic functionality to the max-height for non-text items feature, and sets this as the default value.
Advanced tab spin box replaced with combo box.

Test coverage expanded on other functions in listpreviewwidget and pep8 errors fixed.
Fixed test naming.
Merged with trunk as of 2016-06-02_10:30.

lp:~knightrider0xd/openlp/non-text-item-height-default (revision 2674)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1584/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1495/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1433/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1212/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/802/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/870/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/738/

[Raoul Snyman (raoul-snyman)]

Got this when I started OpenLP.

Traceback (most recent call last):
  File "./openlp.py", line 44, in <module>
    main()
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/core/__init__.py", line 392, in main
    sys.exit(application.run(qt_args))
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/core/__init__.py", line 142, in run
    Registry().execute('bootstrap_initialise')
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/core/common/registry.py", line 137, in execute
    result = function(*args, **kwargs)
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/core/lib/pluginmanager.py", line 68, in bootstrap_initialise
    self.initialise_plugins()
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/core/lib/pluginmanager.py", line 183, in initialise_plugins
    plugin.initialise()
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/plugins/media/mediaplugin.py", line 66, in initialise
    super().initialise()
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/core/lib/plugin.py", line 277, in initialise
    self.media_item.initialise()
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/plugins/media/lib/mediaitem.py", line 303, in initialise
    self.load_list(Settings().value(self.settings_section + '/media files'))
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/plugins/media/lib/mediaitem.py", line 368, in load_list
    item_name.setToolTip('%s@%s-%s' % (file_name, format_milliseconds(start), format_milliseconds(end)))
  File "/home/raoul/Projects/OpenLP/OpenLP/non-text-item-height-default/openlp/core/ui/media/__init__.py", line 143, in format_milliseconds
    millis=millis)
ValueError: Unknown format code 'd' for object of type 'float'

review: Needs Fixing

[Ian Knight (knightrider0xd)]

Hey superfly.
I can't replicate this bug, and looking at that trace, it appears to be related to something in media, not my changes.

[Tim Bentley (trb143)]

This code is existing and connected to CD /dvd items in media.
Did you have some media items created and then take to cd / dvd out of the machine?

[Raoul Snyman (raoul-snyman)]

I figured this out, it's a bug in Ken's new formatting code. I've fixed it.

[Raoul Snyman (raoul-snyman)]

Please just merge trunk into your branch and resubmit? Thanks.

[Raoul Snyman (raoul-snyman)]

Sorry, just spotted this. Your tests are incorrectly named, and won't be picked up by the test runner (especially when we move to nose2).

review: Needs Fixing

[Raoul Snyman (raoul-snyman)]

review: Approve

=== modified file 'openlp/core/common/settings.py'
--- openlp/core/common/settings.py	2016-05-15 17:32:04 +0000
+++ openlp/core/common/settings.py	2016-06-02 10:36:58 +0000
@@ -129,7 +129,7 @@
         'advanced/recent file count': 4,
         'advanced/save current plugin': False,
         'advanced/slide limits': SlideLimits.End,
-        'advanced/slide max height': 0,
+        'advanced/slide max height': -4,
         'advanced/single click preview': False,
         'advanced/single click service preview': False,
         'advanced/x11 bypass wm': X11_BYPASS_DEFAULT,
 
=== modified file 'openlp/core/ui/advancedtab.py'
--- openlp/core/ui/advancedtab.py	2016-05-20 16:22:06 +0000
+++ openlp/core/ui/advancedtab.py	2016-06-02 10:36:58 +0000
@@ -87,11 +87,14 @@
         self.ui_layout.addRow(self.expand_service_item_check_box)
         self.slide_max_height_label = QtWidgets.QLabel(self.ui_group_box)
         self.slide_max_height_label.setObjectName('slide_max_height_label')
-        self.slide_max_height_spin_box = QtWidgets.QSpinBox(self.ui_group_box)
-        self.slide_max_height_spin_box.setObjectName('slide_max_height_spin_box')
-        self.slide_max_height_spin_box.setRange(0, 1000)
-        self.slide_max_height_spin_box.setSingleStep(20)
-        self.ui_layout.addRow(self.slide_max_height_label, self.slide_max_height_spin_box)
+        self.slide_max_height_combo_box = QtWidgets.QComboBox(self.ui_group_box)
+        self.slide_max_height_combo_box.addItem('', userData=0)
+        self.slide_max_height_combo_box.addItem('', userData=-4)
+        # Generate numeric values for combo box dynamically
+        for px in range(60, 801, 5):
+            self.slide_max_height_combo_box.addItem(str(px) + 'px', userData=px)
+        self.slide_max_height_combo_box.setObjectName('slide_max_height_combo_box')
+        self.ui_layout.addRow(self.slide_max_height_label, self.slide_max_height_combo_box)
         self.autoscroll_label = QtWidgets.QLabel(self.ui_group_box)
         self.autoscroll_label.setObjectName('autoscroll_label')
         self.autoscroll_combo_box = QtWidgets.QComboBox(self.ui_group_box)
@@ -265,7 +268,8 @@
                                                              'Expand new service items on creation'))
         self.slide_max_height_label.setText(translate('OpenLP.AdvancedTab',
                                                       'Max height for non-text slides\nin slide controller:'))
-        self.slide_max_height_spin_box.setSpecialValueText(translate('OpenLP.AdvancedTab', 'Disabled'))
+        self.slide_max_height_combo_box.setItemText(0, translate('OpenLP.AdvancedTab', 'Disabled'))
+        self.slide_max_height_combo_box.setItemText(1, translate('OpenLP.AdvancedTab', 'Automatic'))
         self.autoscroll_label.setText(translate('OpenLP.AdvancedTab',
                                                 'When changing slides:'))
         self.autoscroll_combo_box.setItemText(0, translate('OpenLP.AdvancedTab', 'Do not auto-scroll'))
@@ -355,10 +359,13 @@
         self.single_click_preview_check_box.setChecked(settings.value('single click preview'))
         self.single_click_service_preview_check_box.setChecked(settings.value('single click service preview'))
         self.expand_service_item_check_box.setChecked(settings.value('expand service item'))
-        self.slide_max_height_spin_box.setValue(settings.value('slide max height'))
+        slide_max_height_value = settings.value('slide max height')
+        for i in range(0, self.slide_max_height_combo_box.count()):
+            if self.slide_max_height_combo_box.itemData(i) == slide_max_height_value:
+                self.slide_max_height_combo_box.setCurrentIndex(i)
         autoscroll_value = settings.value('autoscrolling')
         for i in range(0, len(self.autoscroll_map)):
-            if self.autoscroll_map[i] == autoscroll_value:
+            if self.autoscroll_map[i] == autoscroll_value and i < self.autoscroll_combo_box.count():
                 self.autoscroll_combo_box.setCurrentIndex(i)
         self.enable_auto_close_check_box.setChecked(settings.value('enable exit confirmation'))
         self.hide_mouse_check_box.setChecked(settings.value('hide mouse'))
@@ -439,7 +446,9 @@
         settings.setValue('single click preview', self.single_click_preview_check_box.isChecked())
         settings.setValue('single click service preview', self.single_click_service_preview_check_box.isChecked())
         settings.setValue('expand service item', self.expand_service_item_check_box.isChecked())
-        settings.setValue('slide max height', self.slide_max_height_spin_box.value())
+        slide_max_height_index = self.slide_max_height_combo_box.currentIndex()
+        slide_max_height_value = self.slide_max_height_combo_box.itemData(slide_max_height_index)
+        settings.setValue('slide max height', slide_max_height_value)
         settings.setValue('autoscrolling', self.autoscroll_map[self.autoscroll_combo_box.currentIndex()])
         settings.setValue('enable exit confirmation', self.enable_auto_close_check_box.isChecked())
         settings.setValue('hide mouse', self.hide_mouse_check_box.isChecked())
 
=== modified file 'openlp/core/ui/lib/listpreviewwidget.py'
--- openlp/core/ui/lib/listpreviewwidget.py	2016-05-05 03:57:04 +0000
+++ openlp/core/ui/lib/listpreviewwidget.py	2016-06-02 10:36:58 +0000
@@ -63,6 +63,7 @@
         # Initialize variables.
         self.service_item = ServiceItem()
         self.screen_ratio = screen_ratio
+        self.auto_row_height = 100
         # Connect signals
         self.verticalHeader().sectionResized.connect(self.row_resized)
 
@@ -87,8 +88,14 @@
                 height = self.viewport().width() // self.screen_ratio
                 max_img_row_height = Settings().value('advanced/slide max height')
                 # Adjust for row height cap if in use.
-                if isinstance(max_img_row_height, int) and max_img_row_height > 0 and height > max_img_row_height:
-                    height = max_img_row_height
+                if isinstance(max_img_row_height, int):
+                    if max_img_row_height > 0 and height > max_img_row_height:
+                        height = max_img_row_height
+                    elif max_img_row_height < 0:
+                        # If auto setting, show that number of slides, or if the resulting slides too small, 100px.
+                        # E.g. If setting is -4, 4 slides will be visible, unless those slides are < 100px high.
+                        self.auto_row_height = max(self.viewport().height() / (-1 * max_img_row_height), 100)
+                        height = min(height, self.auto_row_height)
                 # Apply new height to slides
                 for frame_number in range(len(self.service_item.get_frames())):
                     self.setRowHeight(frame_number, height)
@@ -99,7 +106,7 @@
         """
         # Only for non-text slides when row height cap in use
         max_img_row_height = Settings().value('advanced/slide max height')
-        if self.service_item.is_text() or not isinstance(max_img_row_height, int) or max_img_row_height <= 0:
+        if self.service_item.is_text() or not isinstance(max_img_row_height, int) or max_img_row_height == 0:
             return
         # Get and validate label widget containing slide & adjust max width
         try:
@@ -165,11 +172,15 @@
                 slide_height = width // self.screen_ratio
                 # Setup and validate row height cap if in use.
                 max_img_row_height = Settings().value('advanced/slide max height')
-                if isinstance(max_img_row_height, int) and max_img_row_height > 0:
-                    if slide_height > max_img_row_height:
+                if isinstance(max_img_row_height, int) and max_img_row_height != 0:
+                    if max_img_row_height > 0 and slide_height > max_img_row_height:
+                        # Manual Setting
                         slide_height = max_img_row_height
-                    label.setMaximumWidth(max_img_row_height * self.screen_ratio)
-                    label.resize(max_img_row_height * self.screen_ratio, max_img_row_height)
+                    elif max_img_row_height < 0 and slide_height > self.auto_row_height:
+                        # Auto Setting
+                        slide_height = self.auto_row_height
+                    label.setMaximumWidth(slide_height * self.screen_ratio)
+                    label.resize(slide_height * self.screen_ratio, slide_height)
                     # Build widget with stretch padding
                     container = QtWidgets.QWidget()
                     hbox = QtWidgets.QHBoxLayout()
 
=== modified file 'tests/functional/openlp_core_common/test_registryproperties.py'
--- tests/functional/openlp_core_common/test_registryproperties.py	2016-05-31 21:40:13 +0000
+++ tests/functional/openlp_core_common/test_registryproperties.py	2016-06-02 10:36:58 +0000
@@ -75,4 +75,3 @@
 
         # THEN the application should be none
         self.assertEqual(self.application, application, 'The application value should match')
-
 
=== modified file 'tests/functional/openlp_core_ui_lib/test_listpreviewwidget.py'
--- tests/functional/openlp_core_ui_lib/test_listpreviewwidget.py	2016-06-01 21:42:54 +0000
+++ tests/functional/openlp_core_ui_lib/test_listpreviewwidget.py	2016-06-02 10:36:58 +0000
@@ -227,6 +227,44 @@
 
     @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.resizeRowsToContents')
     @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.setRowHeight')
+    def test_replace_recalculate_layout_img_auto(self, mocked_setRowHeight, mocked_resizeRowsToContents):
+        """
+        Test if "Max height for non-text slides..." auto, img slides resized in replace_service_item & __recalc...
+        """
+        # GIVEN: A setting to adjust "Max height for non-text slides in slide controller",
+        #        an image ServiceItem and a ListPreviewWidget.
+
+        # Mock Settings().value('advanced/slide max height')
+        self.mocked_Settings_obj.value.return_value = -4
+        # Mock self.viewport().width()
+        self.mocked_viewport_obj.width.return_value = 200
+        self.mocked_viewport_obj.height.return_value = 600
+        # Mock image service item
+        service_item = MagicMock()
+        service_item.is_text.return_value = False
+        service_item.is_capable.return_value = False
+        service_item.get_frames.return_value = [{'title': None, 'path': None, 'image': None},
+                                                {'title': None, 'path': None, 'image': None}]
+        # init ListPreviewWidget and load service item
+        list_preview_widget = ListPreviewWidget(None, 1)
+        list_preview_widget.replace_service_item(service_item, 200, 0)
+        # Change viewport width before forcing a resize
+        self.mocked_viewport_obj.width.return_value = 400
+
+        # WHEN: __recalculate_layout() is called (via screen_size_changed)
+        list_preview_widget.screen_size_changed(1)
+        self.mocked_viewport_obj.height.return_value = 200
+        list_preview_widget.screen_size_changed(1)
+
+        # THEN: resizeRowsToContents() should not be called, while setRowHeight() should be called
+        #       twice for each slide.
+        self.assertEquals(mocked_resizeRowsToContents.call_count, 0, 'Should not be called')
+        self.assertEquals(mocked_setRowHeight.call_count, 6, 'Should be called 3 times for each slide')
+        calls = [call(0, 100), call(1, 100), call(0, 150), call(1, 150), call(0, 100), call(1, 100)]
+        mocked_setRowHeight.assert_has_calls(calls)
+
+    @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.resizeRowsToContents')
+    @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.setRowHeight')
     @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.cellWidget')
     def test_row_resized_text(self, mocked_cellWidget, mocked_setRowHeight, mocked_resizeRowsToContents):
         """
@@ -331,6 +369,41 @@
         # THEN: self.cellWidget(row, 0).children()[1].setMaximumWidth() should be called
         mocked_cellWidget_child.setMaximumWidth.assert_called_once_with(150)
 
+    @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.resizeRowsToContents')
+    @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.setRowHeight')
+    @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.cellWidget')
+    def test_row_resized_setting_changed(self, mocked_cellWidget, mocked_setRowHeight, mocked_resizeRowsToContents):
+        """
+        Test if "Max height for non-text slides..." enabled while item live, program doesn't crash on row_resized.
+        """
+        # GIVEN: A setting to adjust "Max height for non-text slides in slide controller",
+        #        an image ServiceItem and a ListPreviewWidget.
+
+        # Mock Settings().value('advanced/slide max height')
+        self.mocked_Settings_obj.value.return_value = 0
+        # Mock self.viewport().width()
+        self.mocked_viewport_obj.width.return_value = 200
+        # Mock image service item
+        service_item = MagicMock()
+        service_item.is_text.return_value = False
+        service_item.is_capable.return_value = False
+        service_item.get_frames.return_value = [{'title': None, 'path': None, 'image': None},
+                                                {'title': None, 'path': None, 'image': None}]
+        # Mock self.cellWidget().children()
+        mocked_cellWidget_obj = MagicMock()
+        mocked_cellWidget_obj.children.return_value = None
+        mocked_cellWidget.return_value = mocked_cellWidget_obj
+        # init ListPreviewWidget and load service item
+        list_preview_widget = ListPreviewWidget(None, 1)
+        list_preview_widget.replace_service_item(service_item, 200, 0)
+        self.mocked_Settings_obj.value.return_value = 100
+
+        # WHEN: row_resized() is called
+        list_preview_widget.row_resized(0, 100, 150)
+
+        # THEN: self.cellWidget(row, 0).children()[1].setMaximumWidth() should fail
+        self.assertRaises(Exception)
+
     @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.selectRow')
     @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.scrollToItem')
     @patch(u'openlp.core.ui.lib.listpreviewwidget.ListPreviewWidget.item')
 
=== modified file 'tests/functional/openlp_plugins/bibles/test_lib.py'
--- tests/functional/openlp_plugins/bibles/test_lib.py	2016-06-01 23:14:58 +0000
+++ tests/functional/openlp_plugins/bibles/test_lib.py	2016-06-02 10:36:58 +0000
@@ -43,6 +43,7 @@
         separators = {'sep_r': '\\s*(?:e)\\s*', 'sep_e_default': 'end', 'sep_v_display': 'w', 'sep_l_display': 'r',
                       'sep_v_default': ':|v|V|verse|verses', 'sep_l': '\\s*(?:r)\\s*', 'sep_l_default': ',|and',
                       'sep_e': '\\s*(?:t)\\s*', 'sep_v': '\\s*(?:w)\\s*', 'sep_r_display': 'e', 'sep_r_default': '-|to'}
+
         def _update_side_effect():
             """
             Update the references after mocking out the method
 
=== modified file 'tests/functional/openlp_plugins/songs/test_opsproimport.py'
--- tests/functional/openlp_plugins/songs/test_opsproimport.py	2016-06-01 23:14:58 +0000
+++ tests/functional/openlp_plugins/songs/test_opsproimport.py	2016-06-02 10:36:58 +0000
@@ -171,4 +171,3 @@
         result_data = json.loads(result_file.read().decode())
         self.assertListEqual(importer.verses, _get_item(result_data, 'verses'))
         self.assertListEqual(importer.verse_order_list_generated, _get_item(result_data, 'verse_order_list'))
-