feat: Add all scanned products to list #3401

M123-dev · 2022-12-02T16:42:09Z

What

Now you can add all scanned products to a List

For: Add all scanned products to a list in a couple clicks #3371
+ Changed an Ink to a Container for the problem mentioned in Discussion: The delete gestures are not equal #2985, the content moving out of the card. Not the card moving

codecov-commenter · 2022-12-02T16:48:38Z

Codecov Report

Merging #3401 (ff21ad1) into develop (4a3b1a9) will increase coverage by 0.94%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop    #3401      +/-   ##
===========================================
+ Coverage    10.32%   11.27%   +0.94%     
===========================================
  Files          260      260              
  Lines        12595    12575      -20     
===========================================
+ Hits          1301     1418     +117     
+ Misses       11294    11157     -137

Impacted Files	Coverage Δ
...cards/product_cards/smooth_product_card_found.dart	`0.00% <0.00%> (ø)`
...ages/smooth_app/lib/database/dao_product_list.dart	`0.00% <0.00%> (ø)`
...mooth_app/lib/pages/personalized_ranking_page.dart	`0.00% <0.00%> (ø)`
...smooth_app/lib/pages/product/new_product_page.dart	`0.00% <0.00%> (ø)`
...app/lib/pages/product_list_user_dialog_helper.dart	`0.00% <0.00%> (-0.56%)`	⬇️
packages/smooth_app/lib/pages/image_crop_page.dart	`0.00% <0.00%> (-1.36%)`	⬇️
...ckages/smooth_app/lib/tmp_crop_image/rotation.dart	`2.50% <0.00%> (-0.36%)`	⬇️
...kages/smooth_app/lib/helpers/analytics_helper.dart	`5.00% <0.00%> (-0.13%)`	⬇️
packages/smooth_app/lib/main.dart	`14.40% <0.00%> (-0.13%)`	⬇️
...mooth_app/lib/widgets/smooth_product_carousel.dart	`2.45% <0.00%> (-0.07%)`	⬇️
... and 21 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

monsieurtanuki

Looks OK @M123-dev, but there is a couple of remarks that are rather important:

you should await when you update with async
it would be great to code a distinct dialog method that lets the user select user lists (and create new ones)

Feel free to dismiss the other comments.

packages/smooth_app/lib/pages/product_list_user_dialog_helper.dart

packages/smooth_app/lib/pages/personalized_ranking_page.dart

packages/smooth_app/lib/database/dao_product_list.dart

packages/smooth_app/lib/pages/personalized_ranking_page.dart

M123-dev · 2022-12-06T20:30:04Z

I did some refactoring @monsieurtanuki hopefully all your suggestions should be adressed with this

monsieurtanuki

Hi @M123-dev!
I have the funny impression that you did refactor, but not in the direction I suggested - I had in mind an agnostic selectedLists selectUserLists(allLists, initiallySelectedLists) dialog method.
That doesn't help my review, especially as these days I have limited time to dedicate to the project.
Feel free to ignore my comments and to merge anyway, as long as the code works as intended.

packages/smooth_app/lib/pages/product_list_user_dialog_helper.dart

monsieurtanuki · 2022-12-07T12:49:15Z

packages/smooth_app/lib/pages/product_list_user_dialog_helper.dart

-                formKey,
-                textEditingController,
-              ),
+              onFieldSubmitted: (_) {


I cannot see the added value of removing method _onNewListValueSubmitted as it's called twice,

packages/smooth_app/lib/pages/product_list_user_dialog_helper.dart

monsieurtanuki

Hi @M123-dev!
Again, I cannot say that there's something obviously wrong in your code, it's just that I imagined a different way to code it - and there are tons of ways to code it anyway.
Feel free to merge any time.

M123-dev · 2022-12-16T15:17:43Z

Sorry for my absence on this PR, had a lot of exams over the last few weeks 😄

Now everything should be done, the lists are preselected if all barcodes to add are included and the dialog is a little more abstract so that it can be reused more often.

Feel free to have a look at the changes or just merge @monsieurtanuki 😃

monsieurtanuki

Hi @M123-dev!
Obviously you don't like my concept of "method that returns the product lists selected by the user". Fair enough.
Please have a look at my comments: I'm afraid there are some await missing.

packages/smooth_app/lib/pages/product_list_user_dialog_helper.dart

packages/smooth_app/lib/database/dao_product_list.dart

packages/smooth_app/lib/pages/product_list_user_dialog_helper.dart

monsieurtanuki · 2022-12-16T15:28:30Z

packages/smooth_app/lib/pages/product_list_user_dialog_helper.dart

-                .bulkInsert(ProductList.user(name), widget.barcodes.toList());
-          }
+      builder: (BuildContext context) => _UserLists(
+        lists: lists.toSet(),


You may loose the order, mayn't you? (for the record that's my first "mayn't you?")

