openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #29093
Re: [Merge] lp:~suutari-olli/openlp/combined-bible-quick-search into lp:openlp
Not even bothered to look at. Needs to be fixed as last comment was ignored
which is not acceptable
On 4 Apr 2016 2:14 a.m., "Azaziah" <suutari.olli@xxxxxxxxx> wrote:
> Azaziah has proposed merging
> lp:~suutari-olli/openlp/combined-bible-quick-search into lp:openlp.
>
> Requested reviews:
> Tim Bentley (trb143)
> Tomas Groth (tomasgroth)
>
> For more details, see:
>
> https://code.launchpad.net/~suutari-olli/openlp/combined-bible-quick-search/+merge/290846
>
> Thank you for your review Tim,
>
> I’ve managed to simplify the code structure.
>
> “This needs to be broken down into vers small bits which cannot be broken.
> Look at the about ui for a example.”
> I’m not sure if I understand what you meant with that.
> The example verses need to be translatable.
>
> I agree with you, the string with the scripture reference error is
> already quite horrifying and I can remember how I used to struggle
> with it for the Finnish translation. However, I feel like the
> sample verses make the message much easier to understand.
>
> If we can’t find an alternative solution for this, I propose the following:
> We (I) copy paste the old translation from scripture reference error and
> add localized sample verses by ourselves, thus limiting the possibility of
> translation screw ups. By having the template and just replacing the
> foreign Bible book names to it, it shouldn’t take too long.
>
> Refactored the code for combined search.
> - Added: def on_quick_reference_search(self):
> and moved definition of reference search there.
> - Added: def on_quick_text_search(self):
> and moved definition of text search there.
> - Removed some un-needed code duplicates
> (Double finalizing, 3rd normalizing of mouse cursor)
> - Searching scripture ref with shorter than 3 char search is now possible
> (G1 = Genesis 1)
>
> Also removed “Search” from “Search Text or Reference…”
> since it does not fit the box properly.
>
> "- Noticed I had left an old comment to a wrong place. (Moved it to def
> on_quick_reference_search)"
>
> lp:~suutari-olli/openlp/combined-bible-quick-search (revision 2624)
> [←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-01-Pull/1382/
> [←[1;32mSUCCESS←[1;m]
> https://ci.openlp.io/job/Branch-02-Functional-Tests/1300/
> [←[1;32mSUCCESS←[1;m]
> https://ci.openlp.io/job/Branch-03-Interface-Tests/1239/
> [←[1;32mSUCCESS←[1;m]
> https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1071/
> [←[1;32mSUCCESS←[1;m]
> https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/662/
> [←[1;32mSUCCESS←[1;m]
> https://ci.openlp.io/job/Branch-05a-Code_Analysis/729/
> [←[1;32mSUCCESS←[1;m]
> https://ci.openlp.io/job/Branch-05b-Test_Coverage/597/
>
> ------------------------------------------------------------------------------
> This branch introduces following improvements to Quick Bible search:
> - Combined Reference/Text search which first performs the Reference
> search and then moves to Text search if nothing is found.
> - Possibility to use “.” when shortening Book names in Reference search.
> For an example Gen. 1 = Gen 1 = Genesis 1.
> - New/Improved error messages (E.g. added actual example verses
> to Reference error)
> (Parts of the new messages are Bolded so <br> is required since
> \n does not work with bolding)
>
> This branch also prevents users from performing Text searches which are:
> - Shorter than 3 characters long (not including spaces)
> - Searches consisting from only spaces
>
> These currently possible bad search quarries result in LONG search times
> and program instability/crashing. It’s still possible to search 3
> characters
> separated by spaces, but that scenario is relatively rarer.
> --
> You are requested to review the proposed merge of
> lp:~suutari-olli/openlp/combined-bible-quick-search into lp:openlp.
>
> === modified file 'openlp/core/common/uistrings.py'
> --- openlp/core/common/uistrings.py 2016-01-23 08:15:37 +0000
> +++ openlp/core/common/uistrings.py 2016-04-04 01:13:54 +0000
> @@ -151,3 +151,22 @@
> self.Version = translate('OpenLP.Ui', 'Version')
> self.View = translate('OpenLP.Ui', 'View')
> self.ViewMode = translate('OpenLP.Ui', 'View Mode')
> + self.BibleShortSearchTitle = translate('OpenLP.Ui', 'Search is
> Empty or too Short')
> + self.BibleScriptureError = translate('OpenLP.Ui', '<br><br>Book
> Chapter | John 3:16<br>'
> + 'Book
> Chapter%(range)sChapter | John 3%(range)s4<br>'
> + 'Book
> Chapter%(verse)sVerse%(range)sVerse | John 3%(verse)'
> +
> 's16%(range)s17<br>Book Chapter%(verse)sVerse%(range)sVerse%'
> +
> '(list)sVerse%(range)sVerse | John 3%(verse)s16-17%(list)s20%'
> +
> '(range)s22<br>Book Chapter%(verse)sVerse%(range)sVerse%'
> +
> '(list)sChapter%(verse)sVerse%(range)sVerse | John 3%(verse)'
> +
> 's16%(range)s17%(list)s5%(verse)s7%(range)s9<br>Book Chapter%'
> +
> '(verse)sVerse%(range)sChapter%(verse)sVerse | John 3%(verse)'
> +
> 's16%(range)s4%(verse)s2<br><br> Book names may be shortened '
> + 'from full
> names, for an example: Joh 3 = John 3',
> + 'Please pay attention to the
> appended "s" of the wildcards and refrain '
> + 'from translating the words
> inside the names in the brackets.')
> + self.BibleShortSearch = translate('OpenLP.Ui', '<strong>The
> search you have entered is empty or shorter '
> + 'than 3 characters
> long.<br>Please try again with '
> + 'a longer
> search.</strong><br><br>You can separate different '
> + 'keywords by a
> space to search for all of your keywords and you '
> + 'can separate them
> by a comma to search for one of them.')
>
> === modified file 'openlp/plugins/bibles/lib/manager.py'
> --- openlp/plugins/bibles/lib/manager.py 2015-12-31 22:46:06 +0000
> +++ openlp/plugins/bibles/lib/manager.py 2016-04-04 01:13:54 +0000
> @@ -23,7 +23,7 @@
> import logging
> import os
>
> -from openlp.core.common import RegistryProperties, AppLocation, Settings,
> translate
> +from openlp.core.common import RegistryProperties, AppLocation, Settings,
> UiStrings, translate
> from openlp.core.utils import delete_file
> from openlp.plugins.bibles.lib import parse_reference,
> get_reference_separator, LanguageSelection
> from openlp.plugins.bibles.lib.db import BibleDB, BibleMeta
> @@ -279,21 +279,10 @@
> 'list': get_reference_separator('sep_l_display')}
> self.main_window.information_message(
> translate('BiblesPlugin.BibleManager', 'Scripture
> Reference Error'),
> - translate('BiblesPlugin.BibleManager', 'Your
> scripture reference is either not supported by '
> - 'OpenLP or is invalid. Please make sure
> your reference '
> - 'conforms to one of the following patterns
> or consult the manual:\n\n'
> - 'Book Chapter\n'
> - 'Book Chapter%(range)sChapter\n'
> - 'Book Chapter%(verse)sVerse%(range)sVerse\n'
> - 'Book
> Chapter%(verse)sVerse%(range)sVerse%(list)sVerse'
> - '%(range)sVerse\n'
> - 'Book
> Chapter%(verse)sVerse%(range)sVerse%(list)sChapter'
> - '%(verse)sVerse%(range)sVerse\n'
> - 'Book
> Chapter%(verse)sVerse%(range)sChapter%(verse)sVerse',
> - 'Please pay attention to the appended "s"
> of the wildcards '
> - 'and refrain from translating the words
> inside the names in the brackets.')
> - % reference_separators
> - )
> + translate('BiblesPlugin.BibleManager',
> '<strong>OpenLP couldn’t find anything '
> + 'with your
> search.<br><br>'
> + 'Please make sure that your reference
> follows one of these patterns:</strong>%s'
> + % UiStrings().BibleScriptureError %
> reference_separators))
> return None
>
> def get_language_selection(self, bible):
> @@ -339,22 +328,40 @@
> if second_bible:
> second_web_bible =
> self.db_cache[second_bible].get_object(BibleMeta, 'download_source')
> if web_bible or second_web_bible:
> + self.application.set_normal_cursor()
> self.main_window.information_message(
> translate('BiblesPlugin.BibleManager', 'Web Bible cannot
> be used'),
> - translate('BiblesPlugin.BibleManager', 'Text Search is
> not available with Web Bibles.')
> + translate('BiblesPlugin.BibleManager', 'Text Search is
> not available with Web Bibles.\n'
> + 'Please use the
> Scripture Reference Search instead.')
> )
> return None
> - if text:
> + if len(text) - text.count(' ') < 3:
> + self.main_window.information_message(
> + ('%s' % UiStrings().BibleShortSearchTitle),
> + ('%s' % UiStrings().BibleShortSearch))
> + return None
> + elif text:
> return self.db_cache[bible].verse_search(text)
> else:
> - self.main_window.information_message(
> - translate('BiblesPlugin.BibleManager', 'Scripture
> Reference Error'),
> - translate('BiblesPlugin.BibleManager', 'You did not enter
> a search keyword.\nYou can separate '
> - 'different keywords by a space to search for
> all of your keywords and you can separate '
> - 'them by a comma to search for one of them.')
> - )
> return None
>
> + def get_verses_combined(self, bible, verse_text, book_ref_id=False,
> show_error=False):
> + log.debug('BibleManager.get_verses("%s", "%s")', bible,
> verse_text)
> + if not bible:
> + if show_error:
> + self.main_window.information_message(
> + translate('BiblesPlugin.BibleManager', 'No Bibles
> Available'),
> + translate('BiblesPlugin.BibleManager', 'There are
> no Bibles currently installed. Please use the'
> + ' Import Wizard to install one or more
> Bibles.')
> + )
> + return None
> + language_selection = self.get_language_selection(bible)
> + ref_list = parse_reference(verse_text, self.db_cache[bible],
> language_selection, book_ref_id)
> + if ref_list:
> + return self.db_cache[bible].get_verses(ref_list,
> show_error)
> + else:
> + return None
> +
> def save_meta_data(self, bible, version, copyright, permissions,
> book_name_language=None):
> """
> Saves the bibles meta data.
>
> === modified file 'openlp/plugins/bibles/lib/mediaitem.py'
> --- openlp/plugins/bibles/lib/mediaitem.py 2016-01-08 17:44:47 +0000
> +++ openlp/plugins/bibles/lib/mediaitem.py 2016-04-04 01:13:54 +0000
> @@ -45,6 +45,7 @@
> """
> Reference = 1
> Text = 2
> + Combined = 3
>
>
> class BibleMediaItem(MediaManagerItem):
> @@ -309,6 +310,9 @@
> self.plugin.manager.media = self
> self.load_bibles()
> self.quick_search_edit.set_search_types([
> + (BibleSearch.Combined, ':/bibles/bibles_search_combined.png',
> + translate('BiblesPlugin.MediaItem', 'Text or Scripture
> Reference'),
> + translate('BiblesPlugin.MediaItem', 'Text or Scripture
> Reference...')),
> (BibleSearch.Reference,
> ':/bibles/bibles_search_reference.png',
> translate('BiblesPlugin.MediaItem', 'Scripture
> Reference'),
> translate('BiblesPlugin.MediaItem', 'Search Scripture
> Reference...')),
> @@ -423,7 +427,7 @@
> def update_auto_completer(self):
> """
> This updates the bible book completion list for the search field.
> The completion depends on the bible. It is
> - only updated when we are doing a reference search, otherwise the
> auto completion list is removed.
> + only updated when we are doing reference or combined search, in
> text search the completion list is removed.
> """
> log.debug('update_auto_completer')
> # Save the current search type to the configuration.
> @@ -431,8 +435,8 @@
> # Save the current bible to the configuration.
> Settings().setValue(self.settings_section + '/quick bible',
> self.quickVersionComboBox.currentText())
> books = []
> - # We have to do a 'Reference Search'.
> - if self.quick_search_edit.current_search_type() ==
> BibleSearch.Reference:
> + # We have to do a 'Reference Search' (Or as part of Combined
> Search).
> + if self.quick_search_edit.current_search_type() is not
> BibleSearch.Text:
> bibles = self.plugin.manager.get_bibles()
> bible = self.quickVersionComboBox.currentText()
> if bible:
> @@ -648,10 +652,59 @@
> self.check_search_result()
> self.application.set_normal_cursor()
>
> + def on_quick_reference_search(self):
> + # We are doing a 'Reference Search'.
> + bible = self.quickVersionComboBox.currentText()
> + second_bible = self.quickSecondComboBox.currentText()
> + # Get input from field and replace '. ' with ''
> + # This will check if field has any '.' and removes them. Eg. Gen.
> 1 = Gen 1 = Genesis 1
> + text_direct = self.quick_search_edit.text()
> + text = text_direct.replace('. ', ' ')
> + if self.quick_search_edit.current_search_type() ==
> BibleSearch.Reference:
> + self.search_results = self.plugin.manager.get_verses(bible,
> text)
> + else:
> + self.search_results =
> self.plugin.manager.get_verses_combined(bible, text)
> + if second_bible and self.search_results:
> + self.second_search_results = \
> + self.plugin.manager.get_verses(second_bible, text,
> self.search_results[0].book.book_reference_id)
> +
> + def on_quick_text_search(self):
> + # We are doing a 'Text Search'.
> + bible = self.quickVersionComboBox.currentText()
> + second_bible = self.quickSecondComboBox.currentText()
> + text = self.quick_search_edit.text()
> + self.application.set_busy_cursor()
> + bibles = self.plugin.manager.get_bibles()
> + self.search_results = self.plugin.manager.verse_search(bible,
> second_bible, text)
> + if second_bible and self.search_results:
> + text = []
> + new_search_results = []
> + count = 0
> + passage_not_found = False
> + for verse in self.search_results:
> + db_book =
> bibles[second_bible].get_book_by_book_ref_id(verse.book.book_reference_id)
> + if not db_book:
> + log.debug('Passage "%s %d:%d" not found in Second
> Bible' %
> + (verse.book.name, verse.chapter,
> verse.verse))
> + passage_not_found = True
> + count += 1
> + continue
> + new_search_results.append(verse)
> + text.append((verse.book.book_reference_id, verse.chapter,
> verse.verse, verse.verse))
> + if passage_not_found:
> + self.main_window.information_message(
> + translate('BiblesPlugin.MediaItem', 'Information'),
> + translate('BiblesPlugin.MediaItem', 'The second Bible
> does not contain all the verses '
> + 'that are in the
> main Bible. Only verses found in both Bibles '
> + 'will be shown.
> %d verses have not been included '
> + 'in the
> results.') % count)
> + self.search_results = new_search_results
> + self.second_search_results =
> bibles[second_bible].get_verses(text)
> +
> def on_quick_search_button(self):
> """
> - Does a quick search and saves the search results. Quick search
> can either be "Reference Search" or
> - "Text Search".
> + Does a quick search and saves the search results. Quick search
> can be:
> + "Reference Search" or "Text Search" or Combined search.
> """
> log.debug('Quick Search Button clicked')
> self.quickSearchButton.setEnabled(False)
> @@ -660,40 +713,49 @@
> second_bible = self.quickSecondComboBox.currentText()
> text = self.quick_search_edit.text()
> if self.quick_search_edit.current_search_type() ==
> BibleSearch.Reference:
> - # We are doing a 'Reference Search'.
> - self.search_results = self.plugin.manager.get_verses(bible,
> text)
> - if second_bible and self.search_results:
> - self.second_search_results = \
> - self.plugin.manager.get_verses(second_bible, text,
> self.search_results[0].book.book_reference_id)
> - else:
> - # We are doing a 'Text Search'.
> - self.application.set_busy_cursor()
> - bibles = self.plugin.manager.get_bibles()
> - self.search_results = self.plugin.manager.verse_search(bible,
> second_bible, text)
> - if second_bible and self.search_results:
> - text = []
> - new_search_results = []
> - count = 0
> - passage_not_found = False
> - for verse in self.search_results:
> - db_book =
> bibles[second_bible].get_book_by_book_ref_id(verse.book.book_reference_id)
> - if not db_book:
> - log.debug('Passage "%s %d:%d" not found in Second
> Bible' %
> - (verse.book.name, verse.chapter,
> verse.verse))
> - passage_not_found = True
> - count += 1
> - continue
> - new_search_results.append(verse)
> - text.append((verse.book.book_reference_id,
> verse.chapter, verse.verse, verse.verse))
> - if passage_not_found:
> - QtWidgets.QMessageBox.information(
> - self, translate('BiblesPlugin.MediaItem',
> 'Information'),
> - translate('BiblesPlugin.MediaItem', 'The second
> Bible does not contain all the verses '
> - 'that are in the main Bible. Only
> verses found in both Bibles will be shown. %d '
> - 'verses have not been included in the
> results.') % count,
> -
> QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Ok))
> - self.search_results = new_search_results
> - self.second_search_results =
> bibles[second_bible].get_verses(text)
> + # We are doing a 'Reference Search'. (Get script from def
> on_quick_reference_search)
> + self.on_quick_reference_search()
> + elif self.quick_search_edit.current_search_type() ==
> BibleSearch.Text:
> + # We are doing a 'Text Search'. (Get script from def
> on_quick_text_search)
> + self.on_quick_text_search()
> + # Combined search, starting with reference search.
> + elif self.quick_search_edit.current_search_type() ==
> BibleSearch.Combined:
> + self.on_quick_reference_search()
> + # If keyword is shorter than 3 (not including spaces), message is
> given and search is finalized.
> + # It's actually to find verses with less than 3 chars (Eg. G1 =
> Genesis 1) thus this error is not shown if
> + # any results are found. This check needs to be here in order to
> avoid duplicate errors.
> + if not self.search_results and len(text) - text.count(' ') <
> 3:
> + self.main_window.information_message(
> + ('%s' % UiStrings().BibleShortSearchTitle),
> + ('%s' % UiStrings().BibleShortSearch))
> + # Text search starts here if no reference was found and
> keyword is longer than 2.
> + # This is required in order to avoid duplicate error messages
> for short keywords.
> + if not self.search_results and len(text) - text.count(' ') >
> 2:
> + self.on_quick_text_search()
> + # If no Text or Reference is found, message is given.
> + if not self.search_results:
> + self.application.set_normal_cursor()
> + reference_separators = {
> + 'verse':
> get_reference_separator('sep_v_display'),
> + 'range':
> get_reference_separator('sep_r_display'),
> + 'list':
> get_reference_separator('sep_l_display')}
> +
> self.main_window.information_message(translate('BiblesPlugin.BibleManager',
> 'Nothing found'),
> +
> translate('BiblesPlugin.BibleManager', '<strong>OpenLP '
> +
> 'couldn’t find '
> +
> 'anything with your'
> +
> ' search.</strong>'
> +
> '<br><br>'
> +
> 'If you tried to '
> +
> 'search with '
> +
> 'Scripture '
> +
> 'Reference, please '
> +
> 'make<br>sure that '
> +
> 'your reference '
> +
> 'follows one of '
> +
> 'these patterns:%s'
> + %
> UiStrings().BibleScriptureError %
> +
> reference_separators))
> + # Finalizing the search
> if not self.quickLockButton.isChecked():
> self.list_view.clear()
> if self.list_view.count() != 0 and self.search_results:
>
> === modified file 'resources/images/openlp-2.qrc'
> --- resources/images/openlp-2.qrc 2016-02-06 17:50:58 +0000
> +++ resources/images/openlp-2.qrc 2016-04-04 01:13:54 +0000
> @@ -30,6 +30,7 @@
> <file>image_new_group.png</file>
> </qresource>
> <qresource prefix="bibles">
> + <file>bibles_search_combined.png</file>
> <file>bibles_search_text.png</file>
> <file>bibles_search_reference.png</file>
> <file>bibles_upgrade_alert.png</file>
>
>
>
--
https://code.launchpad.net/~suutari-olli/openlp/combined-bible-quick-search/+merge/290846
Your team OpenLP Core is subscribed to branch lp:openlp.
References
