Issue/715 request cancel image upload

[mzorz]

Fixes #715

Gutenberg PR WordPress/gutenberg#14391
WPAndroid PR wordpress-mobile/WordPress-Android#9402

Until now, when the user started uploading an image in an image block, the image will continue to upload until finished doing so regardless of the block containing such image being deleted.

This PR adds a new method to the bridge interface to request the host app to handle an in-progress upload cancellation.

The bridge methods added are self-explained as they follow the same structure of what we already have in place, but the way we detect a block being deleted might use some further explanation:

• when the user taps on the trash icon, we detect that event without ambiguity in the Toolbar handler. It is at this point that we add a filter add an action that will be called later. The filter action is added here to handle the situation, but is needed to be invoked from the component that has knowledge of the mediaId corresponding to the in-progress upload.

• when the Image component that is part of this ImageBlock is about to be unmounted, we make use of the action. If the filter action exists, it will be executed and is passed the needed information (media id and whether it's an in progress upload or not).

• the filter action then checks that the mediaId exists and is an in-progress upload and signals the host app to cancel this upload by means of the bridge.

• Finally, the filter action is removed as it no longer serves a purpose. You can see this filter action is added and removed in a brief amount of time.

I'm not 100% sold on this approach so open to suggestions, but I couldn't find a better way to:

• a) make sure the intent from the user is detected unequivocally (that is, they really did tap on the trash icon, intending to eliminate this block entirely). This is where the filter is installed, so the Image component (who knows about mediaID) knows it's being unmounted because of a removal.
• b) get the media ID information from the underlying Image component (as this information lives there at that level, and not on the Block or Block Holder levels).
• c) be able to call the bridge from where it makes sense. Note that we could also do this at the Image component level (as we are doing with other Media elements), but it still made sense to make the filter perform the bridge call and remove itself afterwards, giving the loop a closure.

Note: The iOS side of the bridge still needs to be implemented

To test:

1. start a draft
2. insert an image block
3. choose an image from the device so it starts uploading
4. once it starts uploading, tap on the block's trash icon
5. observe the foreground upload notification is also dismissed (meaning, the upload has been cancelled).

[marecar3]

Hey @mzorz, thanks for the explanation.

I am sharing your thoughts that this approach could be better. Not sure if that it's a good approach ( from architecture side ) that BlockHolder is aware of Image upload process (cancellation), also I am not a fan of event driven approach as in some cases it is really hard to debug the application.

Maybe cleaner way would be to provide (pass delete/remove action) event from BlockHolder -> ImageBlock before this.props.removeBlock() is got called.

Probably it would be good to hear from @Tug if he has something on his mind about this specific problem ?

[mzorz]

Not sure if that it's a good approach ( from architecture side ) that BlockHolder is aware of Image upload process (cancellation), also I am not a fan of event driven approach as in some cases it is really hard to debug the application.

Agreed! Maybe we can use the filter to only plant the flag that it was the trash icon that was pressed, and do the actual cancellation and logic in the Image component. I can do that and see how it goes!

Probably it would be good to hear from @Tug if he has something on his mind about this specific problem ?

Great idea, thank you for proposing it! I'd also love to hear any hints from @Tug :)

[mzorz]

@marecar3 I moved the bridge-specific code to the Image component in 160c51b and WordPress/gutenberg@cb063b8

Maybe cleaner way would be to provide (pass delete/remove action) event from BlockHolder -> ImageBlock before this.props.removeBlock() is got called.

This kept me wondering, I think passing a prop down to the Component will make it re-render and ideally that's something we would want to avoid (especially since we are going to the redux store to remove it right after); I think the change suggested in the commit above makes the logic sligthly better (or easier to understand / separating responsibilities) - still open to suggestions as to how it'd be best 👍

[daniloercoli]

the filter then just checks that the mediaId exists and is an in-progress upload and signals the host app to cancel this upload by means of the bridge.

Filters in WordPress eco-system are functions used to modify and pass data through. (Passing data through filters allows developers to modify the default behavior of a specific function.)

I guess in this case we need to use an Action instead, and add the mediaID to the payload of the action. WordPress actions are used to execute a specific code at a specific event.

[mzorz]

I guess in this case we need to use an Action instead, and add the mediaID to the payload of the action. WordPress actions are used to execute a specific code at a specific event.

🙇 thank you @daniloercoli ! Will go that way 👍

[mzorz]

Implemented the action instead of filter as suggested by @daniloercoli in 1abd725 and WordPress/gutenberg@65fd883 - cc @marecar3 as the actual cancellation and bridge call have been moved back to where the action is defined (the block-holder). Hope it makes more sense now!

[Tug]

Sorry for the delay, I had a quick look and I must admit I'm not a fan of having this logic in BlockHolder, but I don't think we have much choice at the moment.

I guess a better alternative would be to design a better bridge that can handle setting/removing hooks from the native code, this way we could have the hook set when the upload starts until it finishes and be notified from gb when the component is unmounted.

[mzorz]

Sorry for the delay, I had a quick look and I must admit I'm not a fan of having this logic in BlockHolder, but I don't think we have much choice at the moment.

Thanks for chiming in @Tug ! Yes I agree this is not ideal but does the job for now.

