Automattic / stories-android

Loop concept app - WP Stories library
GNU General Public License v2.0
20 stars 6 forks source link

Inflate added views when saving if these are not populated yet #663

Closed mzorz closed 3 years ago

mzorz commented 3 years ago

Fix #581

This issue was a tricky to reproduce, but once I wrapped my head around it, I realized it was easily reproducible. Finding the root cause was a bit hard since I was too focused on the "reordering" frames part as being central to the issue.

After discarding a few theories (problems when adding views? problems with saving? problem with serialization/deserialization) and much debugging, I realized at this part of code:

        withContext(Dispatchers.Main) {
            // now call addViewToParent the addedViews remembered by this frame
            for (oneView in frame.addedViews) {
                oneView.view?.let {
                    removeViewFromParent(it)
                    // this is needed, otherwise some vector graphics such as emoji in text will not render
                    // correctly when a hardware display is not in place (such is the case of FrameSaveManager,
                    // as we're laying out the views on an off-screen view).
                    it.setLayerType(View.LAYER_TYPE_SOFTWARE, null)
                    ghostPhotoEditorView.addView(it, getViewLayoutParams())
                }
            }
        }

we were getting null oneView.views, which gave me the hint that we weren't inflating the added views until these were actually needed (by tapping on a frame on the frame selector). Hence, the only frame that would actually have its added views was the first one, always. In the case you'd rearrange the first slide and move it elsewhere, it would still be the only one selected, but the rest of the frames would have never had their added views inflated and properly shown on the screen.

So with that root cause figured out, I wrote this solution that takes care of inflating added views for a given frame when it's being prepared to be saved, in the case any such added views is null.

Few things to have in mind:

The following videos show a Story that has been edited and only its slides have been rearranged. All slides have 1 added view of type text. First video shows the defect on develop, second video shows how things are after performing the test steps on this branch.

Before

https://user-images.githubusercontent.com/6597771/115985196-15cc3200-a581-11eb-8725-2ceff3eff2a0.mov

After

https://user-images.githubusercontent.com/6597771/115985192-14026e80-a581-11eb-97e9-647f7b216117.mov

To test:

  1. create story with 2 slides, add one text on them
  2. save
  3. go to post, edit post
  4. tap block to edit
  5. rearrange story: tap and hold the first slide and move it to the right (beware to NEVER TAP on the slide that now is at the first position! this would otherwise refresh the slide and make the text visible)
  6. save (verify at this point the block in Gutenberg, which shows the first slide by default, which was the last slide before step 5, does have an added view)
  7. tap UPDATE
  8. verify the server version reflects the changes and has all the added views
peril-automattic[bot] commented 3 years ago

You can test the changes on this Pull Request by downloading the APK here.

aforcier commented 3 years ago

Working well @mzorz, nice sleuthing as usual ✨ Reproduced the issue on current develop per your steps, and confirmed it's resolved with your change. :shipit: