openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #29515
Re: [Merge] lp:~suutari-olli/openlp/combined-bible-quick-search into lp:openlp
I added your comments and replied to them.
Thanks.
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 05:35:40 +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."
Removed the comment.
> + 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"
Removed the comment.
> + 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 05:35:40 +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"
Removed the comment.
> :param bible: Unicode. The Bible to use.
> :param verse_text:
> @@ -265,41 +266,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))
> 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)
"Not needed"
Removed the comment.
> 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):
> @@ -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"
Removed comment:
#Web Bible checking method is currently bound to this file, so it can't be properly moved to mediaitem.py
> + # 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.')
"you no not need "and may not be used"
OK, I've removed the "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 05:35:40 +0000
> @@ -648,52 +659,144 @@
> self.check_search_result()
> self.application.set_normal_cursor()
>
> + def on_quick_reference_search(self):
> + # We are doing a 'Reference Search'.
"Need formatted doc type here"
"""
Added stuff here.
"""
> + # 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"
Ah, I had to remove this but I forgot to do it, it was required
before I re-structured this for the auto search function.
(Removed it)
> + 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'.
missing doc block.
"""
Added stuff here.
"""
> + # 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"
Removed comment:
> + 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,
> + # new_search_results is needed to make sure 2nd bible contains all verses. (And counting them on error)
"not needed"
I removed
# Set up variables,
> + 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' %
> + (verse.book.name, verse.chapter, verse.verse))
"Need to start using python3 formatted messages. Seen merge requests from aliosnken1"
Will look into this later.
> + 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"
Will look into this later.
> + # 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.
> self.quickSearchButton.setEnabled(False)
"not needed"
Removed comment:
# Disable the button while processing, get text from Quick search field.
> self.application.process_events()
> + # These need to be defined here too so the search results can be displayed.
"not needed"
Comment? Removed it.
In the last review you asked me to explain this function
and instructed I should add bunch of documentations,
suppose that meant """ """"
> 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.
Removed comment:
# if nothing is found, message is given.
> + # 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"
Removed the comment.
> + 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 +844,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."
I've simplified it.
Searching while typing is more demanding, Text search is more demanding, than Reference search, thus more characters are required to stop it from breaking the search. I changed Text search to 4 as well, although 5 may gives a bit better stability. Shorter min len for Combined is required for eq. Ps 3
> + 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
"Is the screen locked?
Does the search stop if more typing?
Why 1.3 seconds?"
Don't think the screen is locked.
It's still possible to type but if search is on, chars may be delayed.
Timer resets when new chars are entered or removed.
Why 1.3 seconds? So you can press an another key before it starts searching.
If text search is performed on every char OpenLP would break. Just for the sake of confusing things, I've changed it to 1.2 to trigger searches a bit faster.
> + # Use the self.on_quick_search_while_typing, 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_while_typing)
> + # 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/293496
Your team OpenLP Core is subscribed to branch lp:openlp.
References
