← Back to team overview

openlp-core team mailing list archive

[Merge] lp:~trb143/openlp/ft into lp:openlp

 

Tim Bentley has proposed merging lp:~trb143/openlp/ft into lp:openlp.

Requested reviews:
  Andreas Preikschat (googol)

For more details, see:
https://code.launchpad.net/~trb143/openlp/ft/+merge/188207

Review time.
This is a re-write of the Formatting tags UI.
Please review for usability and the use of the Controller for functionality.


-- 
https://code.launchpad.net/~trb143/openlp/ft/+merge/188207
Your team OpenLP Core is subscribed to branch lp:openlp.
=== modified file 'openlp/core/lib/formattingtags.py'
--- openlp/core/lib/formattingtags.py	2013-08-31 18:17:38 +0000
+++ openlp/core/lib/formattingtags.py	2013-09-28 20:56:43 +0000
@@ -36,7 +36,7 @@
 
 class FormattingTags(object):
     """
-    Static Class to HTML Tags to be access around the code the list is managed by the Options Tab.
+    Static Class for HTML Tags to be access around the code the list is managed by the Options Tab.
     """
     html_expands = []
 
@@ -48,22 +48,15 @@
         return FormattingTags.html_expands
 
     @staticmethod
-    def save_html_tags():
-        """
-        Saves all formatting tags except protected ones.
-        """
-        tags = []
-        for tag in FormattingTags.html_expands:
-            if not tag['protected'] and not tag.get('temporary'):
-                # Using dict ensures that copy is made and encoding of values a little later does not affect tags in
-                # the original list
-                tags.append(dict(tag))
-                tag = tags[-1]
-                # Remove key 'temporary' from tags. It is not needed to be saved.
-                if 'temporary' in tag:
-                    del tag['temporary']
+    def save_html_tags(new_tags):
+        """
+        Saves all formatting tags except protected ones
+
+        `new_tags`
+            The tags to be saved..
+        """
         # Formatting Tags were also known as display tags.
-        Settings().setValue('formattingTags/html_tags', json.dumps(tags) if tags else '')
+        Settings().setValue('formattingTags/html_tags', json.dumps(new_tags) if new_tags else '')
 
     @staticmethod
     def load_tags():

=== modified file 'openlp/core/ui/__init__.py'
--- openlp/core/ui/__init__.py	2013-08-31 18:17:38 +0000
+++ openlp/core/ui/__init__.py	2013-09-28 20:56:43 +0000
@@ -95,6 +95,7 @@
 from .pluginform import PluginForm
 from .settingsform import SettingsForm
 from .formattingtagform import FormattingTagForm
+from .formattingtagcontroller import FormattingTagController
 from .shortcutlistform import ShortcutListForm
 from .mediadockmanager import MediaDockManager
 from .servicemanager import ServiceManager
@@ -104,4 +105,4 @@
     'ThemeManager', 'MediaDockManager', 'ServiceItemEditForm', 'FirstTimeForm', 'FirstTimeLanguageForm', 'ThemeForm',
     'ThemeLayoutForm', 'FileRenameForm', 'StartTimeForm', 'MainDisplay', 'Display', 'ServiceNoteForm',
     'SlideController', 'DisplayController', 'GeneralTab', 'ThemesTab', 'AdvancedTab', 'PluginForm',
-    'FormattingTagForm', 'ShortcutListForm']
+    'FormattingTagForm', 'ShortcutListForm', 'FormattingTagController']