https://api.dart.dev/stable/2.18.1/dart-core/Set/toSet.html

Creates a Set with the same elements and behavior as this Set.

The returned set behaves the same as this set with regard to adding and removing elements. It initially contains the same elements. If this set specifies an ordering of the elements, the returned set will have the same order.

Just double checked your may :D
Looks like the order is kept and this wouldn't be a problem would it? The lists in the list screen are also ordered alphabetically

If this set specifies an ordering of the elements

I don't know what that means. We just add items. The set itself does not specify anything, does it?
I don't like this dart confusion with tacitly "ordered" Sets.

and this wouldn't be a problem

That would be a minor problem, I'll give you that.

monsieurtanuki · 2022-12-16T16:46:23Z

Sorry for my absence on this PR, had a lot of exams over the last few weeks 😄

I hope you passed ;)

M123-dev · 2022-12-16T18:01:56Z

Added the awaits + renamed the one list and no I don't have something against a method which returns the selected lists I only think that it's simpler to understand this way. Returning the lists would mean we have a method which returns the return of a showAlert which would (preferably) show a StatelessWidget which would need to pop them. Doable but not as easy to read.

And thanks 😄

monsieurtanuki · 2022-12-16T18:20:51Z

Added the awaits + renamed the one list and no I don't have something against a method which returns the selected lists I only think that it's simpler to understand this way.

Then we disagree on that matter. Not a big deal though.

Returning the lists would mean we have a method which returns the return of a showAlert which would (preferably) show a StatelessWidget which would need to pop them. Doable but not as easy to read.

As far as I remember you cannot display a StatelessWidget, you need to refresh the checkboxes and handle the "create list" feature.

And thanks 😄

You're welcome :)

Please get rid of the first LoadingDialog.run(future: daoProductList.getUserLists(), ...), we don't need it because we call it afterwards with more parameters, and we don't need the LoadingDialog either.

And then we're probably done.

M123-dev · 2022-12-21T11:31:42Z

I removed the loading dialog but we still need the async call for all lists, later we only get the lists which should be selected. daoProductList.getUserLists(withBarcodes: barcodes);

monsieurtanuki

Hi @M123-dev!
Looks ok to me - you're right, you have to call twice the "getUserLists" method, once for all lists and once for the lists that contain the barcodes.

monsieurtanuki · 2022-12-16T17:59:44Z

packages/smooth_app/lib/database/dao_product_list.dart

