openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #29510
Re: [Merge] lp:~suutari-olli/openlp/combined-bible-quick-search into lp:openlp
Review: Needs Fixing
Lots of extra comments in the code which are not needed and make reading more difficult.
The search as type code needs more thinking as the timer has issues.
Need to use python 3 string change features.
Please do not keep updating the main section in each merge request use the comments. The top section is for the overall request not each individual one and that is what is merged and preserved.
Diff comments:
> === modified file 'openlp/core/common/uistrings.py'
> --- openlp/core/common/uistrings.py 2016-04-16 19:51:35 +0000
> +++ openlp/core/common/uistrings.py 2016-05-02 04:59:52 +0000
> @@ -152,3 +153,34 @@
> self.Version = translate('OpenLP.Ui', 'Version')
> self.View = translate('OpenLP.Ui', 'View')
> self.ViewMode = translate('OpenLP.Ui', 'View Mode')
> + # Translations that are used in bibles\lib\mediaitem.py and bibles\lib\manager.py
No needed they are common.
> + 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.
not needed
> + book_chapter = translate('OpenLP.Ui', 'Book Chapter')
> + chapter = translate('OpenLP.Ui', 'Chapter')
> + verse = translate('OpenLP.Ui', 'Verse')
> + gap = ' | '
> + 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 = \
> + [book_chapter, gap, psalm, ' 23<br>',
> + book_chapter, '%(range)s', chapter, gap, psalm, ' 23%(range)s24<br>',
> + book_chapter, '%(verse)s', verse, '%(range)s', verse, gap, psalm, ' 23%(verse)s1%(range)s2<br>',
> + book_chapter, '%(verse)s', verse, '%(range)s', verse, '%(list)s', verse, '%(range)s', verse, gap, psalm,
> + ' 23%(verse)s1%(range)s2%(list)s5%(range)s6<br>',
> + book_chapter, '%(verse)s', verse, '%(range)s', verse, '%(list)s', chapter, '%(verse)s', verse, '%(range)s',
> + verse, gap, psalm, ' 23%(verse)s1%(range)s2%(list)s24%(verse)s1%(range)s3<br>',
> + book_chapter, '%(verse)s', verse, '%(range)s', chapter, '%(verse)s', verse, 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)
> + 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-12 20:05:58 +0000
> +++ openlp/plugins/bibles/lib/manager.py 2016-05-02 04:59:52 +0000
> @@ -247,6 +247,7 @@
> """
> Parses a scripture reference, fetches the verses from the Bible
> specified, and returns a list of ``Verse`` objects.
> + This function is called in \bibles\lib\mediaitem.py by def on_quick_search_button
Not needed
>
> :param bible: Unicode. The Bible to use.
> :param verse_text:
> @@ -325,19 +305,18 @@
> def verse_search(self, bible, second_bible, text):
> """
> Does a verse search for the given bible and text.
> + This function is called in \bibles\lib\mediaitem.py by def on_quick_search_button.
not needed
>
> :param bible: The bible to search in (unicode).
> :param second_bible: The second bible (unicode). We do not search in this bible.
> :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),
> + ('%s' % UiStrings().BibleNoBibles))
> return None
> # Check if the bible or second_bible is a web bible.
> web_bible = self.db_cache[bible].get_object(BibleMeta, 'download_source')
> @@ -345,20 +324,27 @@
> if second_bible:
> second_web_bible = self.db_cache[second_bible].get_object(BibleMeta, 'download_source')
> if web_bible or second_web_bible:
> - self.main_window.information_message(
> - translate('BiblesPlugin.BibleManager', 'Web Bible cannot be used'),
> - translate('BiblesPlugin.BibleManager', 'Text Search is not available with Web Bibles.')
> - )
> - return None
> - if text:
> + # If either Bible is Web, cursor is reset to normal and message is given.
> + self.application.set_normal_cursor()
> + # If we are performing "Search while typing", do not show this error.
> + # Web Bible checking method is currently bound to this file, so it can't be properly moved to mediaitem.py
not needed
> + # Without making some changes to the stucture. (= self.db_cache[bible].get_object(BibleMeta,...)
> + if not Settings().value('bibles/hide web bible error if searching while typing'):
> + self.main_window.information_message(
> + translate('BiblesPlugin.BibleManager', 'Web Bible cannot be used'),
> + translate('BiblesPlugin.BibleManager', 'Text Search is not available with Web Bibles.\n'
> + 'Please use the Scripture Reference Search instead.\n\n'
> + 'This means that the currently used Bible or Second Bible\n'
> + 'is installed as Web Bible and may not be used.')
is installed as Web Bible.
you no not need "and may not be used".
> + )
> + return None
> + # Shorter than 3 char searches break OpenLP with very long search times, thus they are blocked.
> + if len(text) - text.count(' ') < 3:
> + return None
> + # Fetch the results from db. If no results are found, return None, no message is given for this.
> + 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 save_meta_data(self, bible, version, copyright, permissions, book_name_language=None):
>
> === 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-02 04:59:52 +0000
> @@ -648,52 +660,144 @@
> self.check_search_result()
> self.application.set_normal_cursor()
>
> + def on_quick_reference_search(self):
Need formatted doc type here
> + # We are doing a 'Reference Search'.
> + # 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 '. ' with ''
> + # This will check if field has any '.' and removes them. Eg. Gen. 1 = Gen 1 = Genesis 1
> + text = self.quick_search_edit.text()
> + text = text.replace('. ', ' ')
> + # This is triggered on 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)
> + elif self.quick_search_edit.current_search_type() == BibleSearch.Combined:
> + # In Combined Reference search no error is given if no results are found. (This would result in duplicate)
not needed
> + 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):
missing doc block.
> + # We are doing a 'Text Search'.
> + # 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()
> + # This changes the curson to "Loading animation"
not needed
> + 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:
> + # Set up variables,
not needed
> + # 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 "%s %d:%d" not found in Second Bible' %
Need to start using python3 formatted messages. Seen merge requests from aliosnken1
> + (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:
> + # 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. Only verses found in both Bibles '
> + 'will be shown. %d verses have not been included '
> + 'in the results.') % count)
message formatting see above
> + # 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)
> + # Disable the button while processing, get text from Quick search field.
not needed
> self.quickSearchButton.setEnabled(False)
> self.application.process_events()
> + # These need to be defined here too so the search results can be displayed.
not needed
> 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()
> + # if nothing is found, message is given.
not needed
> + # 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))
> + elif self.quick_search_edit.current_search_type() == BibleSearch.Combined:
> + # We are doing a 'Combined search'. Starting with reference search.
> + 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))
> + 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 no Text or Reference is found, message is given, unless a setting for not showing it is enabled.
not needed
> + 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:
> @@ -715,6 +845,37 @@
> self.search_results = {}
> self.second_search_results = {}
>
> + def on_search_text_edit_changed(self):
> + """
> + If search automatically while typing is enabled, perform the search and list results when conditions are met.
> + """
> + text = self.quick_search_edit.text()
> + # If web bible is used, don't show the error while searching and typing.
> + # This would result in seeing the same message multiple times.
> + # This message is located in lib\manager.py, so the setting is required.
> + Settings().setValue('bibles/hide web bible error if searching while typing', True)
> + search_length = 1
> + if self.quick_search_edit.current_search_type() == BibleSearch.Combined:
> + search_length = 4
> + if self.quick_search_edit.current_search_type() == BibleSearch.Reference:
> + search_length = 3
> + elif self.quick_search_edit.current_search_type() == BibleSearch.Text:
> + search_length = 5
> + # Regex for finding space + any non whitemark character. (Prevents search from starting on 1 word searches)
Are these correct. You look for 2 characters in the code above (hard coded)
You have 4, 3, and 5 here confusing.
Need to be constants at the top of the file with names to provide some clarity as I am confused.
> + space_and_any = re.compile(' \S')
> + # Turn this into a format that may be used in if statement.
> + count_space_any = space_and_any.findall(text)
> + # Start searching if this behaviour is not disabled in settings and conditions are met.
> + if Settings().value('bibles/is search while typing enabled'):
> + if len(text) > search_length and len(count_space_any) != 0:
> + # Start search if no chars are entered or deleted for 1.3 seconds
> + # Use the self.on_quick_search_search_as_type_text, this does not contain any error messages.
> + # This method may be a bit buggy sometimes and starts shorter than required searches due to the delay.
> + QtCore.QTimer().singleShot(1300, self.on_quick_search_search_as_type_text)
Is the screen locked?
Does the search stop if more typing?
Why 1.3 seconds?
> + # If text length is less than 4 and results are not locked, it's still possible to search short references.
> + if not self.quickLockButton.isChecked() and len(text) < 4:
> + self.list_view.clear()
> +
> def build_display_results(self, bible, second_bible, search_results):
> """
> Displays the search results in the media manager. All data needed for further action is saved for/in each row.
--
https://code.launchpad.net/~suutari-olli/openlp/combined-bible-quick-search/+merge/293495
Your team OpenLP Core is subscribed to branch lp:openlp.
References
