Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue/715 request cancel image upload #736

Merged
merged 21 commits into from
Mar 19, 2019
Merged

Issue/715 request cancel image upload #736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
b5860f8
Merge pull request #726 from wordpress-mobile/release/1.1.0
etoledom Mar 8, 2019
bb017db
added hook 'blocks.onRemoveBlockCheckUpload' for trashed image block …
mzorz Mar 11, 2019
b7c72c5
fixed filter removal to work from callback function only
mzorz Mar 11, 2019
1f3f4cf
fixed hash for gutenberg, unused import
mzorz Mar 11, 2019
e4925d9
added bridge functionality to allow JS to request an in-progress medi…
mzorz Mar 11, 2019
7887b64
removed TODO comment
mzorz Mar 12, 2019
38bcc24
merged develop
mzorz Mar 12, 2019
05a734c
added type annotation
mzorz Mar 12, 2019
adcc19f
added missing space
mzorz Mar 12, 2019
14c2520
updated gutenberg hash
mzorz Mar 13, 2019
160c51b
moving bridge-specific code to Image component - if the filter exists…
mzorz Mar 13, 2019
6b682a7
Merge branch 'develop' into issue/715-request-cancel-image-upload
mzorz Mar 13, 2019
1abd725
using hook actions instead of filters
mzorz Mar 13, 2019
d182b08
removed extra line
mzorz Mar 13, 2019
79d789a
Implement image upload cancelation in iOS.
SergioEstevao Mar 14, 2019
6bc5f46
updated gb hash
mzorz Mar 15, 2019
0a51ab3
Merge branch 'issue/715-request-cancel-image-upload' of https://githu…
mzorz Mar 15, 2019
c7686eb
Merge branch 'develop' into issue/715-request-cancel-image-upload
mzorz Mar 15, 2019
53ff558
Merge branch 'develop' into issue/715-request-cancel-image-upload
SergioEstevao Mar 15, 2019
a415593
updated GB hash
mzorz Mar 19, 2019
47ea808
Merge branch 'develop' into issue/715-request-cancel-image-upload
mzorz Mar 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions android/app/src/main/java/com/gutenberg/MainApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ public void requestImageFailedRetryDialog(int mediaId) {}
@Override
public void requestImageUploadCancelDialog(int mediaId) {}

@Override
public void requestImageUploadCancel(int mediaId) {}

@Override
public void editorDidMount(boolean hasUnsupportedBlocks) {}
})
Expand Down
2 changes: 1 addition & 1 deletion gutenberg
Open in desktop
Submodule gutenberg updated 1 files
+6 −0 packages/block-library/src/image/edit.native.js
9 changes: 9 additions & 0 deletions ios/gutenberg/GutenbergViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,15 @@ extension GutenbergViewController: GutenbergBridgeDelegate {
alertController.popoverPresentationController?.permittedArrowDirections = .any
present(alertController, animated: true, completion: nil)
}

/// Tells the delegate that an image block requested for the upload cancelation.
///
func gutenbergDidRequestMediaUploadCancelation(for mediaID: Int32) {
guard let progress = mediaUploadCoordinator.progressForUpload(mediaID: mediaID) else {
return
}
progress.cancel()
}
}

