Feat: search bar improved #1904

Alspb · 2024-05-28T23:21:06Z

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

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I titled the PR using Conventional Commits.
I did not modify the CHANGELOG.md nor the package version in pubspec.yaml files.
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).
No, this is not a breaking change.

…from icons to checkboxes with titles, search happens on the fly, other minor improvements

Alspb · 2024-05-28T23:42:49Z

@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 · 2024-05-28T23:54:16Z

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 · 2024-05-29T03:55:15Z

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

singerdmx · 2024-05-29T03:56:13Z

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

Just a few changes to be addressed, I will update the quill_en.arb file to include the new needed translations, you just have to pull the changes by merging them or rebase to your branch and use the new translations

EchoEllet · 2024-05-29T17:41:06Z

lib/src/widgets/toolbar/buttons/search/search_button.dart

+  // Color _dialogBarrierColor(BuildContext context) {
+  //   return options.dialogBarrierColor ??
+  //       context.quillSharedConfigurations?.dialogBarrierColor ??
+  //       Colors.black54;
+  // }


This would allow customizing the dialog barrier color for all the dialogs, the previous code is far from perfect but if the new default for this dialog is Colors.transparent then we should consider applying it in this function, if we decide to remove function, it's better to remove the comments of the previous code as the code will get outdated quickly since the IDE will ignore the commented code, and it makes the code a little bit less readable

In my opinion, the search bar is something in between of a regular bar (like AppBar) and a dialog, since it should be dismissed when the user taps outside it. So, that's natural to have transparent barrierColor, otherwise the editor content is obscured when the search is on. Do you have any idea why barrierColor property could be of use here? If it's not, than I'd suggest to remove the commented code above and mark dialogBarrierColor option as deprecated.

Do you have any idea why barrierColor property could be of use here?

There was a feature request, but I don't exactly remember the problem. Maybe they used a different theme or some different color/custom component something like this. They wanted an option to change it as the package depends on Material 3, developers already expecting this, we did too many breaking changes in the last year (mostly by me), and it was disturbing for developers who are trying to get their job done

I suggest minimizing the breaking changes and doing it only when it's really necessary, especially for the next two years

In this case, it's possible to not break change without any disadvantages

if we all agreed on removing this property, this change of barrier color, the change should be in a separate PR. There are other places, and almost all dialogs have this option. Having an option to work on all dialogs except one might be seems as a bug or inconsistency.

Looks like the cause of that differences is that ideally search bar shouldn't be implemented as dialog but as some bar at the top/bottom of the editor, maybe replacing the toolbar to save space on mobile devices. Sure, the editor's content shouldn't change during the search but there might be other ways to achieve it except using a dialog.
The problem is that it's hard to implement the search bar that way, and the value of that change is not that significant.

But since it's currently a dialog, that's reasonable to preserve the solutions made for dialogs.
At the same time, providing non-transparent barrier color for search bar by default looks more like a bug than a feature.
The problem is that context.quillSharedConfigurations?.dialogBarrierColor has the default value "black". It means that Colors.black54 from the commented code above is not used anyway, so replacing it with Colors.transparent doesn't help.
Are you ok if we make QuillToolbarSearchButtonOptions.dialogBarrierColor to have the default value Colors.transparent (instead of null)? But in that case context.quillSharedConfigurations?.dialogBarrierColor will still have no impact...

EchoEllet · 2024-05-29T17:41:36Z

lib/src/widgets/toolbar/buttons/search/search_button.dart

