Queue Events for better media uploads

[chipsnyder]

Fixes (the iOS part of): #2325
WordPress-iOS wordpress-mobile/WordPress-iOS#14311

To test:

Follow the testing steps in #2041 and #1928

PR submission checklist:

• I have considered adding unit tests where possible.
• I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

[peril-wordpress-mobile]

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

[etoledom]

Thank you for this improvement @chipsnyder !

Tested on the WPiOS build and it works great 🎉

I left a couple of comments on code. It'd be great to fix the warning before merging. The rest at your discretion 👍

[etoledom]

We can use .RCTContentDidAppear instead of the String.

Suggested change

	NotificationCenter.default.addObserver(forName: Notification.Name("RCTContentDidAppearNotification"), object: nil, queue: nil) { (_) in
	NotificationCenter.default.addObserver(forName: .RCTContentDidAppear, object: nil, queue: nil) { (_) in

[chipsnyder]

Thanks @etoledom, I was struggling to find this!

[mkevins]

Also tested, and really tried to force a race condition (tried multiple times via image and gallery block, images with various filesizes, and reopening the editor with and without metro). Everything is working as expected 🎉 . Nice work Chip!

[chipsnyder]

Thanks for the review @mkevins!

@etoledom Will you take another pass and see if I hit all of your concerns? I think by "It'd be great to fix the warning before merging." you were referring to #2366 (comment) but let me know if I missed a different warning :)

[etoledom]

Sorry! I meant the warning on this line. I wrote the comment but it disappeared somehow (second time that happens to me this week ¯\_(ツ)_/¯ )

The warning says Coercion of implicitly unwrappable value of type 'Any?' to 'Any' does not unwrap optional

Would the override still work if we change the optionality of the parameters?
Actually on a second look, that Any! looks quite dangerous. This might crash if any queued event had no body.

It would be great if name were not optional and body is an actual non-implicitly-unwrapped optional, but open to possibilities too.

[etoledom]

I just did a test and it doesn't crash. Any seems to accept a nil value even if it's not an optional itself (?)

But still... I wouldn't trust the runtime completely with this 😬

[mkevins]

I just did a test and it doesn't crash. Any seems to accept a nil value even if it's not an optional itself (?)

Interesting! I wouldn't have expected that. Does that mean the struct's default initializer is not actually force-unwrapping the value? When you tested with a nil, do you know if the GutenbergEvent was initialized, or did the code branch directly to the super.sendEvent?

Update:

In a playground, I just tried this:

struct SomeData {
    let thing1: String
    let thing2: Any
}

func test(t1: String, t2: Any!) {
    var nilThing: Any

    let data = SomeData(thing1: t1, thing2: t2)
    print(data.thing1)

    // no errors
    nilThing = t2
    nilThing = data.thing2
}

test(t1: "Hello world!", t2: nil)

And there are no errors in runtime. Interestingly, when I use AnyObject, I get a run-time crash (Terminated by signal 4), just initializing the struct:

struct SomeData {
    let thing1: String
    let thing2: AnyObject
}

func test(t1: String, t2: AnyObject!) {
    let data = SomeData(thing1: t1, thing2: t2)
}

test(t1: "Hello world!", t2: nil)

Perhaps there is some subtlety with permissiveness afforded for potential value or reference types? 🤔

[etoledom]

Perhaps there is some subtlety with permissiveness afforded for potential value or reference types? 🤔

This kinds of make sense in a way that Optional is basically a Value type, and we are passing Optional<>.none as Any.

And AnyObject wouldn't accept it since it expects Reference types only, so it will try to unwrap the optional.

[mkevins]

Thanks for the explanation Eduardo! I also found this explanation, which is just as you mentioned. The discussion is interesting because it explains why you cannot directly assign nil to a var with type Any, since there is no inferred type for the generic in the Optional. That there is no error in the above case was surprising to me, but this explains it (kind of a language design edge case, I suppose).

[etoledom]

So @mkevins @chipsnyder , what do you think: Should we avoid this edge case?

I think it would be good to try avoid it, but since it's clear now that it won't crash, maybe it's not a blocker for this PR. Still it would be good to clear the warning 😬

[chipsnyder]

comment but it disappeared somehow

Oh, that is weird! Thanks for confirming though.

The warning says Coercion of implicitly unwrappable value of type 'Any?' to 'Any' does not unwrap optional

I totally missed this error thank you for catching it. Also thank you both for the investigation and knowledge share. That's really interesting on how Any and Optionals interact.

(kind of a language design edge case, I suppose).

Yeah this is neat, sometimes it's easy to forget that language design has its own edge cases to deal with.

what do you think: Should we avoid this edge case?

To me, it seems like an easy fix to adjust. So I'll make that tweak today and push up a commit to this PR here shortly.

[chipsnyder]

This is actually an interesting situation. @etoledom I didn't run into the warning that you were seeing so I was wondering if it was a runtime flag, but I wasn't able to see it at runtime either.

I do think it would be a good idea to be explicit about the optionality of the event parameters though so I did an update to address that. If you don't mind will you let me know if it fixes the warning?

XCode 11.5

[mkevins]

Fwiw, I didn't see the warning either (but I was exploring the code via the WordPress-iOS project, so I wonder if that's a factor?)

[etoledom]

Oh right, you will see it if you run the gutenberg example project on Xcode.

EDIT:
The last change from 8f13252 solved the warning!

Thank you @chipsnyder for the update 🙏

[etoledom]

Looking great now after the latest changes ✨

Thank you @chipsnyder and @mkevins 🙏