extension GutenbergViewController: GutenbergBridgeDataSource {
Expand Down
2 changes: 2 additions & 0 deletions ...c/main/java/org/wordpress/mobile/ReactNativeGutenbergBridge/GutenbergBridgeJS2Parent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,6 @@ interface MediaUploadCallback {
void requestImageFailedRetryDialog(int mediaId);

void requestImageUploadCancelDialog(int mediaId);

void requestImageUploadCancel(int mediaId);
}
5 changes: 5 additions & 0 deletions ...a/org/wordpress/mobile/ReactNativeGutenbergBridge/RNReactNativeGutenbergBridgeModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ public void requestImageUploadCancelDialog(final int mediaId) {
mGutenbergBridgeJS2Parent.requestImageUploadCancelDialog(mediaId);
}

@ReactMethod
public void requestImageUploadCancel(final int mediaId) {
mGutenbergBridgeJS2Parent.requestImageUploadCancel(mediaId);
}

private MediaSelectedCallback getNewMediaSelectedCallback(final Callback jsCallback) {
return new MediaSelectedCallback() {
@Override public void onMediaSelected(int mediaId, String mediaUrl) {
Expand Down
6 changes: 6 additions & 0 deletions ...rg-bridge/android/src/main/java/org/wordpress/mobile/WPAndroidGlue/WPAndroidGlueCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public interface OnMediaLibraryButtonListener {
void onCapturePhotoButtonClicked();
void onRetryUploadForMediaClicked(int mediaId);
void onCancelUploadForMediaClicked(int mediaId);
void onCancelUploadForMediaDueToDeletedBlock(int mediaId);
}

public interface OnReattachQueryListener {
Expand Down Expand Up @@ -142,6 +143,11 @@ public void requestImageUploadCancelDialog(int mediaId) {
mOnMediaLibraryButtonListener.onCancelUploadForMediaClicked(mediaId);
}

@Override
public void requestImageUploadCancel(int mediaId) {
mOnMediaLibraryButtonListener.onCancelUploadForMediaDueToDeletedBlock(mediaId);
}

@Override
public void editorDidMount(boolean hasUnsupportedBlocks) {
mOnEditorMountListener.onEditorDidMount(hasUnsupportedBlocks);
Expand Down
4 changes: 4 additions & 0 deletions react-native-gutenberg-bridge/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,8 @@ export function requestImageUploadCancelDialog( mediaId ) {
return RNReactNativeGutenbergBridge.requestImageUploadCancelDialog( mediaId );
}

export function requestImageUploadCancel( mediaId ) {
return RNReactNativeGutenbergBridge.requestImageUploadCancel( mediaId );
}

export default RNReactNativeGutenbergBridge;
4 changes: 4 additions & 0 deletions react-native-gutenberg-bridge/ios/GutenbergBridgeDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ public protocol GutenbergBridgeDelegate: class {
///
func gutenbergDidRequestMediaUploadActionDialog(for mediaID: Int32)

/// Tells the delegate that an image block requested for the upload cancelation.
///
func gutenbergDidRequestMediaUploadCancelation(for mediaID: Int32)

/// Tells the delegate that the Gutenberg module has finished loading.
///
func gutenbergDidLoad()
Expand Down
1 change: 1 addition & 0 deletions react-native-gutenberg-bridge/ios/RNReactNativeGutenbergBridge.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ @interface RCT_EXTERN_MODULE(RNReactNativeGutenbergBridge, NSObject)
RCT_EXTERN_METHOD(mediaUploadSync)
RCT_EXTERN_METHOD(requestImageFailedRetryDialog:(int)mediaID)
RCT_EXTERN_METHOD(requestImageUploadCancelDialog:(int)mediaID)
RCT_EXTERN_METHOD(requestImageUploadCancel:(int)mediaID)
RCT_EXTERN_METHOD(editorDidLayout)
RCT_EXTERN_METHOD(editorDidMount:(BOOL)hasUnsupportedBlocks)

Expand Down
7 changes: 7 additions & 0 deletions react-native-gutenberg-bridge/ios/RNReactNativeGutenbergBridge.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,13 @@ public class RNReactNativeGutenbergBridge: RCTEventEmitter {
}
}

@objc
func requestImageUploadCancel(_ mediaID: Int32) {
DispatchQueue.main.async {
self.delegate?.gutenbergDidRequestMediaUploadCancelation(for: mediaID)
}
}

@objc
func editorDidLayout() {
DispatchQueue.main.async {
Expand Down
18 changes: 18 additions & 0 deletions src/block-management/block-holder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,16 @@ import {
NativeTouchEvent,
} from 'react-native';
import InlineToolbar, { InlineToolbarActions } from './inline-toolbar';
import {
requestImageUploadCancel,
} from 'react-native-gutenberg-bridge';

/**
* WordPress dependencies
*/
import { withDispatch, withSelect } from '@wordpress/data';
import { compose } from '@wordpress/compose';
import { addAction, removeAction, hasAction } from '@wordpress/hooks';

import type { BlockType } from '../store/types';

Expand Down Expand Up @@ -77,6 +84,14 @@ export class BlockHolder extends React.Component<PropsType, StateType> {
this.props.onSelect( this.props.clientId );
};

onRemoveBlockCheckUpload = ( mediaId: number ) => {
if ( hasAction( 'blocks.onRemoveBlockCheckUpload' ) ) {
// now remove the action as it's a one-shot use and won't be needed anymore
removeAction( 'blocks.onRemoveBlockCheckUpload', 'gutenberg-mobile/blocks' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙇

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

requestImageUploadCancel( mediaId );
}
}

onInlineToolbarButtonPressed = ( button: number ) => {
switch ( button ) {
case InlineToolbarActions.UP:
Expand All @@ -86,6 +101,9 @@ export class BlockHolder extends React.Component<PropsType, StateType> {
this.props.moveBlockDown();
break;
case InlineToolbarActions.DELETE:
// adding a action that will exist for as long as it takes for the block to be removed and the component unmounted
// this acts as a flag for the code using the action to know of its existence
addAction( 'blocks.onRemoveBlockCheckUpload', 'gutenberg-mobile/blocks', this.onRemoveBlockCheckUpload );
this.props.removeBlock();
break;
}
Expand Down