openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #29126
Re: [Merge] lp:~suutari-olli/openlp/combined-bible-quick-search into lp:openlp
I fixed most of these things and replied to the comments.
Now re-proposing. (Still need some tests though)
------------------
Comments in line.
Mainly style and missing bits like comments.
Some tests will be needed.
Some of the comments are based on moved code but it was wrong then and can be made better.
Left you a challenge at the end.
Diff comments:
> === 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-09 21:11:39 +0000
> @@ -151,3 +152,38 @@
> self.Version = translate('OpenLP.Ui', 'Version')
> self.View = translate('OpenLP.Ui', 'View')
> self.ViewMode = translate('OpenLP.Ui', 'View Mode')
> + # Translations used in both, bibles\lib\mediaitem.py and bibles\lib\manager.py
> + self.BibleShortSearchTitle = translate('OpenLP.Ui', 'Search is Empty or too Short')
> + 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.')
> + self.BibleNoBiblesTitle = translate('OpenLP.Ui', 'No Bibles Available')
> + self.BibleNoBibles = translate('OpenLP.Ui', '<strong>There are no Bibles currently installed.</strong><br><br>'
> + 'Please use the Import Wizard to install one or more Bibles.')
> + # Scripture reference error combined from small translation stings by using itertools.
> + book_chapter = translate('OpenLP.Ui', 'Book Chapter')
> + bc = book_chapter
I have now removed the shortened tags.
The limit of 120 char width made it a bit difficult to build the message.
I managed to get more space by adding new line before starting the new message.
> + chapter = translate('OpenLP.Ui', 'Chapter')
> + cha = chapter
> + verse = translate('OpenLP.Ui', 'Verse')
> + ver = verse
> + gap = ' | '
Do you mean the Gap?
Why repeat it and take the change of messing the spaces?
> + psalm = translate('OpenLP.Ui', 'Psalm')
> + may_shorten = translate('OpenLP.Ui', 'Book names may be shortened from full names, for an example Ps 23 = '
> + 'Psalm 23')
> + bible_scripture_items = [bc, gap, psalm, ' 23<br>',
> + bc, '%(range)s', cha, gap, psalm, ' 23%(range)s24<br>',
> + bc, '%(verse)s', ver, '%(range)s', ver, gap, psalm, ' 23%(verse)s1%(range)s2<br>',
> + bc, '%(verse)s', ver, '%(range)s', ver, '%(list)s', ver, '%(range)s', ver, gap, psalm,
> + ' 23%(verse)s1%(range)s2%(list)s5%(range)s6<br>',
> + bc, '%(verse)s', ver, '%(range)s', ver, '%(list)s', cha, '%(verse)s', ver, '%(range)s',
> + ver, gap, psalm, ' 23%(verse)s1%(range)s2%(list)s24%(verse)s1%(range)s3<br>', bc,
> + '%(verse)s', ver, '%(range)s', cha, '%(verse)s', ver, gap, psalm,
> + ' 23%(verse)s1%(range)s24%(verse)s1<br><br>', may_shorten]
> + itertools.chain.from_iterable(itertools.repeat(strings, 1) if isinstance(strings, str) else strings for strings
> + in bible_scripture_items)
> + bible_scripture_error_joined = ''.join(str(joined) for joined in bible_scripture_items)
> + # This is what gets called to other files.
> + self.BibleScriptureError = bible_scripture_error_joined
There's absolutely no reason, done that.
>
> === modified file 'openlp/plugins/bibles/lib/manager.py'
> --- openlp/plugins/bibles/lib/manager.py 2016-04-05 17:10:51 +0000
> +++ openlp/plugins/bibles/lib/manager.py 2016-04-09 21:11:39 +0000
> @@ -338,22 +322,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=True):
I removed this method and moved the error message to mediaitem so it was no longer needed.
> + log.debug('BibleManager.get_verses("%s", "%s")', bible, verse_text)
Looks just right in PyCharm.
> + if not bible:
> + if show_error:
> + if not bible:
> + self.main_window.information_message(
> + ('%s' % UiStrings().BibleNoBiblesTitle),
> + ('%s' % UiStrings().BibleNoBibles))
> + return None
> + 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-04-03 19:44:09 +0000
> +++ openlp/plugins/bibles/lib/mediaitem.py 2016-04-09 21:11:39 +0000
> @@ -648,10 +652,61 @@
> 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('. ', ' ')
Good point.
This might had been a problem when it was not yet separated into def on_quick_reference_search.
> + # If we are doing "Reference" search, use the search from manager.py
> + if self.quick_search_edit.current_search_type() == BibleSearch.Reference:
> + self.search_results = self.plugin.manager.get_verses(bible, text)
> + # If we are doing "Combined Reference" search, use the get_verses_combined from manager.py (No error included)
> + 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 = []
I found this 24 times with search.
This was not coded by me, rather leave it as it is.
> + new_search_results = []
There's already second_search_results, it can't be the same.
Apparently this is needed to check all the passages are found in the 2nd Bible too
and is joined to search_results.
It may be changed but I really don't see the need for it. (I didn't code it either)
> + 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 +715,52 @@
> 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.
Fixed that.
> + # 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 no Bibles are installed, this message is not shown - "No bibles" message is whon 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))
> + # 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 and bible:
It's rather short and simple function. I think it wouldn't make the code simpler.
This function is actually the same just twice, once its with > 2.
> + self.on_quick_text_search()
> + # If no Text or Reference is found, message is given.
> + if not self.search_results and not \
> + Settings().value(self.settings_section + '/hide combined quick error'):
> + 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'),
Yam!
Looks much better now!
> + 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
> 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/291442
Your team OpenLP Core is subscribed to branch lp:openlp.
References
