rednotebook icon indicating copy to clipboard operation
rednotebook copied to clipboard

New search

Open RJ722 opened this issue 3 years ago • 7 comments

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

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

RJ722 avatar Feb 08 '21 22:02 RJ722

@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?

RJ722 avatar Mar 05 '21 19:03 RJ722

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

Sounds good!

jendrikseipp avatar Mar 05 '21 22:03 jendrikseipp

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

RJ722 avatar Mar 09 '21 18:03 RJ722

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

jendrikseipp avatar Mar 10 '21 22:03 jendrikseipp

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 avatar May 15 '21 05:05 RJ722

Closing and re-opening to trigger CI again.

RJ722 avatar May 15 '21 05:05 RJ722

@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.

jendrikseipp avatar Aug 20 '23 09:08 jendrikseipp