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.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
