Feat: search bar improved

[Alspb]

Description

Search bar improved: alignment option added, search settings changed from icons to checkboxes with titles, search happens on the fly, other minor improvements.

Checklist

• [x] I read the Contributor Guide and followed the process outlined there for submitting PRs.
• [x] I titled the PR using Conventional Commits.
• [x] I did not modify the CHANGELOG.md nor the package version in pubspec.yaml files.
• [x] All existing and new tests are passing.
• [ ] I have run the commands in ./scripts/before_push.sh and it all passed successfully

Breaking Change

Does your PR require developers to manually update their apps to accommodate your change?

• [ ] Yes, this is a breaking change (please indicate that with a ! in the title as explained in Conventional Commits).
• [x] No, this is not a breaking change.

[Alspb]

@AtlasAutocode , please take a look at the improved search bar. In particular, "Search" button is removed since search is now on-the-fly (i.e. when the user enters text).

@ellet0 , I added searchBarAlignment parameter into search button options. It's just responsible for the search dialog alignment (previously the search dialog was always at the bottom thus overlapping the editor if the toolbar is on the top). Please double-check if you're fine with how this parameter is set in the code. If you could help with the translation ('Case sensitive and whole word' is now split into 'Case sensitive' and 'Whole word' separately, also 'Close' and 'Search settings' tooltips added.), it would be perfect.

[EchoEllet]

Thank you for your contributions

"Search" button is removed since search is now on-the-fly (i.e. when the user enters text).

This might be considered as breaking change by some developers. It doesn't require additional changes to work but changes the behavior

Can we add an option to change the behavior? Also, for large documents, this can be more resources expensive

 I added searchBarAlignment parameter into search button options. It's just responsible for the search dialog alignment (previously the search dialog was always at the bottom thus overlapping the editor if the toolbar is on the top). Please double-check if you're fine with how this parameter is set in the code.

I'm okay with either case

If you could help with the translation ('Case sensitive and whole word' is now split into 'Case sensitive' and 'Whole word' separately, also 'Close' and 'Search settings' tooltips added.), it would be perfect.

I'm not near my main machine at the moment, I will back soon to review, test the changes and the translations

Thank you once again.

[singerdmx]

I think we just need to bump major version. It is ok to have breaking change that changes behavior

[singerdmx]

I mean there is no need to have an option to go back to old behavior which is tedious to maintain and readability is bad.

[EchoEllet]

('Case sensitive and whole word' is now split into 'Case sensitive' and 'Whole word' separately, also 'Close' and 'Search settings' tooltips added.), it would be perfect.

Added in 592da6c7438b995bfeb7231f66dfd834bda546b3 commit, pull the changes from the upstream master branch to your branch so you can start using the new translations, notice I only added the keys and used the script to update untranslated.json to help developers translate them to their languages more easily, maybe we should use some tool or script to help automating the translations as a default values as the translations we have are pretty simple and can be automated, this is not directly related to the PR

[Alspb]

Can we add an option to change the behavior? Also, for large documents, this can be more resources expensive

I mean there is no need to have an option to go back to old behavior which is tedious to maintain and readability is bad.

Sure, there's no problem to wait for the major release. It looks like the only difference between launching the search with a dedicated "Search" button and on-the-fly is possible performance issues when the search runs multiple times. To address this the search is launched 0.3 sec after the user stops entering the text (into the search field). So, in most of the cases the search should run only once, after the user finishes the input. But of course it's not a problem to deliberately enter the text in a way that the search runs multiple times. However, there shouldn't be any visible lags anyway since the search itself runs fast enough even for long text. For instance, I've checked the example text extended 100 times Document.fromJson([for (var i = 0; i < 100; i++) quillTextSample].expand((x) => x).toList()) The search itself takes 0.2 sec on Android emulator even in that case, while entering a word into the editor takes more than a minute! So, the editor currently can't handle text long enough to cause the search to be really slow.

[EchoEllet]

Adding an option to have a delay seems a good idea, While I prefer the new widget, I still suggest adding an option for different styles or widget implementation. An enum class with two options will do. If the logic is too different, then we could create another widget in the same folder for the old widget (before this PR) that has the bussniues logic, UI logic, and logic in general, the if check should be in the simple toolbar widget to minimize the conditional checks when using custom toolbar or using the components outside of the toolbar

• If you would like to do it yourself
• or leave it to me
• make a breaking change

I suggest any of the first two instead of the third

the choice is up to you.

[Alspb]

I tried to combine these two widgets but the logic is different in several places of the code, so that the whole widget becomes looking overcomplicated. At the same time, most of the logic and UI is the same, so two different widgets cause code duplication. Probably the approach you suggest might be fine but I'm not sure I can implement it in the best way. Could you please make a try?

[EchoEllet]

I tried to combine these two widgets but the logic is different in several places of the code, so that the whole widget becomes looking overcomplicated.

I agree with you, it's better not to combine the two in the same widget to avoid too many conditional checks and other issues as we both mentioned above.

What I suggested is that we use the old widget (before merging this PR), refactor the name to something else, or to avoid breaking changes, we could separate this new widget with a different name that's descriptive and clear and use that as a default in QuillSimpleToolbar which is what I suggest.

In short, I suggest not changing the current search bar/button widget (revert the changes) but instead introducing a new one and using it as default in the Toolbar.

Could you please make a try?

Sure, I will wait for your response first.

[singerdmx]

@Alspb any update?

[Alspb]

@ellet0 , yeah, it's possible to use two widgets. There is an inconvenience that they share a significant portion of common code of course, but these widgets are relatively simple, so it shouldn't significantly complicate their maintenance.

[singerdmx]

@ellet0 do you think we can go ahead merging this change and follow up with additional changes as needed?

[EchoEllet]

@ellet0 do you think we can go ahead merging this change and follow up with additional changes as needed?

Yes, I will restore the old widget with a different name and make some changes that fit the rest of the options, configurations, and customization in other widgets as soon as I can.

An enum class will be used to change the widget to the new one in the QuillSimpleToolbar

Might make the old widget as default to avoid breaking behavior or the new one to simplify the development and configurations for new developers.

Developers who use a custom toolbar will have to use either the old or new widget to avoid conditional checks.

Or we could publish a new version directly with this new widget is as.

[singerdmx]

Sounds good. I will go ahead merging this PR and you can patch it with changes later.

[singerdmx]

hmm, not sure why CI failed on this commit

[EchoEllet]

hmm, not sure why CI failed on this commit

I tried running the workflow again, and it succeeded 26101906925

I'm not sure why it didn't show any details previously. The actions and steps or where it's failing.