feat: Finish the new product page revamp based on the Attributes API - Implementation of product attributes - Pr/#707

[jncosideout]

PR Description

This PR was branched from PR #748 which at this time is still WIP and not merged. But it is complete enough to use for making a mock-up for Issue #707 which aims to create new rows on the prodoct summary page to display the Product Attributes, and show summary info cards from the data supplied by the Product Attributes API

Type of Changes

• Fixes Issue [Proposal] New labels experience (from links to images) #707
• New feature

Proposed changes

• Copy the implementation of IngredientAnalysisTableViewCell and NutritionLevelTableViewCell
• Create 'AttributeView' .swift and .xib
• Create 'AttributeTableViewCell' .swift and .xib
• Integrate with ProductDetailViewController

Screenshots

Before

After

Checklist

Make sure you've done all the following (Put an x in the boxes that apply.)

• If you have multiple commits please combine them into one commit by squashing them.
• Code is well documented
• Included unit tests for new functionality
• All user-visible strings are made translatable
• Code passes Travis builds in your branch

Part of

• What can I work on ? (Tracker issue) #912

[aleene]

Maybe remove the visual impact from these changes from the other pull request. And finish/merge it. The visual/usage part can then be done here.

[jncosideout]

@aleene You are right, that is the best course of action.

[jncosideout]

Got my custom AttributeTableViewCell to work. To test it I am only displaying the first attribute from the first group of this product "Crème entière fluide Bio (30 % MG)"
which is

"Nutri-Score" :: "Poor nutritional quality"

It works in multi-language because the Product Attributes are requested in the users language set in the app's preferences.
As you can see, I haven't figured out how to fetch the picture, even though there is a URL for an image resource that is returned in the JSON response, but at least I have the imageView ready for it.

• here it is in English
  

• here it is in Francais
  

[jncosideout]

No major changes, got a couple of the Product Attribute icons to display, but only the icons of the Labels, which were .png, and none of the others because they were .svg (like Nutrition Level icons or Allergen icons)

Here is the data I tested with

[teolemon]

@stephanegigandet

[teolemon]

Starting to look very good.
Looking at https://stackoverflow.com/questions/35691839/how-to-display-svg-image-using-swift. Not quite sure of the best solution is. Any opinions ?
We could potentially offer PNG fallback on the server, alternatively.

[jncosideout]

I saw the thread on Slack about the PNG vs SVG difficulties in Swift. I agree, working with PNGs is much easier.

Here are the latest results from my progress today, and here is the data I tested with.

---

It does not yet look like your mock-up on #707 but I will make the necessary adjustments. I just ripped the code from IngredientAnalysisView.swift to get the same functionality, so the formatting of the pop-up is the same right now.

Some other little things I need to finish:

1. The text on the AttributeTableViewCells is getting cut off because the height of the UILabels is too short, or the font too big
2. I was thinking of creating those little triangle arrows pointing right like you had in your mock-ups to signify that the cell is "clickable."
3. The BLTN pop-up page is not yet paginated, which is part of the spec you laid out earlier, so I need to add functionality to swipe left on the pop-up to go to a new page with the rest of the info, correct? What other info goes on the other pages? "Description long" is already the first item.

[jncosideout]

Now the descriptionShort will wrap and show all text in the AttributeTableViewcell. 😄
The icons for all attributes returned in the JSON from the server have recently been changed from PNG to SVG, so I can't display them anymore. So in the screenshots below you will see I am using a "Nutri-Score-B" image as a placeholder, since that is the largest width icon that an Attribute could have, or no image at all.

@teolemon @stephanegigandet will the server go back to PNGS for the product attributes icons or will I need to work with the SVGs?

I have added the AttributeView to the BLTN pop-up card to make it look more like the mock-up in #707

---

I'm having a problem displaying long Attribute names, and I think the way to solve that is to include the name in the 'Description Short' UITextView in AttibuteView. Then I can use an attributed string to format them and make them look separate, like in the InfoRowTableViewCell which is used to display the "Taxonomy" FormRows.

---

The 'long name' problem also causes the name to get cut off and partially hidden under the 'X' icon in the top-right of the BLTN pop-up.

---

[teolemon]

I've merged the underlying code (previous PR into develop) @jncosideout @aleene

[jncosideout]

I've merged the underlying code

@teolemon Excellent. I will squash the commits in this branch and merge in 'develop'

As a quick update:
I spent a few days trying to add a little 'handle' to the top of the BLTN page pop-up to indicate that you can 'pull' the page down to dismiss it. This is how it looks in your mock-up on #707 but I just couldn't figure it out. So for now I am using the default BLTN layout and here is how it looks so far:

---

and here is a mock-up of the little grey-handle I was talking about. This is what I was going for:

but I gave up on that. I tried adding a PNG with a UIImageView and also tried using code to draw the line.

I could go back to the way it was with the grey circle with the X in the center, but that was sometimes overlapping long labels.

[jncosideout]

I spent a few days trying to add a little 'handle' to the top of the BLTN page pop-up to indicate that you can 'pull' the page down to dismiss it.
...but I gave up on that.

then I tried again and success!
I needed to give my views height and width to be displayed, since views with h=0 w=0 have no area will not be displayed, of course.
Here you can see that I colored the backgrounds of my views so I could visually debug my constraints:

---