=== added file 'openlp/core/ui/formattingtagcontroller.py'
--- openlp/core/ui/formattingtagcontroller.py	1970-01-01 00:00:00 +0000
+++ openlp/core/ui/formattingtagcontroller.py	2013-09-28 20:56:43 +0000
@@ -0,0 +1,176 @@
+# -*- coding: utf-8 -*-
+# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
+
+###############################################################################
+# OpenLP - Open Source Lyrics Projection                                      #
+# --------------------------------------------------------------------------- #
+# Copyright (c) 2008-2013 Raoul Snyman                                        #
+# Portions copyright (c) 2008-2013 Tim Bentley, Gerald Britton, Jonathan      #
+# Corwin, Samuel Findlay, Michael Gorven, Scott Guerrieri, Matthias Hub,      #
+# Meinert Jordan, Armin Köhler, Erik Lundin, Edwin Lunando, Brian T. Meyer.   #
+# Joshua Miller, Stevan Pettit, Andreas Preikschat, Mattias Põldaru,          #
+# Christian Richter, Philip Ridout, Simon Scudder, Jeffrey Smith,             #
+# Maikel Stuivenberg, Martin Thompson, Jon Tibble, Dave Warnock,              #
+# Frode Woldsund, Martin Zibricky, Patrick Zimmermann                         #
+# --------------------------------------------------------------------------- #
+# This program is free software; you can redistribute it and/or modify it     #
+# under the terms of the GNU General Public License as published by the Free  #
+# Software Foundation; version 2 of the License.                              #
+#                                                                             #
+# This program is distributed in the hope that it will be useful, but WITHOUT #
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY 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, write to the Free Software Foundation, Inc., 59  #
+# Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
+###############################################################################
+"""
+The :mod:`formattingtagform` provides an Tag Edit facility. The Base set are protected and included each time loaded.
+Custom tags can be defined and saved. The Custom Tag arrays are saved in a pickle so QSettings works on them. Base Tags
+cannot be changed.
+"""
+
+import re
+
+from openlp.core.lib import FormattingTags, translate
+
+
+class FormattingTagController(object):
+    """
+    The :class:`FormattingTagController` manages the non UI functions .
+    """
+    def __init__(self):
+        """
+        Initiator
+        """
+        self.html_tag_regex = re.compile(r'<(?:(?P<close>/(?=[^\s/>]+>))?'
+        r'(?P<tag>[^\s/!\?>]+)(?:\s+[^\s=]+="[^"]*")*\s*(?P<empty>/)?'
+        r'|(?P<cdata>!\[CDATA\[(?:(?!\]\]>).)*\]\])'
+        r'|(?P<procinst>\?(?:(?!\?>).)*\?)'
+        r'|(?P<comment>!--(?:(?!-->).)*--))>', re.UNICODE)
+        self.html_regex = re.compile(r'^(?:[^<>]*%s)*[^<>]*$' % self.html_tag_regex.pattern)
+
+    def pre_save(self):
+        """
+        Cleanup the array before save validation runs
+        """
+        self.protected_tags = [tag for tag in FormattingTags.html_expands if tag.get('protected')]
+        self.custom_tags = []
+
+    def validate_for_save(self, desc, tag, start_html, end_html):
+        """
+        Validate a custom tag and add to the tags array if valid..
+
+        `desc`
+            Explanation of the tag.
+
+        `tag`
+            The tag in the song used to mark the text.
+
+        `start_html`
+            The start html tag.
+
+        `end_html`
+            The end html tag.
+
+        """
+        for linenumber, html1 in enumerate(self.protected_tags):
+            if self._strip(html1[u'start tag']) == tag:
+                return translate('OpenLP.FormattingTagForm', 'Tag %s already defined.') % tag
+            if self._strip(html1[u'desc']) == desc:
+                return translate('OpenLP.FormattingTagForm', 'Description %s already defined.') % tag
+        for linenumber, html1 in enumerate(self.custom_tags):
+            if self._strip(html1[u'start tag']) == tag:
+                return translate('OpenLP.FormattingTagForm', 'Tag %s already defined.') % tag
+            if self._strip(html1[u'desc']) == desc:
+                return translate('OpenLP.FormattingTagForm', 'Description %s already defined.') % tag
+        tag = {
+            'desc': desc,
+            'start tag': '{%s}' % tag,
+            'start html': start_html,
+            'end tag': '{/%s}' % tag,
+            'end html': end_html,
+            'protected': False,
+            'temporary': False
+        }
+        self.custom_tags.append(tag)
+
+    def save_tags(self):
+        """
+        Save the new tags if they are valid.
+        """
+        FormattingTags.save_html_tags(self.custom_tags)
+        FormattingTags.load_tags()
+
+    def _strip(self, tag):
+        """
+        Remove tag wrappers for editing.
+
+        `tag`
+            Tag to be stripped
+        """
+        tag = tag.replace('{', '')
+        tag = tag.replace('}', '')
+        return tag
+
+    def start_html_to_end_html(self, start_html):
+        """
+        Return the end HTML for a given start HTML or None if invalid.
+
+        `start_html`
+            The start html tag.
+
+        """
+        end_tags = []
+        match = self.html_regex.match(start_html)
+        if match:
+            match = self.html_tag_regex.search(start_html)
+            while match:
+                if match.group('tag'):
+                    tag = match.group('tag').lower()
+                    if match.group('close'):
+                        if match.group('empty') or not end_tags or end_tags.pop() != tag:
+                            return
+                    elif not match.group('empty'):
+                        end_tags.append(tag)
+                match = self.html_tag_regex.search(start_html, match.end())
+            return u''.join(map(lambda tag: '</%s>' % tag, reversed(end_tags)))
+
+    def start_tag_changed(self, start_html, end_html):
+        """
+        Validate the HTML tags when the start tag has been changed.
+
+        `start_html`
+            The start html tag.
+
+        `end_html`
+            The end html tag.
+
+        """
+        end = self.start_html_to_end_html(start_html)
+        if not end_html:
+            if not end:
+                return translate('OpenLP.FormattingTagForm', 'Start tag %s is not valid HTML' % start_html), None
+            return None, end
+        return None, None
+
+    def end_tag_changed(self, start_html, end_html):
+        """
+        Validate the HTML tags when the end tag has been changed.
+
+        `start_html`
+            The start html tag.
+
+        `end_html`
+            The end html tag.
+
+        """
+        end = self.start_html_to_end_html(start_html)
+        if not end_html:
+            return None, end
+        if end and end != end_html:
+            return translate('OpenLP.FormattingTagForm',
+                'End tag %s does not match end tag for start tag %s' % (end, start_html)), None
+        return None, None
\ No newline at end of file

=== modified file 'openlp/core/ui/formattingtagdialog.py'
--- openlp/core/ui/formattingtagdialog.py	2013-08-31 18:17:38 +0000
+++ openlp/core/ui/formattingtagdialog.py	2013-09-28 20:56:43 +0000
@@ -31,7 +31,7 @@
 """
 from PyQt4 import QtCore, QtGui
 
-from openlp.core.lib import UiStrings, translate
+from openlp.core.lib import UiStrings, translate, build_icon
 from openlp.core.lib.ui import create_button_box
 
 
@@ -45,12 +45,34 @@
         """
         formatting_tag_dialog.setObjectName('formatting_tag_dialog')
         formatting_tag_dialog.resize(725, 548)
-        self.list_data_grid_layout = QtGui.QGridLayout(formatting_tag_dialog)
+        self.list_data_grid_layout = QtGui.QVBoxLayout(formatting_tag_dialog)
         self.list_data_grid_layout.setMargin(8)
         self.list_data_grid_layout.setObjectName('list_data_grid_layout')
+        self.tag_table_widget_read_label = QtGui.QLabel()
+        self.list_data_grid_layout.addWidget(self.tag_table_widget_read_label)
+        self.tag_table_widget_read = QtGui.QTableWidget(formatting_tag_dialog)
+        self.tag_table_widget_read.setHorizontalScrollBarPolicy(QtCore.Qt.ScrollBarAlwaysOff)
+        self.tag_table_widget_read.setEditTriggers(QtGui.QAbstractItemView.NoEditTriggers)
+        self.tag_table_widget_read.setAlternatingRowColors(True)
+        self.tag_table_widget_read.setCornerButtonEnabled(False)
+        self.tag_table_widget_read.setObjectName('tag_table_widget_read')
+        self.tag_table_widget_read.setColumnCount(4)
+        self.tag_table_widget_read.setRowCount(0)
+        self.tag_table_widget_read.horizontalHeader().setStretchLastSection(True)
+        item = QtGui.QTableWidgetItem()
+        self.tag_table_widget_read.setHorizontalHeaderItem(0, item)
+        item = QtGui.QTableWidgetItem()
+        self.tag_table_widget_read.setHorizontalHeaderItem(1, item)
+        item = QtGui.QTableWidgetItem()
+        self.tag_table_widget_read.setHorizontalHeaderItem(2, item)
+        item = QtGui.QTableWidgetItem()
+        self.tag_table_widget_read.setHorizontalHeaderItem(3, item)
+        self.list_data_grid_layout.addWidget(self.tag_table_widget_read)
+        self.tag_table_widget_label = QtGui.QLabel()
+        self.list_data_grid_layout.addWidget(self.tag_table_widget_label)
         self.tag_table_widget = QtGui.QTableWidget(formatting_tag_dialog)
         self.tag_table_widget.setHorizontalScrollBarPolicy(QtCore.Qt.ScrollBarAlwaysOff)
-        self.tag_table_widget.setEditTriggers(QtGui.QAbstractItemView.NoEditTriggers)
+        self.tag_table_widget.setEditTriggers(QtGui.QAbstractItemView.AllEditTriggers)
         self.tag_table_widget.setAlternatingRowColors(True)
         self.tag_table_widget.setSelectionMode(QtGui.QAbstractItemView.SingleSelection)
         self.tag_table_widget.setSelectionBehavior(QtGui.QAbstractItemView.SelectRows)
@@ -67,59 +89,26 @@
         self.tag_table_widget.setHorizontalHeaderItem(2, item)
         item = QtGui.QTableWidgetItem()
         self.tag_table_widget.setHorizontalHeaderItem(3, item)
