fix: Move Hunger Games to a real view instead of overlay (#4681) #6555

LorenzoMascia · 2025-04-16T09:07:24Z

Committer: Lorenzo Mascia [email protected]

Changes :
modified: packages/smooth_app/lib/pages/hunger_games/question_card.dart
modified: packages/smooth_app/lib/pages/hunger_games/question_page.dart
modified: packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart
modified: packages/smooth_app/lib/pages/product/product_questions_widget.dart

What

Moved the Hunger Games screen from an overlay to a real view using a standard Scaffold, as described in the issue.
This improves navigation consistency and aligns the UI with the rest of the app.

fix: Move Hunger Games to real view instead of overlay

Screenshot

Fixes bug(s)

Fixes: Move Hunger Games to a real view, as opposed to an overlay #4681

monsieurtanuki · 2025-04-16T10:46:23Z

Hi @LorenzoMascia!
First thing, have your code formatted (cf. flutter format . and CTRL-S).

monsieurtanuki

Hi @LorenzoMascia!
Of course the first thing is to fix the formatting, but please read my other comments.

monsieurtanuki · 2025-04-16T10:42:28Z

packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart

+        builder: (context) => QuestionsPage( ),
+        ),
+      ).then((questionsAnswered) {
+      // Gestisci il risultato se necessario


Per favore, niente in italiano.

done, no more Italian

monsieurtanuki · 2025-04-16T10:43:30Z

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

@@ -77,20 +77,24 @@ class _ProductQuestionsWidgetState extends State<ProductQuestionsWidget>
  Future<void> _openQuestions() async {
    _trackEvent(AnalyticsEvent.questionClicked);

-    final int? answeredQuestions = await openQuestionPage(
+    Navigator.push(


No await? Then please make it explicit and use unawait(.

monsieurtanuki · 2025-04-16T10:48:02Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

-              _QuestionPageState._numberQuestionsNext,
+          ? AppLocalizations.of(context).robotoff_next_n_questions(10
+
+              // QuestionPageState._numberQuestionsNext,


No useless comments, please.

removed the comment and replaced the 10 with the variable numberQuestionsNext

monsieurtanuki · 2025-04-16T10:51:12Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

@@ -367,7 +281,7 @@ class _LoadingQuestionsView extends StatelessWidget {
          child: Center(
            child: DefaultTextStyle(
              textAlign: TextAlign.center,
-              style: const TextStyle(),
+              style: TextStyle(color: colorScheme.onBackground), 


Not sure what makes you use explicit colors.
Anyway, if you select onBackground as a background color, shouldn't you use background as a front color?
Shouldn't you rather use primary and onPrimary?
Or no color at all, just using the app and the device's default values?

used primary, and I had to specify the color because with the light theme the lettering was not visible

monsieurtanuki · 2025-04-16T10:54:12Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

+
+  void _incrementQuestionsAnswered() {
+    setState(() {
+      _questionsAnswered++;


Not sure how setState works but ++_questionsAnswered would be more appropriate.

monsieurtanuki · 2025-04-16T10:56:45Z

packages/smooth_app/lib/pages/hunger_games/question_card.dart

@@ -62,7 +62,7 @@ class QuestionCard extends StatelessWidget {
                mainAxisSize: MainAxisSize.min,
                children: <Widget>[
                  SizedBox(
-                    height: screenSize.height / 6,
+                    height: screenSize.height / 4,


Would an Expanded give us more flexibility?

using Expanded

monsieurtanuki · 2025-04-16T10:57:50Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

+      );
+      _currentQuestionIndex = 0;
+    } catch (e) {
+      debugPrint('Error loading next questions: $e');


No debugPrint, please.

debugPrint removed

monsieurtanuki · 2025-04-16T10:58:19Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

-    );
-    _currentQuestionIndex = 0;
+    try {
+      setState(() {


Generally speaking, you're supposed to test if(mounted) before calling setState.

monsieurtanuki · 2025-04-16T11:02:17Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

+    }
+  }
+
+  void _incrementQuestionsAnswered() {


Cannot see the added value of moving the code here.
It was more relevant to put that in _saveAnswer, wasn't it?

function removed

monsieurtanuki · 2025-04-16T11:05:23Z

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

+            .toList(growable: false),
+            ),
+          ),
+        ).then((questionsAnswered) {


We don't use that much the then syntax. Harder to read.
Please stick to the int? value = await myMethod(); if (value ... syntax.

changed the construct

codecov-commenter · 2025-04-16T17:46:15Z

Codecov Report

Attention: Patch coverage is 1.20482% with 82 lines in your changes missing coverage. Please review.

Project coverage is 5.84%. Comparing base (4d9c7fc) to head (53dc1de).
Report is 838 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ooth_app/lib/pages/hunger_games/question_page.dart	1.63%	60 Missing ⚠️
...b/pages/hunger_games/question_image_thumbnail.dart	0.00%	9 Missing ⚠️
...pp/lib/pages/product/product_questions_widget.dart	0.00%	8 Missing ⚠️
...pages/preferences/user_preferences_contribute.dart	0.00%	4 Missing ⚠️
...ooth_app/lib/pages/hunger_games/question_card.dart	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           develop   #6555      +/-   ##
==========================================
- Coverage     9.54%   5.84%   -3.70%     
==========================================
  Files          325     499     +174     
  Lines        16411   29817   +13406     
==========================================
+ Hits          1567    1744     +177     
- Misses       14844   28073   +13229

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

monsieurtanuki

Hi @LorenzoMascia, and thank you for your changes!
I like the Expanded version, though the image size isn't correctly computed (the bottom of the big image isn't visible). That needs to be fixed (maybe removing the SizedBox and letting the system compute by itself, I don't know).
Please read my other comments.

monsieurtanuki · 2025-04-18T06:28:19Z

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

@@ -77,16 +77,20 @@ class _ProductQuestionsWidgetState extends State<ProductQuestionsWidget>
  Future<void> _openQuestions() async {
    _trackEvent(AnalyticsEvent.questionClicked);

-    final int? answeredQuestions = await openQuestionPage(
+    final int? questionsAnswered = await Navigator.push<int>(


For the record, don't change the naming of the variables unless the change is very relevant.
It was probably not the case here, and the reviewer still has to check every change...

monsieurtanuki · 2025-04-18T06:29:32Z

packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart

@@ -308,7 +308,13 @@ class UserPreferencesContribute extends AbstractUserPreferences {
    AnalyticsHelper.trackEvent(
      AnalyticsEvent.hungerGameOpened,
    );
-    await openQuestionPage(context);
+
+    Navigator.push(


Please use the simpler await syntax.

monsieurtanuki · 2025-04-18T06:30:06Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

-      ],
+    return Expanded(
+      child: Column(
+        mainAxisSize: MainAxisSize.max, // <--- CAMBIATO


Per favore...

deleted, sorry

monsieurtanuki · 2025-04-18T06:37:31Z

packages/smooth_app/lib/pages/hunger_games/question_image_thumbnail.dart

+                  BuildContext context,
+                  Widget child,
+                  ImageChunkEvent? loadingProgress,
+                ) {
+                  if (loadingProgress == null) {
+                    // TODO(monsieurtanuki): remove this when the image is not null anymore
+                    return child;
+                  }
+                  return const Center(
+                      child: CircularProgressIndicator.adaptive());
+                },
+              ),
+            );
+          },


Honestly, what's the point of changing the parameter names and the coding style, except annoying the reviewer?
You literally changed nothing here, except adding an unexpected TODO with my name...
Please revert that part.

monsieurtanuki · 2025-04-18T06:37:50Z

packages/smooth_app/lib/pages/hunger_games/question_image_thumbnail.dart

+  Widget build(BuildContext context) {
+    return Container(


What's the point of changing the coding style? Please revert that part.

monsieurtanuki · 2025-04-18T06:38:42Z

packages/smooth_app/lib/pages/hunger_games/question_image_thumbnail.dart

+              width: constraints.maxWidth,
+              height: constraints.maxHeight,


Would you please try with double.infinity for width and height?

monsieurtanuki · 2025-04-18T06:39:37Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

-  const _QuestionPage({
+class QuestionsPage extends StatefulWidget {
+  const QuestionsPage({
+    super.key,


We don't need key, do we?

No, we don't. Deleted.

monsieurtanuki · 2025-04-18T06:40:15Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

-    );
-    _currentQuestionIndex = 0;
+    try {
+      setState(() {


monsieurtanuki · 2025-04-18T06:40:57Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

+      setState(() {
+        _state = const _RobotoffQuestionLoadingState();
+      });
+      _loadQuestions(


Please add unawaited(.

monsieurtanuki · 2025-04-18T06:50:16Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

@@ -367,7 +274,7 @@ class _LoadingQuestionsView extends StatelessWidget {
          child: Center(
            child: DefaultTextStyle(
              textAlign: TextAlign.center,
-              style: const TextStyle(),
+              style: TextStyle(color: colorScheme.onSurface),


Please

make a screenshot

remove all your color and style changes, and then make a screenshot

You are right, the screenshots are exactly the same.

LorenzoMascia · 2025-04-21T18:51:42Z

The final result using BoxFit.contain instead of cover for the ìmage:

…verlay Committer: Lorenzo Mascia <[email protected]> Changes : modified: packages/smooth_app/lib/pages/hunger_games/question_card.dart modified: packages/smooth_app/lib/pages/hunger_games/question_page.dart modified: packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart modified: packages/smooth_app/lib/pages/product/product_questions_widget.dart

Committer: Lorenzo Mascia <[email protected]> Changes: modified: packages/smooth_app/lib/pages/hunger_games/question_card.dart modified: packages/smooth_app/lib/pages/hunger_games/question_image_thumbnail.dart modified: packages/smooth_app/lib/pages/hunger_games/question_page.dart modified: packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart modified: packages/smooth_app/lib/pages/product/product_questions_widget.dart

Committer: Lorenzo Mascia <[email protected]> Changes: modified: packages/smooth_app/lib/pages/hunger_games/question_image_thumbnail.dart modified: packages/smooth_app/lib/pages/hunger_games/question_page.dart modified: packages/smooth_app/lib/pages/preferences/user_preferences_contribute.dart modified: packages/smooth_app/lib/pages/product/product_questions_widget.dart

monsieurtanuki

Good job @LorenzoMascia!

If you have 10 minutes, please have a look at my comments. If you already know that you won't have time for that in a near future, please tell me.

I'm still surprised that you had to change the styles and colors, but that's another story.

monsieurtanuki · 2025-04-22T09:38:40Z

packages/smooth_app/lib/pages/hunger_games/question_image_thumbnail.dart

-                progress == null
-                    ? child
-                    : const CircularProgressIndicator.adaptive(),
+          child: LayoutBuilder(


Then we don't need a LayoutBuilder anymore, do we?

No, it's no longer needed. I had used the builder to get the BoxConstraints during some trials, but in the end I didn’t use them.

packages/smooth_app/lib/pages/hunger_games/question_card.dart

monsieurtanuki · 2025-04-22T09:53:44Z

packages/smooth_app/lib/pages/hunger_games/question_page.dart

-          onAnswer: onAnswer,
-        ),
-      ],
+    return Expanded(


Not sure if that Expanded is relevant.

No, it works in the same way returning directly the Column without the Expanded

monsieurtanuki · 2025-04-22T10:09:07Z

@LorenzoMascia And congratulations for your first PR here!

Co-authored-by: monsieurtanuki <[email protected]>

Committer: Lorenzo Mascia <[email protected]> Changes: modified: lib/pages/hunger_games/question_image_thumbnail.dart modified: lib/pages/hunger_games/question_page.dart

monsieurtanuki · 2025-04-25T07:43:57Z

Hi @LorenzoMascia!
Before I merge your code, would you please attach a screenshot?
I'm a bit worried about your choice of adding extra styles and extra colors. Thank you!

github-project-automation bot added this to 🤳🥫 The Open Food Facts mobile app (Android & iOS) Apr 16, 2025

github-project-automation bot moved this to 💬 To discuss and validate in 🤳🥫 The Open Food Facts mobile app (Android & iOS) Apr 16, 2025

github-actions bot assigned LorenzoMascia Apr 16, 2025

github-actions bot added 🥫 Product page 🤖 Robotoff Hunger Games labels Apr 16, 2025

github-project-automation bot added this to 🤖 Artificial Intelligence @ Open Food Facts Apr 16, 2025

github-project-automation bot moved this to To triage in 🤖 Artificial Intelligence @ Open Food Facts Apr 16, 2025

LorenzoMascia changed the title ~~Fix #4681: Move Hunger Games to a real view instead of overlay~~ fix: Move Hunger Games to a real view instead of overlay (#4681) Apr 16, 2025

teolemon requested review from g123k and monsieurtanuki April 16, 2025 10:21

monsieurtanuki reviewed Apr 16, 2025

View reviewed changes

monsieurtanuki reviewed Apr 18, 2025

View reviewed changes

Lorenzo Mascia added 3 commits April 22, 2025 09:23

LorenzoMascia force-pushed the fix/hunger-games-view branch from 81ca678 to 3992c5c Compare April 22, 2025 07:23

monsieurtanuki approved these changes Apr 22, 2025

View reviewed changes

github-project-automation bot moved this from 💬 To discuss and validate to 🎊 Done in 🤳🥫 The Open Food Facts mobile app (Android & iOS) Apr 22, 2025

github-project-automation bot moved this from To triage to In Progress in 🤖 Artificial Intelligence @ Open Food Facts Apr 22, 2025

LorenzoMascia and others added 3 commits April 22, 2025 19:39

Update packages/smooth_app/lib/pages/hunger_games/question_card.dart

f853934

Co-authored-by: monsieurtanuki <[email protected]>

Merge branch 'develop' into fix/hunger-games-view

ae2d011

Fixes after third review

53dc1de

Committer: Lorenzo Mascia <[email protected]> Changes: modified: lib/pages/hunger_games/question_image_thumbnail.dart modified: lib/pages/hunger_games/question_page.dart

monsieurtanuki marked this pull request as draft April 25, 2025 12:22

monsieurtanuki mentioned this pull request Apr 25, 2025

feat: Nutrition facts: check values before sending them #4446 #6562

Open

fix: Move Hunger Games to a real view instead of overlay (#4681) #6555

Are you sure you want to change the base?

fix: Move Hunger Games to a real view instead of overlay (#4681) #6555

Conversation

LorenzoMascia commented Apr 16, 2025

What

Screenshot

Fixes bug(s)

monsieurtanuki commented Apr 16, 2025

monsieurtanuki left a 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

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

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

codecov-commenter commented Apr 16, 2025 • edited Loading

Codecov Report

monsieurtanuki left a 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

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

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

LorenzoMascia commented Apr 21, 2025

monsieurtanuki left a 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

monsieurtanuki commented Apr 22, 2025

monsieurtanuki commented Apr 25, 2025

codecov-commenter commented Apr 16, 2025 •

edited

Loading