Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: find dialog 1760 #106

Merged
merged 48 commits into from
Aug 10, 2023
Merged

Conversation

MayurSMahajan
Copy link
Contributor

@MayurSMahajan MayurSMahajan commented Apr 28, 2023

This PR aims at creating a FInd Dialog, that is able to find string patterns in editor document.

This PR introduces three new files for implementing this feature:

  1. find_menu_service.dart
  2. find_replace_widget.dart
  3. search_service.dart

Find Menu Service
This service is actually implemented by FindReplaceMenu, which is responsible for popping a Find Replace Menu onto the editor.
It is able to do so by adding a new OverlayEntry to the editor.

Find Replace Widget
It is simply a widget component strictly used for presentation. It separates the FindMenuService from the SearchService. It describes the UI of the Find Replace Menu.

Search Service
The Heavy Logic Lifting is done by a class called SearchService. This class contains public methods like:
findAndHighlight: finds the pattern and highlights it.
navigateToMatch: navigates between matched patterns in the vertical direction.
unHighlight: unhighlights the matches when a new pattern is searched or the find dialog is closed.

It also contains a bunch of private utility methods, one particularly interesting method is the _boyerMooreSearch method, which implements the world-renowned Boyer Moore Search algorithm for searching the pattern in the editor.

Limitations

  • Scrolling to the next match using arrow keys, does not behave as expected.
  • Behaviour on already highlighted patterns.

Features Yet to be implemented

  • Replace handler
  • Tests

@MayurSMahajan MayurSMahajan marked this pull request as draft April 28, 2023 18:28
@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #106 (90ba926) into main (953249f) will increase coverage by 0.47%.
The diff coverage is 93.72%.

@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
+ Coverage   75.60%   76.07%   +0.47%     
==========================================
  Files         247      252       +5     
  Lines        9920    10182     +262     
==========================================
+ Hits         7500     7746     +246     
- Misses       2420     2436      +16     
Files Changed Coverage Δ
...k_component/rich_text/appflowy_rich_text_keys.dart 100.00% <ø> (ø)
...tor/block_component/standard_block_components.dart 100.00% <ø> (ø)
...editor/toolbar/desktop/items/color/color_menu.dart 0.00% <0.00%> (ø)
...rc/editor/find_replace_menu/find_menu_service.dart 82.92% <82.92%> (ø)
.../command_shortcut_events/find_replace_command.dart 88.88% <88.88%> (ø)
.../editor/find_replace_menu/find_replace_widget.dart 93.15% <93.15%> (ø)
...b/src/editor/find_replace_menu/search_service.dart 98.94% <98.94%> (ø)
.../block_component/rich_text/appflowy_rich_text.dart 92.17% <100.00%> (+0.27%) ⬆️
lib/src/editor/command/text_commands.dart 93.75% <100.00%> (ø)
...ditor_component/service/scroll_service_widget.dart 83.72% <100.00%> (+0.58%) ⬆️
... and 7 more

@MayurSMahajan MayurSMahajan force-pushed the fr_find_dialog_1760 branch from 3e8eec5 to 0b0ad4d Compare May 1, 2023 18:09
@MayurSMahajan MayurSMahajan marked this pull request as ready for review May 1, 2023 18:23
lib/src/history/undo_manager.dart Outdated Show resolved Hide resolved
lib/src/history/undo_manager.dart Outdated Show resolved Hide resolved
lib/src/render/find_replace_menu/find_replace_widget.dart Outdated Show resolved Hide resolved
lib/src/render/find_replace_menu/find_replace_widget.dart Outdated Show resolved Hide resolved
lib/src/render/find_replace_menu/search_service.dart Outdated Show resolved Hide resolved
lib/src/render/find_replace_menu/search_service.dart Outdated Show resolved Hide resolved
@MayurSMahajan
Copy link
Contributor Author

@Xazin I wanted to ask you this: Currently I am using the default highlight color which comes from the editor styles to highlight found matches, should I use a different color instead?

Copy link

@a-wallen a-wallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, I'm stoked for this. Could you add a handful of unit tests for the search algorithm?

@MayurSMahajan
Copy link
Contributor Author

MayurSMahajan commented May 21, 2023

Sure @a-wallen !

@LucasXu0 LucasXu0 marked this pull request as draft July 10, 2023 10:52
@Xazin
Copy link
Contributor

Xazin commented Jul 22, 2023

@Xazin, the core feature is complete with this set of commits, however, there are two issues to address:

  1. I am using a constant color for highlighting, we want to be able to use a customizable color instead, currently, @LucasXu0 and I are discussing the same.

  2. When multiple matches are found, the scrolling speed while navigating between these found matches is very slow, this is something I would you to take a look at, if possible.

Hey Mayur, thanks for the update.

If possible leave these two issues to me, I will take a look on Monday and give you an update. 👌

@Xazin Xazin self-assigned this Jul 22, 2023
@MayurSMahajan
Copy link
Contributor Author

@Xazin, the core feature is complete with this set of commits, however, there are two issues to address:

  1. I am using a constant color for highlighting, we want to be able to use a customizable color instead, currently, @LucasXu0 and I are discussing the same.
  2. When multiple matches are found, the scrolling speed while navigating between these found matches is very slow, this is something I would you to take a look at, if possible.

Hey Mayur, thanks for the update.

If possible leave these two issues to me, I will take a look on Monday and give you an update. 👌

Sure!

@Xazin Xazin self-requested a review July 25, 2023 09:54
@Xazin Xazin requested a review from LucasXu0 July 28, 2023 14:54
@Xazin Xazin marked this pull request as ready for review August 2, 2023 06:18
@Xazin
Copy link
Contributor

Xazin commented Aug 3, 2023

CC @LucasXu0

@LucasXu0
Copy link
Collaborator

@Xazin @MayurSMahajan Is this PR ready for merging? The CI failed.

@Xazin
Copy link
Contributor

Xazin commented Aug 10, 2023

@Xazin @MayurSMahajan Is this PR ready for merging? The CI failed.

I'm fixing it right now and will merge in 2 minutes. (+ some, need to fix tests)

@Xazin Xazin merged commit f5c6dc2 into AppFlowy-IO:main Aug 10, 2023
@MayurSMahajan MayurSMahajan deleted the fr_find_dialog_1760 branch August 10, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants