fix: Don't rely on `removeDiacritics` to highlight text #4636

g123k · 2023-09-15T09:29:22Z

Hi everyone,

When we highlight suggestions, we use the removeDiacriticsmethod.
However, it replaces some characters (eg: œ) by 2 or more letters (eg: oe)

I use instead a simpler algorithm to only remove accents.

codecov-commenter · 2023-09-15T09:40:33Z

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (36fb948) 9.68% compared to head (b963ca2) 9.69%.
Report is 3 commits behind head on develop.

Files	Patch %	Lines
packages/smooth_app/lib/widgets/smooth_text.dart	12.50%	28 Missing ⚠️
...b/pages/product/nutrition_add_nutrient_button.dart	0.00%	6 Missing ⚠️
...app/lib/generic_lib/widgets/language_selector.dart	0.00%	5 Missing ⚠️
...oth_app/lib/pages/onboarding/country_selector.dart	0.00%	5 Missing ⚠️
...ages/preferences/user_preferences_search_page.dart	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           develop   #4636      +/-   ##
==========================================
+ Coverage     9.68%   9.69%   +0.01%     
==========================================
  Files          318     318              
  Lines        16124   16146      +22     
==========================================
+ Hits          1561    1565       +4     
- Misses       14563   14581      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

monsieurtanuki

Hi @g123k!

What happens then with œ vs. oe?

it's detected as similar (with removeDiacritics) but not highlighted (with _removeAccents)?
it's never considered similar (as not present in _removeAccents)?
should we still use removeDiacritics then, and couldn't we get rid of that package?

g123k · 2023-09-18T18:57:24Z

Hi @g123k!

What happens then with œ vs. oe?

it's detected as similar (with removeDiacritics) but not highlighted (with _removeAccents)?

it's never considered similar (as not present in _removeAccents)?

should we still use removeDiacritics then, and couldn't we get rid of that package?

The initial string with œ will count this as 1 character, vs 2 with removeDiacritics
The goal of _removeAccents is to ignore that kind of case, which will still be the œ character.

removeDiacritics is used elsewhere in the app, so I don't think we can't remove it :(

monsieurtanuki · 2023-09-19T07:19:51Z

The initial string with œ will count this as 1 character, vs 2 with removeDiacritics The goal of _removeAccents is to ignore that kind of case, which will still be the œ character.

removeDiacritics is used elsewhere in the app, so I don't think we can't remove it :(

My question is: will the search match for œ/oe and it's just the highlighting that will not look very good, OR will the search fail to detect the œ/oe similarity?

…s`, as it replaces some letters of 1 word into 2 or +

g123k · 2023-12-09T01:49:03Z

The algorithm was not fully working, by being tricked if the diacritic was in the text and/or in the filter.
Not it works both ways:

Screen.Recording.2023-12-09.at.02.44.52.mov

I've also added an Extension on String to give a global accent to removeAccent / removeDiacritic

monsieurtanuki · 2023-12-10T07:59:17Z

@g123k Ready for review?

g123k · 2023-12-10T08:13:54Z

Yes, that's OK now

monsieurtanuki

Hi @g123k!

In the PR there are many myString.toLowerCase().removeDiacritics() (and sometimes myString.removeDiacritics().toLowerCase(), which is a bit confusing).
We may be better off with a simpler method like String getComparisonSafeString(String) that would return a combo of removeDiacritics and toLowerCase always in the same order.

But my main question is: does your fix work for the initial issue with œ and oe, when filtering and when displaying? Probably worth a test.

monsieurtanuki · 2023-12-10T08:22:02Z

packages/smooth_app/lib/pages/product/nutrition_add_nutrient_button.dart

+                            .removeDiacritics()
+                            .toLowerCase()
+                            .contains(
+                              query!.removeDiacritics().toLowerCase().trim(),
+                            ),


Shouldn't that be first toLowerCase and the removeDiacritics, like on the rest of the code?

monsieurtanuki · 2023-12-10T08:26:13Z

packages/smooth_app/lib/widgets/smooth_text.dart

+/// An extension on [String]
+extension StringExtension on String {
+  /// Simple algorithm to only remove accents AND not diacritics.
+  String removeAccents() {


That method is never used, right? We'd be better off without it, then.

I hesitate on this, as removeDiactrictics requires a lot of memory + CPU.
The idea of this method is to have a quicker way to remove accents, not all, but 90%

Then please get rid of package:diacritic/diacritic.dart instead.
We can't have two methods that do the same thing and one being never used ("your" removeAccents).
If you're confident enough in your faster code, let's use it!

g123k · 2023-12-11T13:11:03Z

Hi @g123k!

In the PR there are many myString.toLowerCase().removeDiacritics() (and sometimes myString.removeDiacritics().toLowerCase(), which is a bit confusing). We may be better off with a simpler method like String getComparisonSafeString(String) that would return a combo of removeDiacritics and toLowerCase always in the same order.

But my main question is: does your fix work for the initial issue with œ and oe, when filtering and when displaying? Probably worth a test.

Ok for the suggestion, but for your question, my video answers it, isn't-it?
Or is there something I'm missing?

…isonSafeString`

monsieurtanuki

Hi @g123k!
Fair enough, the video is convincing!
Thank you for taking into account my comments.
Still, it's problematic to have two methods doing the same thing, one that you coded, that goes fast but is never called, and one from an external package that is more demanding and that is the only one called.
Please get rid of one of them: both methods look OK, but we can't have both.

monsieurtanuki · 2023-12-11T14:27:18Z

packages/smooth_app/lib/generic_lib/widgets/language_selector.dart

-                                  .toLowerCase())
+                          _languages
+                              .getNameInEnglish(item)
+                              .getComparisonSafeString()
                              .contains(query!.toLowerCase()) ||


Suggested change

.contains(query!.toLowerCase()) ||

.contains(query!) ||

monsieurtanuki · 2023-12-11T14:27:42Z

packages/smooth_app/lib/generic_lib/widgets/language_selector.dart

-                                  .toLowerCase())
+                          _languages
+                              .getNameInLanguage(item)
+                              .getComparisonSafeString()
                              .contains(query.toLowerCase()) ||


