openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #29734
Re: [Merge] lp:~suutari-olli/openlp/combined-bible-quick-search into lp:openlp
Review: Needs Fixing
See my inline comments.
Also, please stop using settings for once-off flags. This is not a web app, it maintains state. If you can't save a flag on one object, find another shared object to use. The settings is not the right place for it.
Diff comments:
>
> === modified file 'openlp/plugins/bibles/lib/manager.py'
> --- openlp/plugins/bibles/lib/manager.py 2016-04-12 20:05:58 +0000
> +++ openlp/plugins/bibles/lib/manager.py 2016-05-15 20:46:46 +0000
> @@ -265,41 +265,20 @@
> :param show_error:
> """
> log.debug('BibleManager.get_verses("%s", "%s")', bible, verse_text)
> + # If no bibles are installed, message is given.
> 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.')
> - )
> + ('%s' % UiStrings().BibleNoBiblesTitle),
> + ('%s' % UiStrings().BibleNoBibles))
See my comment further down about this.
> return None
> + # Get the language for books.
> 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)
> + # If nothing is found. Message is given if this is not combined search. (defined in mediaitem.py)
> else:
> - if show_error:
> - 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', '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
> - )
> return None
>
> def get_language_selection(self, bible):
> @@ -331,13 +310,11 @@
> :param text: The text to search for (unicode).
> """
> log.debug('BibleManager.verse_search("%s", "%s")', bible, text)
> + # If no bibles are installed, message is given.
> if not bible:
> 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.')
> - )
> + ('%s' % UiStrings().BibleNoBiblesTitle),
Why are you doing this, it is pointless. Drop the "'%s' %" and just use the string.
> + ('%s' % UiStrings().BibleNoBibles))
> return None
Ditto.
> # Check if the bible or second_bible is a web bible.
> web_bible = self.db_cache[bible].get_object(BibleMeta, 'download_source')
>
> === modified file 'openlp/plugins/bibles/lib/mediaitem.py'
> --- openlp/plugins/bibles/lib/mediaitem.py 2016-04-10 20:24:07 +0000
> +++ openlp/plugins/bibles/lib/mediaitem.py 2016-05-15 20:46:46 +0000
> @@ -648,52 +659,148 @@
> self.check_search_result()
> self.application.set_normal_cursor()
>
> + def on_quick_reference_search(self):
> + """
> + We are doing a 'Reference Search'.
> + This search is called on def on_quick_search_button by Quick Reference and Combined Searches.
> + """
> + # Set Bibles to use the text input from Quick search field.
> + bible = self.quickVersionComboBox.currentText()
> + second_bible = self.quickSecondComboBox.currentText()
> + """
> + Get input from field and replace 'A-Z + . ' with ''
> + This will check if field has any '.' after A-Z and removes them. Eg. Gen. 1 = Gen 1 = Genesis 1
> + If Book name has '.' after number. eg. 1. Genesis, the search fails without the dot, and vice versa.
> + A better solution would be to make '.' optional in the search results. Current solution was easier to code.
> + """
> + text = self.quick_search_edit.text()
> + text = re.sub('\D[.]\s', ' ', text)
> + # This is triggered on reference search, use the search from manager.py
> + if self.quick_search_edit.current_search_type() != BibleSearch.Text:
> + 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)
> +
> + def on_quick_text_search(self):
> + """
> + We are doing a 'Text Search'.
> + This search is called on def on_quick_search_button by Quick Text and Combined Searches.
> + """
> + # Set Bibles to use the text input from Quick search field.
> + bible = self.quickVersionComboBox.currentText()
> + second_bible = self.quickSecondComboBox.currentText()
> + text = self.quick_search_edit.text()
> + self.application.set_busy_cursor()
> + # Get Bibles list
> + bibles = self.plugin.manager.get_bibles()
> + # Add results to "search_results"
> + self.search_results = self.plugin.manager.verse_search(bible, second_bible, text)
> + if second_bible and self.search_results:
> + # new_search_results is needed to make sure 2nd bible contains all verses. (And counting them on error)
> + text = []
> + new_search_results = []
> + count = 0
> + passage_not_found = False
> + # Search second bible for results of search_results to make sure everythigns there.
> + # Count all the unfound passages.
> + 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 ("{versebookname}","{versechapter}","{verseverse}") not found in Second Bible'
> + .format(versebookname=verse.book.name, versechapter='verse.chapter',
> + verseverse=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:
> + # This is for the 2nd Bible.
> + 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.\nOnly verses found in both Bibles'
> + ' will be shown.\n\n {count} verses have not been included '
> + 'in the results.').format(count=count))
> + # Join the searches so only verses that are found on both Bibles are shown.
> + 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".
> + This triggers the proper Quick search based on which search type is used.
> + "Eg. "Reference Search", "Text Search" or "Combined search".
> """
> log.debug('Quick Search Button clicked')
> + # If we are performing "Search while typing", this setting is set to True, here it's reset to "False"
> + Settings().setValue('bibles/hide web bible error if searching while typing', False)
I don't like this. This smells like a flag, not a setting. Only use settings for things that users can set. Find another way to achieve this if it's not supposed to be user-editable.
> self.quickSearchButton.setEnabled(False)
> self.application.process_events()
> bible = self.quickVersionComboBox.currentText()
> 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()
> + # Get reference separators from settings.
> + if not self.search_results:
> + 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', 'Scripture Reference Error'),
> + 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><br><br>%s'
> + % UiStrings().BibleScriptureError % reference_separators))
> + 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()
> + if not self.search_results and len(text) - text.count(' ') < 3 and bible:
> + self.main_window.information_message(
> + ('%s' % UiStrings().BibleShortSearchTitle),
> + ('%s' % UiStrings().BibleShortSearch))
See my previous comment about the same.
> + elif self.quick_search_edit.current_search_type() == BibleSearch.Combined:
> + # We are doing a 'Combined search'. Starting with reference search.
> + # Perform only if text contains any numbers
> + if (char.isdigit() for char in text):
> + self.on_quick_reference_search()
> + # If results are found, search will be finalized.
> + # This check needs to be here in order to avoid duplicate errors.
> + # If keyword is shorter than 3 (not including spaces), message is given. It's actually possible to find
> + # verses with less than 3 chars (Eg. G1 = Genesis 1) thus this error is not shown if any results are found.
> + # if no Bibles are installed, this message is not shown - "No bibles" message is shown instead. (and bible)
> + if not self.search_results and len(text) - text.count(' ') < 3 and bible:
> + self.main_window.information_message(
> + ('%s' % UiStrings().BibleShortSearchTitle),
> + ('%s' % UiStrings().BibleShortSearch))
Same as earlier
> + if not self.search_results and len(text) - text.count(' ') > 2 and bible:
> + # Text search starts here if no reference was found and keyword is longer than 2.
> + # > 2 check is required in order to avoid duplicate error messages for short keywords.
> + self.on_quick_text_search()
> + if not self.search_results and not \
> + Settings().value(self.settings_section + '/hide combined quick error'):
> + self.application.set_normal_cursor()
> + # Reference separators need to be defined both, in here and on reference search,
> + # error won't work if they are left out from one.
> + 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: <br><br>%s'
> + % UiStrings().BibleScriptureError %
> + reference_separators))
> + # Finalizing the search
> + # List is cleared if not locked, results are listed, button is set available, cursor is set to normal.
> if not self.quickLockButton.isChecked():
> self.list_view.clear()
> if self.list_view.count() != 0 and self.search_results:
--
https://code.launchpad.net/~suutari-olli/openlp/combined-bible-quick-search/+merge/294740
Your team OpenLP Core is subscribed to branch lp:openlp.
References