@@ -124,13 +124,13 @@ class QuillToolbarSearchButton extends StatelessWidget {
      return;
    }
    await showDialog<String>(
-      barrierColor: _dialogBarrierColor(context),
+      barrierColor: Colors.transparent,


If we keep the previous function, then this should be: _dialogBarrierColor(context)

EchoEllet · 2024-05-29T17:43:12Z

lib/src/widgets/toolbar/buttons/search/search_dialog.dart

+        children: [
+          IconButton(
+            icon: const Icon(Icons.close),
+            tooltip: 'Close',


It doesn't need to be translated to 40 languages but we should consider adding this to quill_en.arb file, it will update the untranslated.json file to allow developers to update translations easily

EchoEllet · 2024-05-29T17:43:55Z

lib/src/widgets/toolbar/buttons/search/search_dialog.dart

+              visualDensity: VisualDensity.compact,
+              contentPadding: EdgeInsets.zero,
+              title: const Text(
+                'Case sensitive',


Same like above, const has to be removed once the change is done

EchoEllet · 2024-05-29T17:44:45Z

lib/src/widgets/toolbar/buttons/search/search_dialog.dart

+          IconButton(
+            icon: const Icon(Icons.more_vert),
+            isSelected: _caseSensitive || _wholeWord,
+            tooltip: 'Search settings',


Same as above

EchoEllet · 2024-05-29T17:46:56Z

lib/src/widgets/toolbar/buttons/search/search_dialog.dart

+      if (_offsets.isEmpty) {
+        matchShown = '0/0';
+      } else {
+        matchShown = '${_index + 1}/${_offsets.length}';


Maybe this should be translated too? with using some variables inside the translate so the result won't be a variable, instead, it will be a function and then we will pass the ${_index + 1} and ${_offsets.length}

the / and the direction of the text can be different in different languages

In think it's better to be in-line with how it looks in Chrome, Firefox, Edge, etc. Are you able to check how it looks there for right-to-left languages?

In think it's better to be in line with how it looks in Chrome, Firefox, Edge, etc. Are you able to check how it looks there for right-to-left languages?

Yes, I checked how it looked in Rtl last year

Usually, flutter and flutter localizations take care of this without changing much in the UI code. In most cases, you don't have to change almost anything.

If you want to see how it looks, change the text direction to Rtl in MaterialApp/WidgetsApp/CupertinoApp

Or use one of those languages:
https://www.andiamo.co.uk/resources/right-to-left-languages/

Either by temporarily changing the device/emulator language or explicit use of local in the app widget

Or use theDirectionality widget to override this

I should have added an option in the example to change the language in the app

Couldn't run Rtl for the app, need more time. But in Chrome (meaning the browser itself, not a web-version of the editor) in Arabic the search looks like that

So, it looks like the numbers could be translated while "/" itself - not necessary.
But I fully trust you opinion in translations, so please feel free to translate matchShown as you think it's optimal.

EchoEllet · 2024-05-29T17:49:23Z

lib/src/widgets/toolbar/buttons/search/search_dialog.dart

@@ -42,55 +45,40 @@ class QuillToolbarSearchDialog extends StatefulWidget {
  const QuillToolbarSearchDialog({
    required this.controller,
    this.dialogTheme,
-    this.text,


This sounds like a breaking change, not all developers use the built-in toolbar, some of them use the widgets to create their own toolbar, if we don't have to support this anymore, add @Deprecated with a message of your choice that indicates this is no longer used

Or we could still use this if you want to, in that case, the _text = widget.text ?? ''; shouldn't be removed from the initState

It's your choice. I suggest option 2 as other widgets might already have an empty string as a default and to minimize deprecations and breaking changes.

Initially I thought that there's no need in initial text in the search bar. But on the second thought I see that some developers might use it to remember the previous search the user did. So, I'll return this.text.

EchoEllet · 2024-05-29T17:59:06Z

('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 · 2024-06-02T14:30:29Z

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 · 2024-06-02T18:29:49Z

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.

…search_bar

Alspb · 2024-06-05T00:52:46Z

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 · 2024-06-05T05:34:20Z

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 · 2024-06-09T03:22:25Z

@Alspb any update?

Alspb · 2024-06-11T00:03:03Z

@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 · 2024-06-11T17:53:00Z

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

EchoEllet · 2024-06-11T18:19:24Z

@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 · 2024-06-11T21:04:52Z

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

singerdmx · 2024-06-11T21:10:45Z

hmm, not sure why CI failed on this commit

EchoEllet · 2024-06-11T22:44:58Z

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.

Alspb added 2 commits May 29, 2024 00:12

Search bar improved: alignment option added, search settings changed …

c23cb4e

…from icons to checkboxes with titles, search happens on the fly, other minor improvements

Minor refactoring

e8fe411

EchoEllet requested changes May 29, 2024

View reviewed changes

Merge branch 'master' of https://github.com/Alspb/flutter-quill into …

fd4ea4f

…search_bar

Search bar: translations added, minor fixes

17695ae

singerdmx approved these changes Jun 11, 2024

View reviewed changes

singerdmx merged commit 75f2f2d into singerdmx:master Jun 11, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: search bar improved #1904

Feat: search bar improved #1904

Alspb commented May 28, 2024

Alspb commented May 28, 2024

EchoEllet commented May 28, 2024 •

edited

Loading

singerdmx commented May 29, 2024

singerdmx commented May 29, 2024

EchoEllet left a comment

EchoEllet May 29, 2024

Alspb Jun 2, 2024

EchoEllet Jun 2, 2024 •

edited

Loading

Alspb Jun 5, 2024

EchoEllet May 29, 2024

EchoEllet May 29, 2024

EchoEllet May 29, 2024

EchoEllet May 29, 2024

EchoEllet May 29, 2024

Alspb Jun 2, 2024

EchoEllet Jun 2, 2024 •

edited

Loading

Alspb Jun 5, 2024 •

edited

Loading

EchoEllet May 29, 2024 •

edited

Loading

Alspb Jun 2, 2024

EchoEllet commented May 29, 2024

Alspb commented Jun 2, 2024

EchoEllet commented Jun 2, 2024

Alspb commented Jun 5, 2024

EchoEllet commented Jun 5, 2024 •

edited

Loading

singerdmx commented Jun 9, 2024

Alspb commented Jun 11, 2024

singerdmx commented Jun 11, 2024

EchoEllet commented Jun 11, 2024

singerdmx commented Jun 11, 2024

singerdmx commented Jun 11, 2024

EchoEllet commented Jun 11, 2024

Feat: search bar improved #1904

Feat: search bar improved #1904

Conversation

Alspb commented May 28, 2024

Description

Checklist

Breaking Change

Alspb commented May 28, 2024

EchoEllet commented May 28, 2024 • edited Loading

singerdmx commented May 29, 2024

singerdmx commented May 29, 2024

EchoEllet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EchoEllet Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EchoEllet Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Alspb Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

EchoEllet May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EchoEllet commented May 29, 2024

Alspb commented Jun 2, 2024

EchoEllet commented Jun 2, 2024

Alspb commented Jun 5, 2024

EchoEllet commented Jun 5, 2024 • edited Loading

singerdmx commented Jun 9, 2024

Alspb commented Jun 11, 2024

singerdmx commented Jun 11, 2024

EchoEllet commented Jun 11, 2024

singerdmx commented Jun 11, 2024

singerdmx commented Jun 11, 2024

EchoEllet commented Jun 11, 2024

EchoEllet commented May 28, 2024 •

edited

Loading

EchoEllet Jun 2, 2024 •

edited

Loading

EchoEllet Jun 2, 2024 •

edited

Loading

Alspb Jun 5, 2024 •

edited

Loading

EchoEllet May 29, 2024 •

edited

Loading

EchoEllet commented Jun 5, 2024 •

edited

Loading