Suggested change

.contains(query.toLowerCase()) ||

.contains(query!) ||

monsieurtanuki · 2023-12-11T14:32:51Z

packages/smooth_app/lib/widgets/smooth_text.dart

+/// An extension on [String]
+extension StringExtension on String {
+  /// Simple algorithm to only remove accents AND not diacritics.
+  String removeAccents() {


Then please get rid of package:diacritic/diacritic.dart instead.
We can't have two methods that do the same thing and one being never used ("your" removeAccents).
If you're confident enough in your faster code, let's use it!

g123k · 2023-12-11T20:08:22Z

Hi @g123k! Fair enough, the video is convincing! Thank you for taking into account my comments. Still, it's problematic to have two methods doing the same thing, one that you coded, that goes fast but is never called, and one from an external package that is more demanding and that is the only one called. Please get rid of one of them: both methods look OK, but we can't have both.

Actually I use the diacritics package, it's just that I want to hide the import of the lib in all the app.
RemoveAccents is only custom code, but without the case of the œ

monsieurtanuki · 2023-12-11T20:29:31Z

Actually I use the diacritics package, it's just that I want to hide the import of the lib in all the app.

Understood.

RemoveAccents is only custom code, but without the case of the œ

It is dead code. We don't need dead code, do we?

g123k · 2023-12-12T10:25:06Z

Ok, I've removed the method + added a test for the new method.
These tests are not very useful (in terms of complexity), but they ensure no regression.

monsieurtanuki

Thank you @g123k for this PR!

teolemon · 2023-12-16T11:07:58Z

Thanks @g123k and @monsieurtanuki for the review

g123k added ✏️ Editing Many products are incomplete and don't have Nutri-Score, Eco-Score…so editing is important for users ✏️ Editing - Text fields labels Sep 15, 2023

g123k requested a review from a team as a code owner September 15, 2023 09:29

g123k self-assigned this Sep 15, 2023

g123k force-pushed the suggestion_crash_diatric branch from 106cce3 to bd40471 Compare September 15, 2023 09:31

g123k linked an issue Sep 15, 2023 that may be closed by this pull request

Suggestions: the app crashes with the "oeuf" keyword #4635

Closed

monsieurtanuki reviewed Sep 15, 2023

View reviewed changes

g123k added 2 commits December 9, 2023 00:36

When we highlight a suggestion, we can't rely on the `removeDiacritic…

72f6d5a

…s`, as it replaces some letters of 1 word into 2 or +

Better algorithm

7af2c89

g123k force-pushed the suggestion_crash_diatric branch from bd40471 to 7af2c89 Compare December 9, 2023 01:47

github-actions bot added 🥫 Product page 🤗 Onboarding We need to onboard users on how the app works, but also on content like Nutri-Score, Eco-Score… 🧪 Tests labels Dec 9, 2023

g123k added 3 commits December 9, 2023 02:50

Remove unused things

6a775a6

Fix typo

a8fddda

Unnecessary brackets

daecba1

monsieurtanuki self-requested a review December 10, 2023 08:32

monsieurtanuki reviewed Dec 10, 2023

View reviewed changes

g123k added 2 commits December 11, 2023 14:17

Replace all toLowerCase + removeDiacritics by a single `getCompar…

23caf4f

…isonSafeString`

Reformat CountrySelector

d29e34e

monsieurtanuki reviewed Dec 11, 2023

View reviewed changes

Merge branch 'develop' into suggestion_crash_diatric

a2cb4aa

removeAccents removed + 1 new test

b963ca2

monsieurtanuki approved these changes Dec 12, 2023

View reviewed changes

teolemon merged commit e904b05 into openfoodfacts:develop Dec 16, 2023
6 checks passed

github-actions bot mentioned this pull request Dec 16, 2023

chore(develop): release 4.14.0 #4894

Merged

g123k mentioned this pull request Jan 8, 2024

[Discussion] Are we ready for a release? #4961

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Don't rely on `removeDiacritics` to highlight text #4636

fix: Don't rely on `removeDiacritics` to highlight text #4636

g123k commented Sep 15, 2023

codecov-commenter commented Sep 15, 2023 •

edited

Loading

monsieurtanuki left a comment

g123k commented Sep 18, 2023

monsieurtanuki commented Sep 19, 2023

g123k commented Dec 9, 2023 •

edited

Loading

monsieurtanuki commented Dec 10, 2023

g123k commented Dec 10, 2023

monsieurtanuki left a comment

monsieurtanuki Dec 10, 2023

monsieurtanuki Dec 10, 2023

g123k Dec 11, 2023

monsieurtanuki Dec 11, 2023

g123k commented Dec 11, 2023

monsieurtanuki left a comment

monsieurtanuki Dec 11, 2023

monsieurtanuki Dec 11, 2023

monsieurtanuki Dec 11, 2023

g123k commented Dec 11, 2023

monsieurtanuki commented Dec 11, 2023

g123k commented Dec 12, 2023

monsieurtanuki left a comment

teolemon commented Dec 16, 2023

fix: Don't rely on removeDiacritics to highlight text #4636

fix: Don't rely on removeDiacritics to highlight text #4636

Conversation

g123k commented Sep 15, 2023

codecov-commenter commented Sep 15, 2023 • edited Loading

Codecov Report

monsieurtanuki left a comment

Choose a reason for hiding this comment

g123k commented Sep 18, 2023

monsieurtanuki commented Sep 19, 2023

g123k commented Dec 9, 2023 • edited Loading

monsieurtanuki commented Dec 10, 2023

g123k commented Dec 10, 2023

monsieurtanuki left a comment

Choose a reason for hiding this comment

monsieurtanuki Dec 10, 2023

Choose a reason for hiding this comment

monsieurtanuki Dec 10, 2023

Choose a reason for hiding this comment

g123k Dec 11, 2023

Choose a reason for hiding this comment

monsieurtanuki Dec 11, 2023

Choose a reason for hiding this comment

g123k commented Dec 11, 2023

monsieurtanuki left a comment

Choose a reason for hiding this comment

monsieurtanuki Dec 11, 2023

Choose a reason for hiding this comment

monsieurtanuki Dec 11, 2023

Choose a reason for hiding this comment

monsieurtanuki Dec 11, 2023

Choose a reason for hiding this comment

g123k commented Dec 11, 2023

monsieurtanuki commented Dec 11, 2023

g123k commented Dec 12, 2023

monsieurtanuki left a comment

Choose a reason for hiding this comment

teolemon commented Dec 16, 2023

fix: Don't rely on `removeDiacritics` to highlight text #4636

fix: Don't rely on `removeDiacritics` to highlight text #4636

codecov-commenter commented Sep 15, 2023 •

edited

Loading

g123k commented Dec 9, 2023 •

edited

Loading