I guess a better alternative would be to design a better bridge that can handle setting/removing hooks from the native code, this way we could have the hook set when the upload starts until it finishes and be notified from gb when the component is unmounted.

Definitely that'd be closer to ideal! One problem posed by having callback references in both worlds is that these reside in memory, so it would be best to be able to be able to recover callbacks / hooks on state change, for example in Aztec we can recover media items by marking them in content itself. We don't have such "metadata" in Gutenberg (or do we?). Anyway, certainly matter of lengthier discussion 👍

So, ready for another round @marecar3 🙇

[marecar3]

Maybe it would be a good option to put those two arguments in some constants so that we can reuse them in other parts of the code?

[mzorz]

I was looking into the Gutenberg code for other addAction / removeAction calls elsewhere and they seem to have the text right there so, followed that. Does that make sense?

Adding a note here as well - I also considered using an instance ID as seen in that piece of code before to make sure to have different actions for each of the blocks, but given it all happens in a brief amount of time (from the time the user presses the trash icon making the action be added, and then when the component gets unmounted where the action gets removed), it didn't make sense to add the complexity of having to add a props to pass down to the component (which essentially would make it re-render and then again, make the bridge change unnecessary at the block holder level).

[marecar3]

Well if someone tries to change action name, he would need to change it on 3 different places. I think that it's a good practice to have one constant which will hold the name of the action?

[mzorz]

I think that it's a good practice

sure thing - don't get me wrong, I totally understand what you're saying in terms of how having a constant is useful to avoid having the text modified and introduce bugs this way 😅 👍.

The thing (I left a comment about it in the other PR) is we would need to define it in gutenberg-mobile (where block holder lives) and import it in the gutenberg repo (where the Image component lives) making the block holder even aware of the Image component, which seems off. If we do it the other way around then we make the Image component aware of something defined in gutenberg-mobile block-holder. Both ways seem strange to me 🤔 .

Perhaps I'm missing something, can you explain how would you do it? thanks in advance for your time and patience 🙇

[marecar3]

Ah ok thanks for the explanation, I thought that we are not on the same page regarding what should be done. Ok in that case :)

[mzorz]

Excellent! Glad it's sorted out 😄 - do you think this is good to be approved then @marecar3?

[SergioEstevao]

@mzorz I'm implementing the behaviour in iOS using this PR/branch.

[SergioEstevao]

@Tug one question, the component being umounted it doesn't mean the block is deleted right?

I was thinking couldn't we send an onDelete event from the block holder when the delete button is pressed? the onDelete event will have the block Id , and the image block could listen to it, so if they receive the event and it belongs to them they send cancelUpload message to the bridge? or the event will arrive too late to them?

[marecar3]

@Tug one question, the component being umounted it doesn't mean the block is deleted right?

I was thinking couldn't we send an onDelete event from the block holder when the delete button is pressed? the onDelete event will have the block Id , and the image block could listen to it, so if they receive the event and it belongs to them they send cancelUpload message to the bridge? or the event will arrive too late to them?

Yeah, I am sharing @SergioEstevao thoughts as it could be a cleaner solution to send an event from BlockHolder to ImageBlock. @mzorz did you maybe try that approach?

[mzorz]

hi @SergioEstevao @marecar3 !

the onDelete event will have the block Id , and the image block could listen to it, so if they receive the event and it belongs to them

Unfortunately AFAIU the Block does not have the mediaID that we need in order to figure out which in-progress upload to cancel. What we know at the block level is the clientId but it has nothing to do with the media it's holding. On the other hand, the Image component within the block only knows about mediaId (through props.attributes.id). We could maybe pass the block's clientId as a props, but not sure the Image component needs to know about that.

And then this other problem:

or the event will arrive too late to them?

I think that's the case, as we are first removing the block and then we wuold be sending the event. We could try otherwise but AFAIU there's no guarantee both events will be serialized (we can't assume order will be respected)

[mzorz]

Forgot to add this @SergioEstevao :

the component being umounted it doesn't mean the block is deleted right?

That's correct, and that's the purpose of adding the addAction/hasAction/doAction: to signal the component that, if the hook's hasAction returns true, it means it's being unmounted because of the user tapped on the trash icon.

Not a fan of deriving this fact from that flag, but...

[Tug]

Yeah I don't think it would improve the code in this case, you'd have the Image block listen for an event that's defined outside of gutenberg.

The way it's designed at the moment, Image emits its own event and it can be caught (or ignored) from anywhere, pretty neat.

In the future, I'd like to see the hook system hard wired to the native app, so you could do something like:

uploadImageFile( mediaFile );
GutenbergEditorFragment.setGutenbergActionListener('blocks.onRemoveBlockCheckUpload', 'gutenberg-mobile/blocks', new GutenbergActionListener() {
    @Override
    public boolean onAction(Object data) {
        int mediaId = (int) data;
        mOnMediaLibraryButtonListener.onCancelUploadForMediaDueToDeletedBlock(mediaId);
    }
});

[mzorz]

In the future, I'd like to see the hook system hard wired to the native app, so you could do something like:

That'd be great! Opening up for extensibility sounds good 👍

[marecar3]

LGTM!