-        self.list_data_grid_layout.addWidget(self.tag_table_widget, 0, 0, 1, 1)
-        self.horizontal_layout = QtGui.QHBoxLayout()
-        self.horizontal_layout.setObjectName('horizontal_layout')
-        spacer_item = QtGui.QSpacerItem(40, 20, QtGui.QSizePolicy.Expanding, QtGui.QSizePolicy.Minimum)
-        self.horizontal_layout.addItem(spacer_item)
-        self.delete_push_button = QtGui.QPushButton(formatting_tag_dialog)
-        self.delete_push_button.setObjectName('delete_push_button')
-        self.horizontal_layout.addWidget(self.delete_push_button)
-        self.list_data_grid_layout.addLayout(self.horizontal_layout, 1, 0, 1, 1)
-        self.edit_group_box = QtGui.QGroupBox(formatting_tag_dialog)
-        self.edit_group_box.setObjectName('edit_group_box')
-        self.data_grid_layout = QtGui.QGridLayout(self.edit_group_box)
-        self.data_grid_layout.setObjectName('data_grid_layout')
-        self.description_label = QtGui.QLabel(self.edit_group_box)
-        self.description_label.setAlignment(QtCore.Qt.AlignCenter)
-        self.description_label.setObjectName('description_label')
-        self.data_grid_layout.addWidget(self.description_label, 0, 0, 1, 1)
-        self.description_line_edit = QtGui.QLineEdit(self.edit_group_box)
-        self.description_line_edit.setObjectName('description_line_edit')
-        self.data_grid_layout.addWidget(self.description_line_edit, 0, 1, 2, 1)
-        self.new_push_button = QtGui.QPushButton(self.edit_group_box)
-        self.new_push_button.setObjectName('new_push_button')
-        self.data_grid_layout.addWidget(self.new_push_button, 0, 2, 2, 1)
-        self.tag_label = QtGui.QLabel(self.edit_group_box)
-        self.tag_label.setAlignment(QtCore.Qt.AlignCenter)
-        self.tag_label.setObjectName('tag_label')
-        self.data_grid_layout.addWidget(self.tag_label, 2, 0, 1, 1)
-        self.tag_line_edit = QtGui.QLineEdit(self.edit_group_box)
-        self.tag_line_edit.setMaximumSize(QtCore.QSize(50, 16777215))
-        self.tag_line_edit.setMaxLength(5)
-        self.tag_line_edit.setObjectName('tag_line_edit')
-        self.data_grid_layout.addWidget(self.tag_line_edit, 2, 1, 1, 1)
-        self.start_tag_label = QtGui.QLabel(self.edit_group_box)
-        self.start_tag_label.setAlignment(QtCore.Qt.AlignCenter)
-        self.start_tag_label.setObjectName('start_tag_label')
-        self.data_grid_layout.addWidget(self.start_tag_label, 3, 0, 1, 1)
-        self.start_tag_line_edit = QtGui.QLineEdit(self.edit_group_box)
-        self.start_tag_line_edit.setObjectName('start_tag_line_edit')
-        self.data_grid_layout.addWidget(self.start_tag_line_edit, 3, 1, 1, 1)
-        self.end_tag_label = QtGui.QLabel(self.edit_group_box)
-        self.end_tag_label.setAlignment(QtCore.Qt.AlignCenter)
-        self.end_tag_label.setObjectName('end_tag_label')
-        self.data_grid_layout.addWidget(self.end_tag_label, 4, 0, 1, 1)
-        self.end_tag_line_edit = QtGui.QLineEdit(self.edit_group_box)
-        self.end_tag_line_edit.setObjectName('end_tag_line_edit')
-        self.data_grid_layout.addWidget(self.end_tag_line_edit, 4, 1, 1, 1)
-        self.save_push_button = QtGui.QPushButton(self.edit_group_box)
-        self.save_push_button.setObjectName('save_push_button')
-        self.data_grid_layout.addWidget(self.save_push_button, 4, 2, 1, 1)
-        self.list_data_grid_layout.addWidget(self.edit_group_box, 2, 0, 1, 1)
-        self.button_box = create_button_box(formatting_tag_dialog, 'button_box', ['close'])
-        self.list_data_grid_layout.addWidget(self.button_box, 3, 0, 1, 1)
-
+        self.list_data_grid_layout.addWidget(self.tag_table_widget)
+        self.edit_button_layout = QtGui.QHBoxLayout()
+        self.new_button = QtGui.QPushButton(formatting_tag_dialog)
+        self.new_button.setIcon(build_icon(':/general/general_new.png'))
+        self.new_button.setObjectName('new_button')
+        self.edit_button_layout.addWidget(self.new_button)
+        self.delete_button = QtGui.QPushButton(formatting_tag_dialog)
+        self.delete_button.setIcon(build_icon(':/general/general_delete.png'))
+        self.delete_button.setObjectName('delete_button')
+        self.edit_button_layout.addWidget(self.delete_button)
+        self.edit_button_layout.addStretch()
+        self.list_data_grid_layout.addLayout(self.edit_button_layout)
+        self.button_box = create_button_box(formatting_tag_dialog, 'button_box',
+            ['cancel', 'save', 'defaults'])
+        self.save_button = self.button_box.button(QtGui.QDialogButtonBox.Save)
+        self.save_button.setObjectName('save_button')
+        self.restore_button = self.button_box.button(QtGui.QDialogButtonBox.RestoreDefaults)
+        self.restore_button.setIcon(build_icon(':/general/general_revert.png'))
+        self.restore_button.setObjectName('restore_button')
+        self.list_data_grid_layout.addWidget(self.button_box)
         self.retranslateUi(formatting_tag_dialog)
 
     def retranslateUi(self, formatting_tag_dialog):
@@ -127,14 +116,19 @@
         Translate the UI on the fly
         """
         formatting_tag_dialog.setWindowTitle(translate('OpenLP.FormattingTagDialog', 'Configure Formatting Tags'))
-        self.edit_group_box.setTitle(translate('OpenLP.FormattingTagDialog', 'Edit Selection'))
-        self.save_push_button.setText(translate('OpenLP.FormattingTagDialog', 'Save'))
-        self.description_label.setText(translate('OpenLP.FormattingTagDialog', 'Description'))
-        self.tag_label.setText(translate('OpenLP.FormattingTagDialog', 'Tag'))
-        self.start_tag_label.setText(translate('OpenLP.FormattingTagDialog', 'Start HTML'))
-        self.end_tag_label.setText(translate('OpenLP.FormattingTagDialog', 'End HTML'))
-        self.delete_push_button.setText(UiStrings().Delete)
-        self.new_push_button.setText(UiStrings().New)
+        self.delete_button.setText(UiStrings().Delete)
+        self.new_button.setText(UiStrings().New)
+        self.tag_table_widget_read_label.setText(translate('OpenLP.FormattingTagDialog', 'Default Formatting'))
+        self.tag_table_widget_read.horizontalHeaderItem(0).\
+            setText(translate('OpenLP.FormattingTagDialog', 'Description'))
+        self.tag_table_widget_read.horizontalHeaderItem(1).setText(translate('OpenLP.FormattingTagDialog', 'Tag'))
+        self.tag_table_widget_read.horizontalHeaderItem(2).\
+            setText(translate('OpenLP.FormattingTagDialog', 'Start HTML'))
+        self.tag_table_widget_read.horizontalHeaderItem(3).setText(translate('OpenLP.FormattingTagDialog', 'End HTML'))
+        self.tag_table_widget_read.setColumnWidth(0, 120)
+        self.tag_table_widget_read.setColumnWidth(1, 80)
+        self.tag_table_widget_read.setColumnWidth(2, 330)
+        self.tag_table_widget_label.setText(translate('OpenLP.FormattingTagDialog', 'Custom Formatting'))
         self.tag_table_widget.horizontalHeaderItem(0).setText(translate('OpenLP.FormattingTagDialog', 'Description'))
         self.tag_table_widget.horizontalHeaderItem(1).setText(translate('OpenLP.FormattingTagDialog', 'Tag'))
         self.tag_table_widget.horizontalHeaderItem(2).setText(translate('OpenLP.FormattingTagDialog', 'Start HTML'))

=== modified file 'openlp/core/ui/formattingtagform.py'
--- openlp/core/ui/formattingtagform.py	2013-08-31 18:17:38 +0000
+++ openlp/core/ui/formattingtagform.py	2013-09-28 20:56:43 +0000
@@ -31,14 +31,25 @@
 Custom tags can be defined and saved. The Custom Tag arrays are saved in a json string so QSettings works on them.
 Base Tags cannot be changed.
 """
