Add WPMediaUtils and its few dependencies
#25
Conversation
|
@anitaa1990 Just a heads up - I'll be adding a few comments in a minute for easier review |
|
Thanks for the heads up @wzieba! Let me know when it's good for a review by requesting a review and I'll take a look 👍 |
There was a problem hiding this comment.
@anitaa1990 I've added comments, mostly link to new issues I've created while working on this PR.
I've created a new project (Improving quality of MediaPicker codebase). I moved there issues which are nice-to-have but not completely necessary for first iteration.
I hope that this way it'll be easier to focus/pick issues that are must-have (which are in Extracting MediaPicker and make it a library project).
| * [UiString] is a utility sealed class that represents a string to be used in the UI. It allows a string to be | ||
| * represented as both string resource and text. | ||
| */ | ||
| sealed class UiString { |
There was a problem hiding this comment.
Related issue: Decide if we want to use UiString (#31)
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class WPMediaUtils { |
There was a problem hiding this comment.
Related issue: Convert WPMedtiaUtils to kotlin (#27)
| try { | ||
| fileUri = FileProvider.getUriForFile(context, applicationId + ".provider", new File(mediaCapturePath)); | ||
| } catch (IllegalArgumentException e) { | ||
| Log.e(TAG, "Cannot access the file planned to store the new media", e); |
There was a problem hiding this comment.
Related issue: Provide logging interface (#30)
| return R.drawable.media_powerpoint; | ||
| } else if (MediaUtils.isSpreadsheet(url)) { | ||
| return R.drawable.media_spreadsheet; |
There was a problem hiding this comment.
Related issue: Convert all png icons to svg (#26)
| <string name="sdcard_title">SD Card Required</string> | ||
| <string name="sdcard_message">A mounted SD card is required to upload media</string> |
There was a problem hiding this comment.
| import android.content.Context | ||
| import android.net.Uri | ||
|
|
||
| class WPMediaUtilsWrapper(private val context: Context) { |
There was a problem hiding this comment.
Related issue: Make WPMediaUtils and WPMediaUtilsWrapper consistent (#28)
| @@ -216,6 +216,7 @@ class MediaPickerActivity : LocaleAwareActivity(), MediaPickerListener { | |||
| IMAGE_EDITOR_EDIT_IMAGE -> { | |||
| data?.let { | |||
| val intent = Intent() | |||
| // TODO: 20/07/2021 There's a whole ImageEditor module in WPAndroid. Should we import it? | |||
There was a problem hiding this comment.
@anitaa1990 do you think we should also add ImageEditor to the MediaPicker? If not (or not in the first iteration) we will need to provide some interfaces to glue together MediaPicker and ImageEditor from WPAndroid.
| @@ -216,6 +216,7 @@ class MediaPickerActivity : LocaleAwareActivity(), MediaPickerListener { | |||
| IMAGE_EDITOR_EDIT_IMAGE -> { | |||
| data?.let { | |||
| val intent = Intent() | |||
| // TODO: 20/07/2021 There's a whole ImageEditor module in WPAndroid. Should we import it? | |||
There was a problem hiding this comment.
ImageManager is tightly coupled with the media picker feature 🤔 I'm not really thrilled to migrate that here but maybe we can do the bare minimum and import what is necessary here. Because this class seems to handle everything related to image loading.
But I am wondering what would happen when this is getting merged into the WordPress app. Because almost all of the util classes are being utilised by WordPress (not related to the media picker) so they might have two classes of everything 😬
It might be good to also rename files at some point to ensure that it is very specific to media picker. Not blocking but just something to keep in mind
This PR focuses on bringing minimal* version of
WPMediaUtilsand its wrapper.*minimal I mean without code that is not used by
mediapickerpackage