-
-
Notifications
You must be signed in to change notification settings - Fork 280
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
fix: Don't rely on removeDiacritics
to highlight text
#4636
Changes from 5 commits
72f6d5a
7af2c89
6a775a6
a8fddda
daecba1
23caf4f
d29e34e
a2cb4aa
b963ca2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,3 @@ | ||||||
import 'package:diacritic/diacritic.dart'; | ||||||
import 'package:flutter/material.dart'; | ||||||
import 'package:flutter_gen/gen_l10n/app_localizations.dart'; | ||||||
import 'package:openfoodfacts/openfoodfacts.dart'; | ||||||
|
@@ -146,19 +145,21 @@ class LanguageSelector extends StatelessWidget { | |||||
prefixIcon: const Icon(Icons.search), | ||||||
controller: languageSelectorController, | ||||||
onChanged: (String? query) { | ||||||
query = removeDiacritics(query!.trim().toLowerCase()); | ||||||
query = query!.trim().toLowerCase().removeDiacritics(); | ||||||
|
||||||
setState( | ||||||
() { | ||||||
filteredList = leftovers | ||||||
.where((OpenFoodFactsLanguage item) => | ||||||
removeDiacritics(_languages | ||||||
.getNameInEnglish(item) | ||||||
.toLowerCase()) | ||||||
_languages | ||||||
.getNameInEnglish(item) | ||||||
.toLowerCase() | ||||||
.removeDiacritics() | ||||||
.contains(query!.toLowerCase()) || | ||||||
removeDiacritics(_languages | ||||||
.getNameInLanguage(item) | ||||||
.toLowerCase()) | ||||||
_languages | ||||||
.getNameInLanguage(item) | ||||||
.toLowerCase() | ||||||
.removeDiacritics() | ||||||
.contains(query.toLowerCase()) || | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
item.code.contains(query)) | ||||||
.toList(); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
import 'package:diacritic/diacritic.dart'; | ||
import 'package:flutter/material.dart'; | ||
import 'package:flutter_gen/gen_l10n/app_localizations.dart'; | ||
import 'package:openfoodfacts/openfoodfacts.dart'; | ||
|
@@ -55,9 +54,14 @@ class NutritionAddNutrientButton extends StatelessWidget { | |
controller: nutritionTextController, | ||
onChanged: (String? query) => setState( | ||
() => filteredList = leftovers | ||
.where((OrderedNutrient item) => | ||
removeDiacritics(item.name!).toLowerCase().contains( | ||
removeDiacritics(query!).toLowerCase().trim())) | ||
.where( | ||
(OrderedNutrient item) => item.name! | ||
.removeDiacritics() | ||
.toLowerCase() | ||
.contains( | ||
query!.removeDiacritics().toLowerCase().trim(), | ||
), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't that be first |
||
) | ||
.toList(), | ||
), | ||
), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,30 @@ | ||
import 'package:diacritic/diacritic.dart'; | ||
import 'package:diacritic/diacritic.dart' as lib show removeDiacritics; | ||
import 'package:flutter/material.dart'; | ||
import 'package:smooth_app/services/smooth_services.dart'; | ||
|
||
/// An extension on [String] | ||
extension StringExtension on String { | ||
/// Simple algorithm to only remove accents AND not diacritics. | ||
String removeAccents() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That method is never used, right? We'd be better off without it, then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hesitate on this, as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then please get rid of |
||
const String withAccents = | ||
'ÀÁÂÃÄÅàáâãäåÒÓÔÕÕÖØòóôõöøÈÉÊËèéêëðÇçÐÌÍÎÏìíîïÙÚÛÜùúûüÑñŠšŸÿýŽž'; | ||
const String withoutAccents = | ||
'AAAAAAaaaaaaOOOOOOOooooooEEEEeeeeeCcDIIIIiiiiUUUUuuuuNnSsYyyZz'; | ||
|
||
String str = this; | ||
for (int i = 0; i < withAccents.length; i++) { | ||
str = str.replaceAll(withAccents[i], withoutAccents[i]); | ||
} | ||
|
||
return str; | ||
} | ||
|
||
/// Please use this method instead of directly calling the library. | ||
/// It will ease the migration if we decide to remove/change it. | ||
String removeDiacritics() { | ||
return lib.removeDiacritics(this); | ||
} | ||
} | ||
|
||
/// An extension on [TextStyle] that allows to have "well spaced" variant | ||
extension TextStyleExtension on TextStyle { | ||
|
@@ -60,13 +85,23 @@ class TextHighlighter extends StatelessWidget { | |
|
||
@override | ||
Widget build(BuildContext context) { | ||
final List<(String, TextStyle?)> parts = _getParts( | ||
defaultStyle: TextStyle(fontWeight: selected ? FontWeight.bold : null), | ||
highlightedStyle: TextStyle( | ||
fontWeight: selected ? FontWeight.bold : null, | ||
backgroundColor: Theme.of(context).primaryColor.withOpacity(0.2), | ||
), | ||
); | ||
List<(String, TextStyle?)> parts; | ||
try { | ||
parts = _getParts( | ||
defaultStyle: TextStyle(fontWeight: selected ? FontWeight.bold : null), | ||
highlightedStyle: TextStyle( | ||
fontWeight: selected ? FontWeight.bold : null, | ||
backgroundColor: Theme.of(context).primaryColor.withOpacity(0.2), | ||
), | ||
); | ||
} catch (e, trace) { | ||
parts = <(String, TextStyle?)>[(text, null)]; | ||
Logs.e( | ||
'Unable to parse text "$text" with filter "$filter".', | ||
ex: e, | ||
stacktrace: trace, | ||
); | ||
} | ||
|
||
final TextStyle defaultTextStyle = DefaultTextStyle.of(context).style; | ||
|
||
|
@@ -91,9 +126,12 @@ class TextHighlighter extends StatelessWidget { | |
required TextStyle? defaultStyle, | ||
required TextStyle? highlightedStyle, | ||
}) { | ||
final String filterWithoutDiacritics = filter.removeDiacritics(); | ||
final String textWithoutDiacritics = text.removeDiacritics(); | ||
|
||
final Iterable<RegExpMatch> highlightedParts = | ||
RegExp(removeDiacritics(filter).toLowerCase().trim()).allMatches( | ||
removeDiacritics(text).toLowerCase(), | ||
RegExp(filterWithoutDiacritics.toLowerCase().trim()).allMatches( | ||
textWithoutDiacritics.toLowerCase(), | ||
); | ||
|
||
final List<(String, TextStyle?)> parts = <(String, TextStyle?)>[]; | ||
|
@@ -103,24 +141,61 @@ class TextHighlighter extends StatelessWidget { | |
} else { | ||
parts | ||
.add((text.substring(0, highlightedParts.first.start), defaultStyle)); | ||
int diff = 0; | ||
|
||
for (int i = 0; i != highlightedParts.length; i++) { | ||
final RegExpMatch subPart = highlightedParts.elementAt(i); | ||
final int startPosition = subPart.start - diff; | ||
final int endPosition = _computeEndPosition( | ||
startPosition, | ||
subPart.end - diff, | ||
subPart, | ||
textWithoutDiacritics, | ||
filterWithoutDiacritics, | ||
); | ||
diff = subPart.end - endPosition; | ||
|
||
parts.add( | ||
(text.substring(subPart.start, subPart.end), highlightedStyle), | ||
(text.substring(startPosition, endPosition), highlightedStyle), | ||
); | ||
|
||
if (i < highlightedParts.length - 1) { | ||
parts.add(( | ||
text.substring( | ||
subPart.end, highlightedParts.elementAt(i + 1).start), | ||
endPosition, highlightedParts.elementAt(i + 1).start - diff), | ||
defaultStyle | ||
)); | ||
} else if (subPart.end < text.length) { | ||
parts.add((text.substring(subPart.end, text.length), defaultStyle)); | ||
} else if (endPosition < text.length) { | ||
parts.add((text.substring(endPosition, text.length), defaultStyle)); | ||
} | ||
} | ||
} | ||
return parts; | ||
} | ||
|
||
int _computeEndPosition( | ||
int startPosition, | ||
int endPosition, | ||
RegExpMatch subPart, | ||
String textWithoutDiacritics, | ||
String filterWithoutDiacritics, | ||
) { | ||
final String subText = text.substring(startPosition); | ||
if (subText.startsWith(filterWithoutDiacritics)) { | ||
return endPosition; | ||
} | ||
|
||
int diff = 0; | ||
for (int pos = 0; pos < endPosition; pos++) { | ||
if (pos == subText.length - 1) { | ||
diff = pos - subText.length; | ||
break; | ||
} | ||
|
||
final int charLength = subText[pos].removeDiacritics().length; | ||
diff -= charLength > 1 ? charLength - 1 : 0; | ||
} | ||
|
||
return endPosition + diff; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import 'package:flutter_test/flutter_test.dart'; | ||
import 'package:smooth_app/widgets/smooth_text.dart' show StringExtension; | ||
|
||
void main() { | ||
group('Smooth text', () { | ||
group('String extension', () { | ||
test('Remove accents', () { | ||
expect( | ||
'àáâãäåéèêëòóôõöìíîïùúûüñšÿýž'.removeAccents(), | ||
equals('aaaaaaeeeeoooooiiiiuuuunsyyz'), | ||
); | ||
}); | ||
test('Remove diacritics', () { | ||
expect( | ||
'àáâãäåéèêëòóôõöìíîïùúûüñšÿýž'.removeDiacritics(), | ||
equals('aaaaaaeeeeoooooiiiiuuuunsyyz'), | ||
); | ||
}); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.