+
 from PyQt4 import QtGui
 
 from openlp.core.lib import FormattingTags, translate
-from openlp.core.lib.ui import critical_error_message_box
 from openlp.core.ui.formattingtagdialog import Ui_FormattingTagDialog
-
-
-class FormattingTagForm(QtGui.QDialog, Ui_FormattingTagDialog):
+from openlp.core.ui.formattingtagcontroller import FormattingTagController
+
+
+class EditColumn(object):
+    """
+    Hides the magic numbers for the table columns
+    """
+    Description = 0
+    Tag = 1
+    StartHtml = 2
+    EndHtml = 3
+
+
+class FormattingTagForm(QtGui.QDialog, Ui_FormattingTagDialog, FormattingTagController):
     """
     The :class:`FormattingTagForm` manages the settings tab .
     """
@@ -48,17 +59,17 @@
         """
         super(FormattingTagForm, self).__init__(parent)
         self.setupUi(self)
+        self.services = FormattingTagController()
         self.tag_table_widget.itemSelectionChanged.connect(self.on_row_selected)
-        self.new_push_button.clicked.connect(self.on_new_clicked)
-        self.save_push_button.clicked.connect(self.on_saved_clicked)
-        self.delete_push_button.clicked.connect(self.on_delete_clicked)
+        self.new_button.clicked.connect(self.on_new_clicked)
+        #self.save_button.clicked.connect(self.on_saved_clicked)
+        self.delete_button.clicked.connect(self.on_delete_clicked)
+        self.tag_table_widget.currentCellChanged.connect(self.on_current_cell_changed)
         self.button_box.rejected.connect(self.close)
-        self.description_line_edit.textEdited.connect(self.on_text_edited)
-        self.tag_line_edit.textEdited.connect(self.on_text_edited)
-        self.start_tag_line_edit.textEdited.connect(self.on_text_edited)
-        self.end_tag_line_edit.textEdited.connect(self.on_text_edited)
         # Forces reloading of tags from openlp configuration.
         FormattingTags.load_tags()
+        self.is_deleting = False
+        self.reloading = False
 
     def exec_(self):
         """
@@ -66,138 +77,128 @@
         """
         # Create initial copy from master
         self._reloadTable()
-        self.selected = -1
         return QtGui.QDialog.exec_(self)
 
     def on_row_selected(self):
         """
         Table Row selected so display items and set field state.
         """
-        self.save_push_button.setEnabled(False)
-        self.selected = self.tag_table_widget.currentRow()
-        html = FormattingTags.get_html_tags()[self.selected]
-        self.description_line_edit.setText(html['desc'])
-        self.tag_line_edit.setText(self._strip(html['start tag']))
-        self.start_tag_line_edit.setText(html['start html'])
-        self.end_tag_line_edit.setText(html['end html'])
-        if html['protected']:
-            self.description_line_edit.setEnabled(False)
-            self.tag_line_edit.setEnabled(False)
-            self.start_tag_line_edit.setEnabled(False)
-            self.end_tag_line_edit.setEnabled(False)
-            self.delete_push_button.setEnabled(False)
-        else:
-            self.description_line_edit.setEnabled(True)
-            self.tag_line_edit.setEnabled(True)
-            self.start_tag_line_edit.setEnabled(True)
-            self.end_tag_line_edit.setEnabled(True)
-            self.delete_push_button.setEnabled(True)
-
-    def on_text_edited(self, text):
-        """
-        Enable the ``save_push_button`` when any of the selected tag's properties
-        has been changed.
-        """
-        self.save_push_button.setEnabled(True)
+        self.delete_button.setEnabled(True)
 
     def on_new_clicked(self):
         """