@@ -224,31 +224,31 @@ class DaoProductList extends AbstractDao {
  /// Adds or removes list of barcodes to/from a [productList] in one go (depending on [include])
  Future<void> bulkSet(
    final ProductList productList,
-    final List<String> barcodesToAdd, {
+    List<String> barcodes, {


Suggested change

List<String> barcodes, {

final List<String> barcodes, {

monsieurtanuki · 2022-12-16T18:02:44Z

packages/smooth_app/lib/pages/product_list_user_dialog_helper.dart

-          ),
-        ) ??
-        <String>[];
+    final List<String> selectedLists = await daoProductList.getUserLists(


We would be better off calling it in the first place with the parameters, wouldn't we?
cf. just above:

final List<String>? lists = await LoadingDialog.run<List<String>>( context: context, future: daoProductList.getUserLists(), );

I can't follow what you mean by this,

having both calls (with and without barcode filter) directly next to eachother?

I assumed it was possible to get both data (all lists and all lists with barcodes) with one call, but that's not the case.
You're right: we do need two calls.

monsieurtanuki · 2022-12-16T18:08:30Z

packages/smooth_app/lib/pages/product_list_user_dialog_helper.dart

-                .bulkInsert(ProductList.user(name), widget.barcodes.toList());
-          }
+      builder: (BuildContext context) => _UserLists(
+        lists: lists.toSet(),


If this set specifies an ordering of the elements

I don't know what that means. We just add items. The set itself does not specify anything, does it?
I don't like this dart confusion with tacitly "ordered" Sets.

and this wouldn't be a problem

That would be a minor problem, I'll give you that.

M123-dev · 2022-12-21T13:40:30Z

I don't know what that means. We just add items. The set itself does not specify anything, does it?
I don't like this dart confusion with tacitly "ordered" Sets.

I agree with you it's not written very explicitly, my guess is that it just keeps the order of the list. I started switching to sets here so that we avoid dublicates in lists from the ground up.

monsieurtanuki · 2022-12-21T17:51:47Z

I agree with you it's not written very explicitly, my guess is that it just keeps the order of the list. I started switching to sets here so that we avoid dublicates in lists from the ground up.

I think it's a false good idea. As long as we need sorted collections and that we do handle the duplicates, switching to Set instead of List adds more trouble than it solves.

@M123-dev Anyway, you started this PR 3 weeks ago, the conversation level is 51, I approved 3 times including the latest version 6 hours ago, so without further ado please merge unless something really problematic is still there.

M123-dev · 2022-12-22T11:46:38Z

Thanks for the review @monsieurtanuki

feat: Add all scanned products to list

4442d40

M123-dev requested a review from monsieurtanuki December 2, 2022 16:42

M123-dev requested a review from a team as a code owner December 2, 2022 16:42

github-actions bot added the database label Dec 2, 2022

github-actions bot assigned M123-dev Dec 2, 2022

github-actions bot added the User lists label Dec 2, 2022

monsieurtanuki reviewed Dec 2, 2022

View reviewed changes

Code review

76c1a40

github-actions bot added the 🌐 l10n label Dec 4, 2022

monsieurtanuki reviewed Dec 4, 2022

View reviewed changes

packages/smooth_app/lib/pages/personalized_ranking_page.dart Outdated Show resolved Hide resolved

M123-dev added 2 commits December 6, 2022 21:23

refactoring from product_list_user_dialog_helper

8fff003

Update all_user_product_list_page.dart

b4416d6

github-actions bot added Product addition The easier it is to add a product and get Nutri-Score, Eco-Score, the happier the users. 🥫 Product page labels Dec 6, 2022

M123-dev requested a review from monsieurtanuki December 6, 2022 20:29

monsieurtanuki approved these changes Dec 7, 2022

View reviewed changes

Correct use of pop'ed bools

4152912

monsieurtanuki approved these changes Dec 9, 2022

View reviewed changes

Now showing if all products are included

930da6b

monsieurtanuki reviewed Dec 16, 2022

View reviewed changes

Await + rename

ff21ad1

Remove loading dialog

c4ff65d

Update product_list_user_dialog_helper.dart

d89a352

monsieurtanuki approved these changes Dec 21, 2022

View reviewed changes

Update dao_product_list.dart

416a8a8

M123-dev merged commit 1cce8cc into develop Dec 22, 2022

M123-dev deleted the feat-Add-als-scanned-products-to-list branch December 22, 2022 11:46

github-actions bot mentioned this pull request Dec 22, 2022

chore(develop): release 4.2.0 #3422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add all scanned products to list #3401

feat: Add all scanned products to list #3401

M123-dev commented Dec 2, 2022 •

edited by teolemon

codecov-commenter commented Dec 2, 2022 •

edited

monsieurtanuki left a comment

M123-dev commented Dec 6, 2022

monsieurtanuki left a comment

monsieurtanuki Dec 7, 2022

monsieurtanuki left a comment

M123-dev commented Dec 16, 2022 •

edited

monsieurtanuki left a comment

monsieurtanuki Dec 16, 2022

M123-dev Dec 16, 2022

monsieurtanuki Dec 16, 2022

monsieurtanuki commented Dec 16, 2022

M123-dev commented Dec 16, 2022

monsieurtanuki commented Dec 16, 2022

M123-dev commented Dec 21, 2022

monsieurtanuki left a comment

monsieurtanuki Dec 16, 2022

monsieurtanuki Dec 16, 2022

M123-dev Dec 21, 2022

monsieurtanuki Dec 21, 2022

monsieurtanuki Dec 16, 2022

M123-dev commented Dec 21, 2022

monsieurtanuki commented Dec 21, 2022

M123-dev commented Dec 22, 2022

feat: Add all scanned products to list #3401

feat: Add all scanned products to list #3401

Conversation

M123-dev commented Dec 2, 2022 • edited by teolemon

What

codecov-commenter commented Dec 2, 2022 • edited

Codecov Report

monsieurtanuki left a comment

Choose a reason for hiding this comment

M123-dev commented Dec 6, 2022

monsieurtanuki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monsieurtanuki left a comment

Choose a reason for hiding this comment

M123-dev commented Dec 16, 2022 • edited

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

monsieurtanuki commented Dec 16, 2022

M123-dev commented Dec 16, 2022

monsieurtanuki commented Dec 16, 2022

M123-dev commented Dec 21, 2022

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

M123-dev commented Dec 21, 2022

monsieurtanuki commented Dec 21, 2022

M123-dev commented Dec 22, 2022

M123-dev commented Dec 2, 2022 •

edited by teolemon

codecov-commenter commented Dec 2, 2022 •

edited

M123-dev commented Dec 16, 2022 •

edited