And I'm still having trouble displaying the full text in the text view of the AttributeView, so the descriptionShort is getting cut off when in landscape mode. So I will fix that next, then go on to make the BLTN item push or slide the next page on top.

[teolemon]

@jncosideout We use Floating Panel somewhere else in the app: https://github.com/SCENEE/FloatingPanel

[jncosideout]

We use Floating Panel

Oh! I should have known not to reinvent the wheel. Thank you, @teolemon
In my defense, I was trying to leverage existing code in used in IngredientAnalysisView.swift since that has similar functionality, but I should have asked for help before wasting too much time. At least I learned somethings in the process.

[jncosideout]

Status update: I'm still working on changing my design from using the BLTNPageItem to the FloatingPanelViewController. I've managed to set up the FloatingPanelVC by extending ProductDetailVC, but I'm having trouble displaying my content on the FloatingPanel view. See my latest commit for more detail.

[jncosideout]

Status update: I'm having trouble displaying my content on the FloatingPanel view

I got it to display now:

the red and green are for my visual debugging purposes

I don't think we want it to display in 'full' covering the entire screen, so I will change that soon to 'half' once I iron out the layout discrepancies, such as adding in the description_long

Also, I need to run some performance tests and use xcode Instruments' Allocations tool to make sure I haven't introduced memory leaks.

[teolemon]

If the left padding is for the image, possibly this to flow around the image and loose less space: https://stackoverflow.com/questions/44332928/xcode-swift-how-to-wrap-text-around-image

[philippeauriach]

Hi @jncosideout , thank you for your work!
I tried to go through the PR but there is a lot of code :) At first sight, here are my observations:

• I have the impression that you are looking to mix floatingpanel and bltboard and add custom for the gestures, which I would not recommend: this adds complexity and maintenance for features that are implemented (or something like this) in the libs we use

• Also, it seems that you try to stick 100% to mockups, which are mockups not precise designs (and were made based on Android screenshots), I think you should go more in the direction of "doing something that is inspired by mockups but which sticks to the current iOS design, ie which generally follows the main features of libs like BLTBoard or FloatingPanel ". This too is to avoid having too much maintenance on small specific graphic details (like the above ribbon).

Happy to help

[jncosideout]

@teolemon @aleene @philippeauriach
I am back to pick up right where I left off on PR780, the UI for Product Attributes. Is this PR still open or have you decided not to use this feature anymore? :(
I have made significant progress:

1. fixed wrapping text around the Attribute’s icon image
2. allowed SVG images to be displayed

I still want to add

1. caching images after fetching from the server (I can’t use KingFisher because I’m requesting the images directly through a UIWebView)
2. A loading spinner to indicate loading of the image

Here is the result of my latest work (remember that I'm using coloring to debug the ui):

[teolemon]

The feature is still the #1 priority @jncosideout 👍

[jncosideout]

@teolemon @aleene @philippeauriach
I am finished with this PR. To summarize this PR accomplishes:

1. Displaying product attributes as table cells in ProductDetailVC (hardcoded to show just Attributes in the "labels" AttributeGroup, including 'Organic' and 'Fair Trade.')
2. Each table cell contains an AttributeView, which has the Attribute's icon on the left, name and description_short on the right.
3. Tapping on the cell displays a FloatingPanel which shows the same AttributeView, with description_long beneath it.

When you all are satisfied with my work, I will squash my commits and push them one more time and then you can merge.

[jncosideout]

I found and fixed a leak when viewing products from history.

SummaryFormTableVC was causing ProductDetailVC and its VCs to be leaked, creating unbounded memory growth.

IngredientsAnalysisTableViewCell had a strong reference to SummaryForm

so I just made it weak, and that fixed it. You can see that the memory footprint stays constant at the baseline of 33.5mb, rising at a level that is almost unnoticeable, and with no OpenFoodFacts objects being leaked in between products viewed.
Before it reached 51mb after only two products viewed and would have kept climbing.

@teolemon

[aleene]

Great find. Could this be happening elsewhere as well?

(Can not test it, as my old mac no longer supports the latest xcode)

[jncosideout]

@aleene @teolemon @philippeauriach

Great find. Could this be happening elsewhere as well?

Yes, that is likely. I only tested viewing products in my history, not scanning new ones. But I noticed that four major view controllers are leaked within the first ten seconds of launching. They are each duplicated once, but thereafter no more copies were made when I tested.

(Can not test it, as my old mac no longer supports the latest xcode)

I'm not running the latest xcode. I'm running 11.7, because as you and others well know, Carthage cannot build certain modules with the new version of swift in xcode 12.x. I even tried using XCFrameworks. So I fell back to 11.7 and that is also the version of Instruments that I ran my Allocations profiling tests on. So you could recreate it or, I could send you my .trace file of my tests.

[jncosideout]

@aleene @teolemon @philippeauriach
I noticed that four major view controllers are leaked within the first ten seconds of launching.

I think I figured out the source of the duplication of ScannerVC, SettingsTableVC, HistoryTVC, & SearchVC. They are all in Main.storyboard and I believe they are instantiated automatically when the app launches. Next, they are instantiated explicitly in code when RootViewController.init() is called in app delegate.

So nothing to see here, these VC's aren't continuously leaking and they don't take up a large amount of memory anyway. However, as Arnaud mentioned, we should profile the app to try to find other memory leaks, and that should be a separate Issue & PR.