-        Add a new tag to list only if it is not a duplicate.
+        Add a new tag to edit list and select it for editing.
         """
-        for html in FormattingTags.get_html_tags():
-            if self._strip(html['start tag']) == 'n':
-                critical_error_message_box(
-                    translate('OpenLP.FormattingTagForm', 'Update Error'),
-                    translate('OpenLP.FormattingTagForm', 'Tag "n" already defined.'))
-                return
-        # Add new tag to list
-        tag = {
-            'desc': translate('OpenLP.FormattingTagForm', 'New Tag'),
-            'start tag': '{n}',
-            'start html': translate('OpenLP.FormattingTagForm', '<HTML here>'),
-            'end tag': '{/n}',
-            'end html': translate('OpenLP.FormattingTagForm', '</and here>'),
-            'protected': False,
-            'temporary': False
-        }
-        FormattingTags.add_html_tags([tag])
-        FormattingTags.save_html_tags()
-        self._reloadTable()
-        # Highlight new row
-        self.tag_table_widget.selectRow(self.tag_table_widget.rowCount() - 1)
-        self.on_row_selected()
+        new_row = self.tag_table_widget.rowCount()
+        self.tag_table_widget.insertRow(new_row)
+        self.tag_table_widget.setItem(new_row, 0,
+            QtGui.QTableWidgetItem(translate('OpenLP.FormattingTagForm', 'New Tag%s') % str(new_row)))
+        self.tag_table_widget.setItem(new_row, 1, QtGui.QTableWidgetItem('n%s' % str(new_row)))
+        self.tag_table_widget.setItem(new_row, 2,
+            QtGui.QTableWidgetItem(translate('OpenLP.FormattingTagForm', '<HTML here>')))
+        self.tag_table_widget.setItem(new_row, 3, QtGui.QTableWidgetItem(''))
+        self.tag_table_widget.resizeRowsToContents()
         self.tag_table_widget.scrollToBottom()
+        self.tag_table_widget.selectRow(new_row)
 
     def on_delete_clicked(self):
         """
