Review: Needs Fixing
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
Why. Is this to shorten the field length?
> + chapter = translate('OpenLP.Ui', 'Chapter')
> + cha = chapter
> + verse = translate('OpenLP.Ui', 'Verse')
> + ver = verse
> + gap = ' | '
Do we need this why not have it in the text.
> + 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
Why not
self.BibleScriptureError = '.join(str(joined) for joined in
bible_scripture_items)
>
> === 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):
No method documentation.
Just one line and what the variables are.
> + log.debug('BibleManager.get_verses("%s", "%s")', bible,
> verse_text)
Indent looks wrong - One tab too many?
> + 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):
No Method documentation.
> + # 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('. ', ' ')
Don't need text_direct. Introduce extra local variables.
text = text.replace('. ', ' ')
> + # 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):
Needs comment. You seem to have the info but not in the correct format!
> + # 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 = []
Confusing.
Line 317 text is a string now you are redefining it as an array.
search_text would be better.
> + new_search_results = []
second_search_results?
What is new about this - it is a second search.
> + 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.
Comments need to be indented as they confuse the layout of the code.
If the indentation is correct we have a problem as the code is hard difficult
to read.
> + # 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:
Could a have a helper function we have the same code 3 times?
def search_len(text, num)
if len(text) - text.count(' ') > num
return true
return false
> + 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'),
yuck!
Nicely laid out but unreadable!
> +
> 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.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
