New search by RJ722 · Pull Request #558 · jendrikseipp/rednotebook

RJ722 · 2021-02-08T22:28:24Z

Summary of the changes in this pull request

Previously, a search would only return results with exact matches, that is if the search string is a substring of the text in the window buffer. With this new edition, we support searching for multiple words. A result would only be shown if all of those words appear at least once in the text.

There is a bug in there due to which highlights in the main windows aren't working properly. I have figured that one needs to have the day opened on the side, go to the search bar, and hit enter again for the highlights to work correctly. I have a hunch that this is because I've skipped the "scroll to query" part -- which I'm guessing refreshes the view? Just a hunch, I can be totally off on this.

EDIT: Whenever a search result is clicked, Day.show_day method is called to update the view. It used the self.search_text to take care of highlighting and scrolling to search results. I updated it to use the new self.search_queries instead, and everything appears to be working! \o/

Pull request checklist

I have added an entry in CHANGELOG.md including my name and issue and/or pull request number.
If applicable: I have removed the corresponding entry in TODO.md.

jendrikseipp

Looks good :-) I only have a few comments. Maybe the bug you're seeing will be gone after taking care of these.

RJ722 · 2021-03-05T19:33:06Z

@jendrikseipp After the latest changes, indeed the bug went away. (I was actually still checking the old search_text, whereas I had renamed it to search_queries.)

I've found another bug now which causes an exception if only a date string is passed to search (easily fixable). I'll do that. Plus, I was thinking that I should change all occurrences of queries to words. What do you think?

jendrikseipp · 2021-03-05T22:40:22Z

Plus, I was thinking that I should change all occurrences of queries to words. What do you think?

Sounds good!

RJ722 · 2021-03-09T18:20:41Z

Took me a lot longer, but pheww. finally done. :D

jendrikseipp · 2021-03-10T22:10:45Z

Awesome! I hope to be able to review this soon.

jendrikseipp

I finally found the time to have a look :-) This is shaping up nicely!

jendrikseipp · 2021-04-25T10:21:37Z

+            for query in search_words:
+                self.get_find_controller().search(
+                    query, WebKit2.FindOptions.CASE_INSENSITIVE, MAX_HITS
+                )


Can we stop after the first word is found?

A little confused here since the docs say that the search method performs the search asynchronously. If so, would this be possible?

Previously, a search would only return results with exact matches, that is if the search string is a substring of the text in the window buffer. With this new edition, we support searching for multiple words. A result would only be shown if *all* of those words appear at least once in the text.

RJ722 · 2021-05-15T05:26:48Z

Hey, thanks for the review Jendrik! \o/

I believe I've addressed all of the comments. I also took the liberty and renamed the DayEditor.scroll_to_text to DayEditor.scroll_to_non_date_text which will help prevent the confusion because of the method in the parent class by the same name. Let me know if you don't like that and would like to revert.

RJ722 · 2021-05-15T05:39:17Z

Closing and re-opening to trigger CI again.

If one of the searched words is present in the current day's text, highlight it. Previously, the higlighting would only account for the last searched word.

jendrikseipp

Thanks for taking care of my comments! I looked into the webkit highlighting. Now everything should work. I only have minor comments remaining.

jendrikseipp · 2021-06-05T18:56:46Z

+            words_contain_date = len(non_date_words) != len(words)
+            if words_contain_date:
+                results.append(get_text_with_dots(self.text, 0, TEXT_RESULT_LENGTH))
+            # If all the words matched agains the date, return.


"matched agains" -> "match"

jendrikseipp · 2021-06-05T19:00:07Z

    def _get_buffer_for_day(self, day):
        return self._get_buffer(day.date, day.text)

+    def scroll_to_non_date_text(self, words):


I like the new method name.

jendrikseipp · 2021-06-05T19:01:14Z

+        """
+        for word in words:
+            # If word matches date, it probably is not present in the text.
+            if word in str(self.day):


Let's remove the pass by inverting the if statement: if word not in str(self.day)

jendrikseipp · 2021-06-05T19:01:59Z

+            if word in str(self.day):
+                pass
+            else:
+                super().scroll_to_text(word)


Do we still need the super() now that the child method is renamed?

jendrikseipp · 2023-08-20T09:44:48Z

@RJ722 Do you still want to pursue this? If so, it seems you'll have to merge with master first and then take care of my latest few comments.

RJ722 force-pushed the new-search branch from a7566df to 60dde26 Compare February 8, 2021 22:32

jendrikseipp requested changes Feb 9, 2021

View reviewed changes

Comment thread rednotebook/data.py Outdated

Comment thread rednotebook/gui/editor.py Outdated

Comment thread rednotebook/gui/search.py Outdated

RJ722 marked this pull request as ready for review February 28, 2021 12:28

RJ722 force-pushed the new-search branch 2 times, most recently from 38263d8 to d79034e Compare February 28, 2021 12:42

RJ722 mentioned this pull request Mar 11, 2021

Ability to Search for multiple words #564

Open

jendrikseipp requested changes Apr 25, 2021

View reviewed changes

RJ722 and others added 8 commits May 15, 2021 10:50

Add entry to changelog, remove TODO

24fe9c7

Update TODO.md

dfe87d9

Remove (date-matching query; call to enumerate)

fc51d80

Highlight queries and scroll to them

b8eee88

Rename queries to words

a4287cd

If all words are date-matching, don't search in text

a7a829f

Address review comments from Jendrik

6229ab1

RJ722 force-pushed the new-search branch from 12e6e31 to 6229ab1 Compare May 15, 2021 05:23

RJ722 closed this May 15, 2021

RJ722 reopened this May 15, 2021

jendrikseipp added 2 commits June 6, 2021 00:49

Fix highlighting of searched words in webkit.

ff5aeda

If one of the searched words is present in the current day's text, highlight it. Previously, the higlighting would only account for the last searched word.

Stop after first matching word found.

42009c9

jendrikseipp requested changes Jun 5, 2021

View reviewed changes

jendrikseipp mentioned this pull request Dec 29, 2022

Search for multiple words #180

Closed

Conversation

RJ722 commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the changes in this pull request

Pull request checklist

Uh oh!

jendrikseipp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RJ722 commented Mar 5, 2021

Uh oh!

jendrikseipp commented Mar 5, 2021

Uh oh!

RJ722 commented Mar 9, 2021

Uh oh!

jendrikseipp commented Mar 10, 2021

Uh oh!

jendrikseipp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jendrikseipp Apr 25, 2021

Choose a reason for hiding this comment

Uh oh!

RJ722 May 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RJ722 commented May 15, 2021

Uh oh!

RJ722 commented May 15, 2021

Uh oh!

jendrikseipp left a comment

Choose a reason for hiding this comment

Uh oh!

jendrikseipp Jun 5, 2021

Choose a reason for hiding this comment

Uh oh!

jendrikseipp Jun 5, 2021

Choose a reason for hiding this comment

Uh oh!

jendrikseipp Jun 5, 2021

Choose a reason for hiding this comment

Uh oh!

jendrikseipp Jun 5, 2021

Choose a reason for hiding this comment

Uh oh!

jendrikseipp commented Aug 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RJ722 commented Feb 8, 2021 •

edited

Loading