-        Delete selected custom tag.
+        Delete selected custom row.
         """
-        if self.selected != -1:
-            FormattingTags.remove_html_tag(self.selected)
-            # As the first items are protected we should not have to take care
-            # of negative indexes causing tracebacks.
-            self.tag_table_widget.selectRow(self.selected - 1)
-            self.selected = -1
-            FormattingTags.save_html_tags()
-            self._reloadTable()
+        selected = self.tag_table_widget.currentRow()
+        if selected != -1:
+            self.is_deleting = True
+            self.tag_table_widget.removeRow(selected)
 
-    def on_saved_clicked(self):
+    def accept(self):
         """
         Update Custom Tag details if not duplicate and save the data.
         """
-        html_expands = FormattingTags.get_html_tags()
-        if self.selected != -1:
-            html = html_expands[self.selected]
-            tag = self.tag_line_edit.text()
-            for linenumber, html1 in enumerate(html_expands):
-                if self._strip(html1['start tag']) == tag and linenumber != self.selected:
-                    critical_error_message_box(
-                        translate('OpenLP.FormattingTagForm', 'Update Error'),
-                        translate('OpenLP.FormattingTagForm', 'Tag %s already defined.') % tag)
-                    return
-            html['desc'] = self.description_line_edit.text()
-            html['start html'] = self.start_tag_line_edit.text()
-            html['end html'] = self.end_tag_line_edit.text()
-            html['start tag'] = '{%s}' % tag
-            html['end tag'] = '{/%s}' % tag
-            # Keep temporary tags when the user changes one.
-            html['temporary'] = False
-            self.selected = -1
-        FormattingTags.save_html_tags()
-        self._reloadTable()
+        count = 0
+        self.services.pre_save()
+        while count < self.tag_table_widget.rowCount():
+            error = self.services.validate_for_save(self.tag_table_widget.item(count, 0).text(),
+                self.tag_table_widget.item(count, 1).text(), self.tag_table_widget.item(count, 2).text(),
+                self.tag_table_widget.item(count, 3).text())
+            if error:
+                QtGui.QMessageBox.warning(self,
+                    translate('OpenLP.FormattingTagForm', 'Validation Error'), error, QtGui.QMessageBox.Ok)
+                self.tag_table_widget.selectRow(count)
+                return
+            count += 1
+        self.services.save_tags()
+        QtGui.QDialog.accept(self)
 
     def _reloadTable(self):
         """
         Reset List for loading.
         """
+        self.reloading = True
+        self.tag_table_widget_read.clearContents()
+        self.tag_table_widget_read.setRowCount(0)
         self.tag_table_widget.clearContents()
         self.tag_table_widget.setRowCount(0)
-        self.new_push_button.setEnabled(True)
-        self.save_push_button.setEnabled(False)
-        self.delete_push_button.setEnabled(False)
+        self.new_button.setEnabled(True)
+        self.delete_button.setEnabled(False)
         for linenumber, html in enumerate(FormattingTags.get_html_tags()):
-            self.tag_table_widget.setRowCount(self.tag_table_widget.rowCount() + 1)
-            self.tag_table_widget.setItem(linenumber, 0, QtGui.QTableWidgetItem(html['desc']))
-            self.tag_table_widget.setItem(linenumber, 1, QtGui.QTableWidgetItem(self._strip(html['start tag'])))
-            self.tag_table_widget.setItem(linenumber, 2, QtGui.QTableWidgetItem(html['start html']))
-            self.tag_table_widget.setItem(linenumber, 3, QtGui.QTableWidgetItem(html['end html']))
-            # Permanent (persistent) tags do not have this key.
-            if 'temporary' not in html:
-                html['temporary'] = False
+            if html[u'protected']:
+                line = self.tag_table_widget_read.rowCount()
+                self.tag_table_widget_read.setRowCount(line + 1)
+                self.tag_table_widget_read.setItem(line, 0, QtGui.QTableWidgetItem(html[u'desc']))
+                self.tag_table_widget_read.setItem(line, 1, QtGui.QTableWidgetItem(self._strip(html[u'start tag'])))
+                self.tag_table_widget_read.setItem(line, 2, QtGui.QTableWidgetItem(html[u'start html']))
+                self.tag_table_widget_read.setItem(line, 3, QtGui.QTableWidgetItem(html[u'end html']))
+                self.tag_table_widget_read.resizeRowsToContents()
+            else:
+                line = self.tag_table_widget.rowCount()
+                self.tag_table_widget.setRowCount(line + 1)
+                self.tag_table_widget.setItem(line, 0, QtGui.QTableWidgetItem(html[u'desc']))
+                self.tag_table_widget.setItem(line, 1, QtGui.QTableWidgetItem(self._strip(html[u'start tag'])))
+                self.tag_table_widget.setItem(line, 2, QtGui.QTableWidgetItem(html[u'start html']))
+                self.tag_table_widget.setItem(line, 3, QtGui.QTableWidgetItem(html[u'end html']))
+                self.tag_table_widget.resizeRowsToContents()
+                # Permanent (persistent) tags do not have this key
+                html[u'temporary'] = False
+        self.reloading = False
+
+    def on_current_cell_changed(self, cur_row, cur_col, pre_row, pre_col):
+        """
+        This function processes all user edits in the table. It is called on each cell change.
+        """
+        if self.is_deleting:
+            self.is_deleting = False
+            return
+        if self.reloading:
+            return
+        # only process for editable rows
+        if self.tag_table_widget.item(pre_row, 0):
+            item = self.tag_table_widget.item(pre_row, pre_col)
+            text = item.text()
+            errors = None
+            if pre_col is EditColumn.Description:
+                if not text:
+                    errors = translate('OpenLP.FormattingTagForm', 'Description is missing')
+            elif pre_col is EditColumn.Tag:
+                if not text:
+                    errors = translate('OpenLP.FormattingTagForm', 'Tag is missing')
+            elif pre_col is EditColumn.StartHtml:
+                # HTML edited
+                item = self.tag_table_widget.item(pre_row, 3)
+                end_html = item.text()
+                errors, tag = self.services.start_tag_changed(text, end_html)
+                if tag:
+                    self.tag_table_widget.setItem(pre_row, 3, QtGui.QTableWidgetItem(tag))
+                self.tag_table_widget.resizeRowsToContents()
+            elif pre_col is EditColumn.EndHtml:
+                # HTML edited
+                item = self.tag_table_widget.item(pre_row, 2)
+                start_html = item.text()
+                errors, tag = self.services.end_tag_changed(start_html, text)
+                if tag:
+                    self.tag_table_widget.setItem(pre_row, 3, QtGui.QTableWidgetItem(tag))
+            if errors:
+                QtGui.QMessageBox.warning(self,
+                    translate('OpenLP.FormattingTagForm', 'Validation Error'), errors, QtGui.QMessageBox.Ok)
+            #self.tag_table_widget.selectRow(pre_row - 1)
             self.tag_table_widget.resizeRowsToContents()
-        self.description_line_edit.setText('')
-        self.tag_line_edit.setText('')
-        self.start_tag_line_edit.setText('')
-        self.end_tag_line_edit.setText('')
-        self.description_line_edit.setEnabled(False)
-        self.tag_line_edit.setEnabled(False)
-        self.start_tag_line_edit.setEnabled(False)
-        self.end_tag_line_edit.setEnabled(False)
 
-    def _strip(self, tag):
-        """
-        Remove tag wrappers for editing.
-        """
-        tag = tag.replace('{', '')
-        tag = tag.replace('}', '')
-        return tag

=== modified file 'openlp/plugins/songs/forms/editsongform.py'
--- openlp/plugins/songs/forms/editsongform.py	2013-08-31 18:17:38 +0000
+++ openlp/plugins/songs/forms/editsongform.py	2013-09-28 20:56:43 +0000
@@ -692,7 +692,6 @@
             self.verse_edit_button.setEnabled(False)
             self.verse_delete_button.setEnabled(False)
 
-
     def on_verse_order_text_changed(self, text):
         """
         Checks if the verse order is complete or missing. Shows a error message according to the state of the verse

=== added file 'tests/functional/openlp_core_ui/tests_formattingtagscontroller.py'
--- tests/functional/openlp_core_ui/tests_formattingtagscontroller.py	1970-01-01 00:00:00 +0000
+++ tests/functional/openlp_core_ui/tests_formattingtagscontroller.py	2013-09-28 20:56:43 +0000
@@ -0,0 +1,88 @@
+"""
+Package to test the openlp.core.ui.formattingtagscontroller package.
+"""
+from unittest import TestCase
+
+from openlp.core.ui import FormattingTagController
+
+
+class TestFormattingTagController(TestCase):
+
+    def setUp(self):
+        self.services = FormattingTagController()
+
+    def test_strip(self):
+        """
+        Test that the _strip strips the correct chars
+        """
+        # GIVEN: An instance of the Formatting Tag Form and a string containing a tag
+        tag = u'{tag}'
+
+        # WHEN: Calling _strip
+        result = self.services._strip(tag)
+
+        # THEN: The tag should be returned with the wrappers removed.
+        self.assertEqual(result, u'tag', u'FormattingTagForm._strip should return u\'tag\' when called with u\'{tag}\'')
+
+    def test_end_tag_changed_processes_correctly(self):
+        """
+        Test that the end html tags are generated correctly
+        """
+        # GIVEN: A list of start , end tags and error messages
+        tests = []
+        test = {'start': '<b>', 'end': None, 'gen': '</b>', 'valid': None}
+        tests.append(test)
+        test = {'start': '<i>', 'end': '</i>', 'gen': None, 'valid': None}
+        tests.append(test)
+        test = {'start': '<b>', 'end': '</i>', 'gen': None,
+                'valid': 'End tag </b> does not match end tag for start tag <b>'}
+        tests.append(test)
+
+        # WHEN: Testing each one of them in turn
+        for test in tests:
+            error, result = self.services.end_tag_changed(test['start'], test['end'])
+
+            # THEN: The result should match the predetermined value.
+            self.assertTrue(result == test['gen'],
+                'Function should handle end tag correctly : %s and %s for %s ' % (test['gen'], result, test['start']))
+            self.assertTrue(error == test['valid'],
+                'Function should not generate unexpected error messages : %s ' % error)
+
+    def test_start_tag_changed_processes_correctly(self):
+        """
+        Test that the end html tags are generated correctly
+        """
+        # GIVEN: A list of start , end tags and error messages
+        tests = []
+        test = {'start': '<b>', 'end': '', 'gen': '</b>', 'valid': None}
+        tests.append(test)
+        test = {'start': '<i>', 'end': '</i>', 'gen': None, 'valid': None}
+        tests.append(test)
+        test = {'start': 'superfly', 'end': '', 'gen': None, 'valid': 'Start tag superfly is not valid HTML'}
+        tests.append(test)
+
+        # WHEN: Testing each one of them in turn
+        for test in tests:
+            error, result = self.services.start_tag_changed(test['start'], test['end'])
+
+            # THEN: The result should match the predetermined value.
+            self.assertTrue(result == test['gen'],
+                'Function should handle end tag correctly : %s and %s ' % (test['gen'], result))
+            self.assertTrue(error == test['valid'],
+                'Function should not generate unexpected error messages : %s ' % error)
+
+    def test_start_html_to_end_html(self):
+        """
+        Test that the end html tags are generated correctly
+        """
+        # GIVEN: A list of valid and invalid tags
+        tests = {'<b>': '</b>', '<i>': '</i>', 'superfly': '', '<HTML START>': None,
+                 '<span style="-webkit-text-fill-color:red">': '</span>'}
+
+        # WHEN: Testing each one of them
+        for test1, test2 in tests.items():
+            result = self.services.start_html_to_end_html(test1)
+
+            # THEN: The result should match the predetermined value.
+            self.assertTrue(result == test2, 'Calculated end tag should be valid: %s and %s = %s'
+                % (test1, test2, result))
\ No newline at end of file

=== added file 'tests/functional/openlp_core_ui/tests_formattingtagsform.py'
--- tests/functional/openlp_core_ui/tests_formattingtagsform.py	1970-01-01 00:00:00 +0000
+++ tests/functional/openlp_core_ui/tests_formattingtagsform.py	2013-09-28 20:56:43 +0000
@@ -0,0 +1,49 @@
+"""
+Package to test the openlp.core.ui.formattingtagsform package.
+"""
+from unittest import TestCase
+
+from mock import MagicMock, patch
+
+from openlp.core.ui.formattingtagform import FormattingTagForm
+
+# TODO: Tests Still TODO
+# __init__
+# exec_
+# on_new_clicked
+# on_delete_clicked
+# on_saved_clicked
+# _reloadTable
+
+
+class TestFormattingTagForm(TestCase):
+
+    def setUp(self):
+        self.init_patcher = patch(u'openlp.core.ui.formattingtagform.FormattingTagForm.__init__')
+        self.qdialog_patcher = patch(u'openlp.core.ui.formattingtagform.QtGui.QDialog')
+        self.ui_formatting_tag_dialog_patcher = patch(u'openlp.core.ui.formattingtagform.Ui_FormattingTagDialog')
+        self.mocked_init = self.init_patcher.start()
+        self.mocked_qdialog = self.qdialog_patcher.start()
+        self.mocked_ui_formatting_tag_dialog = self.ui_formatting_tag_dialog_patcher.start()
+        self.mocked_init.return_value = None
+
+    def tearDown(self):
+        self.init_patcher.stop()
+        self.qdialog_patcher.stop()
+        self.ui_formatting_tag_dialog_patcher.stop()
+
+    def test_on_text_edited(self):
+        """
+        Test that the appropriate actions are preformed when on_text_edited is called
+        """
+
+        # GIVEN: An instance of the Formatting Tag Form and a mocked save_push_button
+        form = FormattingTagForm()
+        form.save_button = MagicMock()
+
+        # WHEN: on_text_edited is called with an arbitrary value
+        #form.on_text_edited('text')
+
+        # THEN: setEnabled and setDefault should have been called on save_push_button
+        #form.save_button.setEnabled.assert_called_with(True)
+


Follow ups