Media previewer #1742

nvbln · 2017-11-10T20:45:06Z

This is intended as a start on issue #860. I was wondering if I could receive some feedback on the way that I am implementing this. The functionality right now is the following:

User picks a media file to share.
sendMediasIntent() will check if the file is an image.
The intent will be send to MediasPreviewerActivity.java
MediasPreviewerActivity.java will then get the image(s) (in this case only one image is supported) from the intent and display it.
The user taps the FloatingActionButton to confirm that the right media file has been selected.
MediasPreviewerActivity.java sends the intent back.
VectorRoomActivity.java will call onActivityResult() which will in turn call mVectorRoomMediasSender.sendMedias() and so the image will be send.

I primarily want to expand on this by making the contain the necessary information (like a highly needed top bar) and by supporting different and multiple media files at the same time (right now just the first one will be used and the only media type that will trigger the preview is a .jpg).

I was mainly wondering if this is the right way to go about it and if there are default things that I should implement in MediasPreviewerAcitivity.java. As an example, I have already implemented checks if the application should restart or is going to splash, but there might be more.

ylecollen · 2017-11-17T08:46:31Z

vector/src/main/java/im/vector/activity/VectorRoomActivity.java

@@ -1943,7 +1947,12 @@ public void run() {
        }

        if (0 != sharedDataItems.size()) {
-            mVectorRoomMediasSender.sendMedias(sharedDataItems);
+            if (sharedDataItems.get(0).getFileName(this).indexOf(".jpg")>-1) {


contains instead of indexOf (i often make the same thing whereas i should not)

ylecollen · 2017-11-17T08:57:12Z

vector/src/main/java/im/vector/activity/VectorRoomActivity.java

@@ -1943,7 +1947,12 @@ public void run() {
        }

        if (0 != sharedDataItems.size()) {
-            mVectorRoomMediasSender.sendMedias(sharedDataItems);
+            if (sharedDataItems.get(0).getFileName(this).indexOf(".jpg")>-1) {
+                intent.setClass(this, MediasPreviewerActivity.class);


1- some other image types can be supported (png, gif...)
2- you cannot guess the list items type from the first one : with a files explorer, you could select a file, a video and an image.

ylecollen · 2017-11-17T08:59:38Z

vector/src/main/java/im/vector/activity/MediasPreviewerActivity.java

+        if (!sharedDataItems.isEmpty()) {
+            ImageView imagePreview = (ImageView) findViewById(R.id.image_previewer);
+            imagePreview.setImageURI(sharedDataItems.get(0).getUri());
+        }


Only the first item ?
A webview might be more simple to manage animated gifs, videos or files

nvbln · 2018-02-05T08:47:14Z

I think the PR is finished now. As a quick summary, I have added the following functionality:

Multiple files are supported, it is possible to preview all of them through the RecyclerView at the bottom of the activity.
Videos are supported, they can be played and paused by tapping on them.
Gif files are supported, they're constantly repeated in a WebView.
There is a general case for unsupported files. In this case a general icon is shown with the title of the file.
The layout has changed a bit (the tool- and statusbar and the Recycleriew are now transparent with a dark shade, the room name has been added (so the user can be sure that the files are send to the right room) and there is a back button to cancel sending the files (because a wrong file has been included for example)).

ylecollen · 2018-03-02T12:27:43Z

@nvbln
1 - it does not compile
2 - i have a black screen half of the time (with photos taken with my device camera)
3- i've got many crashes (out of memory and others)
4- this previewer should be optional. (with a dedicated settings entry)

nvbln · 2018-03-25T10:32:42Z

This should solve 2 and 4 and I think @giomfo took care of 1? (Many thanks!)
As for 4, I took the liberty of making it opt-out, if you'd rather have it the other way around I won't mind changing it of course.

As for the crashes (especially the out of memory ones, as I have (hopefully) fixed the others by properly handling pictures/videos from the camera), could you elaborate on that? Because I am not sure how to go about fixing it. I don't experience these issues myself either (while using an emulator).

nvbln · 2018-05-29T20:06:47Z

I think the review can be postponed until I have fixed the errors (given by the Travis CI build).

bmarty · 2018-07-10T14:29:08Z

Hi @nvbln ,

I'm replacing Yannick to review the PRs.
Travis was run but not with a proper configuration. It should be better now. If you rebase your code to fix the conflict on VectorRoomActivity, this will trigger a new Travis build.

Please add a line describing your change in CHANGES.rst, and also Sign-Off as per https://github.com/matrix-org/synapse/blob/master/CONTRIBUTING.rst#sign-off

I'll do some tests on your PR

Benoît

bmarty · 2018-07-11T08:47:23Z

Hi @nvbln ,

Thanks for the rebase. Please fix error reported by Travis:

Search for forbidden patterns in code...
❌ Error: '(private|public|protected|    ) (static )?(final )?(HashMap|HashSet|ArrayList)<' detected 2 times, last in file './vector/src/main/java/im/vector/activity/MediasPreviewerActivity.java'.
Rule: Use the interface declaration. Example: use type "Map" instead of type "HashMap" to declare variable or parameter
❌ Error: '.{161}' detected 2 times, last in file './vector/src/main/java/im/vector/activity/MediasPreviewerActivity.java'.
Rule: Line length is limited to 160 chars. Please split long lines
❌ Error: 'android\.R\.id\.home' detected 1 time, in file './vector/src/main/java/im/vector/activity/MediasPreviewerActivity.java'.
Rule: Home menu click is managed in parent Activity, with one exception
3 forbidden patterns detected
Search for forbidden patterns in resources...
❌ Error: '\t' detected 83 times, last in file './vector/src/main/res/layout/image_preview.xml'.
Rule: Tab char is forbidden. Use only spaces
1 forbidden pattern detected

❌ Please add a line describing your change in CHANGES.rst

Thanks

bmarty

Hi @nvbln ,

Thank for the update. I've reported quite a lot of remarks, please let me know if you need more explanation on some.

About the UX: Maybe it is useful to display the filename for every media? Because this information will also be sent to the Room

bmarty · 2018-07-12T13:16:15Z

vector/src/main/java/im/vector/activity/MediasPreviewerActivity.java

+        Toolbar toolbar = (android.support.v7.widget.Toolbar) findViewById(R.id.room_toolbar);
+
+        setSupportActionBar(toolbar);
+        getSupportActionBar().setDisplayHomeAsUpEnabled(true);


Replace the 3 lines above by configureToolbar() and rename the id of the toolbar in layout to toolbar

bmarty · 2018-07-12T13:17:45Z

vector/src/main/java/im/vector/activity/MediasPreviewerActivity.java

+        setSupportActionBar(toolbar);
+        getSupportActionBar().setDisplayHomeAsUpEnabled(true);
+
+        String roomTitle = (String) getIntent().getExtras().get(VectorRoomActivity.EXTRA_ROOM_TITLE);


I would prefer that EXTRA_ROOM_TITLE is define in this class

bmarty · 2018-07-12T13:18:13Z

vector/src/main/java/im/vector/activity/MediasPreviewerActivity.java

+        getSupportActionBar().setDisplayHomeAsUpEnabled(true);
+
+        String roomTitle = (String) getIntent().getExtras().get(VectorRoomActivity.EXTRA_ROOM_TITLE);
+        if (roomTitle != null && !roomTitle.isEmpty()) {


Use !TextUtils.isEmpty(roomTitle)

bmarty · 2018-07-12T13:18:49Z

vector/src/main/java/im/vector/activity/MediasPreviewerActivity.java

+        }
+
+        if (!sharedDataItems.isEmpty()) {
+            RecyclerView imagesPreview = (RecyclerView) findViewById(R.id.images_preview);


Please bind views with Butterknife

bmarty · 2018-07-12T13:19:01Z

vector/src/main/java/im/vector/activity/MediasPreviewerActivity.java

+            WebView webPreview = (WebView) findViewById(R.id.web_previewer);
+            VideoView videoPreview = (VideoView) findViewById(R.id.video_previewer);
+            ImageView filePreview = (ImageView) findViewById(R.id.file_previewer);
+            TextView fileNameView = (TextView) findViewById(R.id.file_name);


Please bind all those views with Butterknife

bmarty · 2018-07-12T13:50:24Z

vector/src/main/java/im/vector/util/PreferencesManager.java

+     * @return true to preview media
+     */
+    public static boolean previewMediaWhenSending(Context context) {
+        return PreferenceManager.getDefaultSharedPreferences(context).getBoolean(SETTINGS_PREVIEW_MEDIA_BEFORE_SENDING_KEY, true);


Please disable by default for the moment

bmarty · 2018-07-12T13:56:01Z

vector/src/main/res/layout/activity_medias_previewer.xml

+    <ImageView
+        android:id="@+id/file_previewer"
+        android:layout_width="265dip"
+        android:layout_height="265dip"


Quality of the icon is not good.
Please use wrap_content for width and height and add
android:src="@drawable/filetype_attachment"
No need to set the src it in code then

bmarty · 2018-07-12T13:56:29Z

vector/src/main/java/im/vector/adapters/MediasPreviewAdapter.java

+                    }
+                });
+            } else {
+                filePreview.setImageDrawable(ContextCompat.getDrawable(context, R.drawable.filetype_attachment));


Remove this line (src will be set in layout)

bmarty · 2018-07-12T13:57:46Z

vector/src/main/java/im/vector/adapters/MediasPreviewAdapter.java

+                videoPreview.seekTo(100);
+                videoPreview.setVisibility(View.VISIBLE);
+
+                videoPreview.setOnTouchListener(new View.OnTouchListener() {


Please use setOnClickListener() rather that setOnTouchListener()

bmarty · 2018-07-12T14:00:21Z

vector/src/main/java/im/vector/adapters/MediasPreviewAdapter.java

+            @Override
+            public void onClick(View view) {
+                ViewGroup viewGroup = (ViewGroup) view.getParent().getParent();
+                ImageView imagePreview = (ImageView) viewGroup.findViewById(R.id.image_previewer);


It's not nice to bind this View here. They have to be bound in the Activity they belong to.
Instead notify the Activity of the click event with a listener to provide the clicked item position

nvbln · 2018-07-14T19:57:59Z

There are two things I wanted to note:

I have used OnTouchListener instead of OnClickListener because (as far as I know) OnClickListener doesn't work (it isn't triggered) on a VideoView while OnTouchListener does.
The filenames are now visible for all files. However, I think it is better to leave them out for files that are displayed (videos, images, etc.) because I think that most people do not need it (what the file actually shows is probably more important than its name) and it only clutters the view. But I do not have a strong preference for one or the other.

bmarty

Hi @nvbln ,

Thanks for the update. The code looks good to me. I have a few remarks, once done, I'll merge the PR. I've added the Sprint 12 milestone, which means it should be including in the August release.

Benoît

bmarty · 2018-07-16T14:04:19Z

vector/src/main/java/im/vector/activity/MediaPreviewerActivity.java

+        }
+
+        if (!mSharedDataItems.isEmpty()) {
+            ButterKnife.bind(this);


Butterknife binding is already done by parent class, you can remove this call

bmarty · 2018-07-16T14:04:37Z

vector/src/main/java/im/vector/activity/MediaPreviewerActivity.java

+        finish();
+    }
+
+    public void setPreview(RoomMediaMessage roomMediaMessage) {


I think this method can be private

bmarty · 2018-07-16T14:05:30Z

vector/src/main/java/im/vector/activity/MediaPreviewerActivity.java

+    public void setPreview(RoomMediaMessage roomMediaMessage) {
+
+        // Prevent blinking when tapping on the same item multiple times.
+        if (mCurrentRoomMediaMessage != null && roomMediaMessage == mCurrentRoomMediaMessage) {


I think test on nullity is not necessary

bmarty · 2018-07-16T14:06:07Z

vector/src/main/java/im/vector/activity/MediaPreviewerActivity.java

+        // Prevent blinking when tapping on the same item multiple times.
+        if (mCurrentRoomMediaMessage != null && roomMediaMessage == mCurrentRoomMediaMessage) {
+            return;
+        } else {


remove the else block, just do the affectation of mCurrentRoomMediaMessage

bmarty · 2018-07-16T14:07:27Z

vector/src/main/java/im/vector/activity/VectorRoomActivity.java

+                intent.setClass(this, MediaPreviewerActivity.class);
+                startActivityForResult(intent, CONFIRM_MEDIA_REQUEST_CODE);
+            } else if (null != mLatestTakePictureCameraUri) {
+                intent.putExtra(MediaPreviewerActivity.EXTRA_CAMERA_PICTURE_URI, mLatestTakePictureCameraUri);


no need to call setClass() here?

bmarty · 2018-07-16T14:08:34Z

vector/src/main/java/im/vector/adapters/MediaPreviewAdapter.java

+            }
+        }
+
+        final int itemPosition = position;


parameter position can be declared final, to avoid creating a new variable itemPosition

bmarty · 2018-07-16T14:09:02Z

vector/src/main/java/im/vector/adapters/MediaPreviewAdapter.java

+
+    @Override
+    public void onAttachedToRecyclerView(RecyclerView recyclerView) {
+        super.onAttachedToRecyclerView(recyclerView);


Is this necessary? I think it can be removed

nvbln · 2018-07-16T16:15:14Z

Sorry for closing/reopening the PR, that was an accident.

I haven't removed setClass() as it will result in a ActivityNotFoundException (as in some cases the class is not clear). I have cleaned the if-clauses up a bit, so things should be more clear now. Also, when using final int position Travis will not accept it, so that's why I use a separate variable.

bmarty

two minor remarks. Also would you squash all your commits (note that I can do it during the merge process)?

bmarty · 2018-07-16T16:47:34Z

vector/src/main/java/im/vector/activity/VectorRoomActivity.java

-            mLatestTakePictureCameraUri = null;
+            intent.setClass(this, MediaPreviewerActivity.class);
+        } else {
+            intent = new Intent(this, MediaPreviewerActivity.class);


Ok, thanks, it's clearer now. Maybe you should keep the if to initiate sharedDataItems, but move the code related to MediaPreviewerActivity to the if block with PreferencesManager.previewMediaWhenSending(this).

bmarty · 2018-07-16T16:48:32Z

vector/src/main/java/im/vector/activity/MediaPreviewerActivity.java

+    private void setPreview(RoomMediaMessage roomMediaMessage) {
+
+        // Prevent blinking when tapping on the same item multiple times.
+        if (!(roomMediaMessage == mCurrentRoomMediaMessage)) {


Please simplify: if (roomMediaMessage != mCurrentRoomMediaMessage) {

Add RecyclerView to MediasPreviewerActivity Add Toolbar to MediasPreviewerActivity Improve the layout of MediasPreviewerActivity Add support for videos and gifs Also changed the name of ImagePreviewAdapter to a more general name, namely: MediasPreviewAdapter Add general case for unsupported filetypes Remove the border around the CardView Properly handle camera URI Make the previewer optional Update code to most recent develop Fix lint error Remove forbidden patterns Describe changes of PR and sign off Signed-off-by: Nathan van Beelen <nathan at vanbeelen dot org> Replace tab indentation with spaces Remove forbidden pattern Improve the code Rename the Activity and Adapter Also correct an incorrect reference to the ItemPositionChangeListener method. Show filename for all files Rename the layout file of the media previewer Properly resize content in the WebView Improve the code further Add and update comments Further improve the code

Update matrix-sdk.aar lib - build 1875 - Revision: ccf12449b8f09b06a7a8f501b9d7a382270b2305 Last rebase was performed June 1 2018. Here is the list of changes from vector-im/riot-android changes: Changes in Riot 0.8.XX (2018-08-24) =================================================== Features: - Manage server quota notices (#2440) Improvements: - Do not ask permission to write external storage at startup (#2483) - Update settings icon and transparent logo for notifications and navigation drawer (#2492) - URL previews are no longer requested from the server when displaying URL previews is disabled (PR #2514) - Fix some plural and puzzle strings, and remove other unused ones (#2444) - Manage System Alerts in a dedicated section Other changes: - Upgrade olm-sdk.aar from version 2.2.2 to version 2.3.0 - move PieFractionView from the SDK to the client (#2525) Bugfix: - Fix media sharing (#2530) - Fix notification sound issue in settings (#2524) - Disable app icon badge for "listen for event" notification (#2104) Changes in Riot 0.8.13 (2018-08-09) =================================================== Features: - Resurrect performance metrics (#2391) - Telemetry to report incidence of UISIs (#2330) - Add a previewer for previewing media before sending it into the room (#1742|#2445) - Implements ReplyTo feature (#2390) - Add auto completion for slash commands (#2384) - Support Room Versioning (#2441) Improvements: - Update matrix-sdk.aar lib (v0.9.7). - Piwik: Update the way how stats are reported (#2402) - Improve BugReport screen: display a preview of the screenshot (#2318) - In the settings, move theme settings just below "language" (#2439) - Improve the display of the sources of the message in the dialog (#2348) - Improve the display of the buttons and the reason in the room preview (#2352) - In the flair section on settings, notify the user when he has no flair (#2430) - Improve GDPR consent webview management (#2491) - Support external keyboard to send messages for recent devices (#220, #1279) - When user ignores or un-ignores someone, notify that the app will restart (#2437) Other changes: - Remove dependency to `android-gif-drawable` lib and use Glide to animate logo on Splashscreen (#2421) - Keep only Room.getState() method and remove Room.getLiveState() because they are similar (matrix-org/matrix-android-sdk#310) Bugfix: - Fix issue on incoming call screen when "Do not disturb mode" is active (#2417) - Fix issue when selecting sound for notifications in the settings - Fix issue when changing device name in the settings (#2416) - Fix issue on verifying device, update the wording of the description message (#1067) - Messages with code blocks show other HTML as plain text (#2280) - Message with <p> was sometimes not properly formatted (#2275) - Fix notification issue when Riot is not started (#2451) - Fix Unable to add Matrix apps (#2466) - Riot auto joined a public room (#2472) - Remove last traces of Firebase analytics (#2481) - code blocks are escaped and therefore hard readable (#2484) - Restore the navigation of the back button in the public rooms preview header (#2473) - Fix issue on preference screen: device lists was not displayed (#2409) - Ensure notification has a title (#2242) Changes in Riot 0.8.12 (2018-07-06) =================================================== Bugfix: - Fix issue on vanished favorite and low priority room (#2413) Changes in Riot 0.8.11 (2018-07-03) =================================================== Features: - Re-request keys manually for encrypted events (#2319) - Add option to send voice message to a room, using a third application to record message. To enable in the Labs settings (PR #1762) Improvements: - Update matrix-sdk.aar lib (v0.9.6). - New Floating Action Menu in Home screen (PR #2335) - Add spacing to device keys (#2314) - use apply() instead of commit() to save shared prefs (#2231) - Do not ring if "Do Not Disturb" is active (#1072) - Manage the "consent not given" error when declining a room invite Other changes: - Remove "Matrix application" activation from the Lab section in the settings (#2341) Bugfix: - Remove black borders on 18:9 phone (#2063) - Auto dismiss the join/reject room notification when user select an action (#2354) - Fix some crashes reported by the PlayStore (#2380, #2382, #2383, #2395) - Fix issues in UrlPreviews (#2312) Translations: - Galician thanks to Miguel Branco Build: - Add script to check code quality - Travis will now check if CHANGES.rst has been modified for each PR ------ Merge commit '94b7925a57b800db62b5ec87966d994ff53392b4' into develop # Conflicts: # vector/src/main/java/im/vector/activity/VectorRoomActivity.java # vector/src/main/java/im/vector/adapters/VectorMediasViewerAdapter.java # vector/src/main/java/im/vector/db/VectorContentProvider.java # vector/src/main/java/im/vector/dialogs/ConsentNotGivenHelper.kt # vector/src/main/java/im/vector/fragments/VectorSettingsPreferencesFragment.kt # vector/src/main/res/values/strings.xml # vector/src/main/res/values/styles.xml

ylecollen reviewed Nov 17, 2017

View reviewed changes

nvbln changed the title ~~WIP: media previewer~~ Media previewer Feb 5, 2018

giomfo self-requested a review February 7, 2018 08:12

bmarty force-pushed the develop branch from 57a3655 to c0f21b6 Compare July 6, 2018 12:07

nvbln force-pushed the develop branch from 1bcf175 to c2dd21f Compare July 10, 2018 19:33

bmarty suggested changes Jul 12, 2018

View reviewed changes

nvbln force-pushed the develop branch from a9f6cfd to 5246d77 Compare July 14, 2018 18:25

bmarty added this to the Sprint 12 milestone Jul 16, 2018

bmarty suggested changes Jul 16, 2018

View reviewed changes

nvbln closed this Jul 16, 2018

nvbln reopened this Jul 16, 2018

nvbln force-pushed the develop branch from ca03b23 to 0dd3f81 Compare July 16, 2018 16:07

bmarty suggested changes Jul 16, 2018

View reviewed changes

bmarty requested a review from ganfra July 17, 2018 07:19

nvbln force-pushed the develop branch from 4ddc676 to eb17636 Compare July 17, 2018 07:48

bmarty merged commit 6fa4686 into element-hq:develop Jul 17, 2018

bmarty mentioned this pull request Jul 17, 2018

Image preview : add image edition before sending it. #860

Closed

Media previewer #1742

Media previewer #1742

Uh oh!

Conversation

nvbln commented Nov 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvbln commented Feb 5, 2018

Uh oh!

ylecollen commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvbln commented Mar 25, 2018

Uh oh!

nvbln commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmarty commented Jul 10, 2018

Uh oh!

bmarty commented Jul 11, 2018

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvbln commented Jul 14, 2018

Uh oh!

bmarty left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nvbln commented Jul 16, 2018

Uh oh!

bmarty left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ylecollen commented Mar 2, 2018 •

edited

Loading

nvbln commented May 29, 2018 •

edited

Loading

bmarty left a comment •

edited

Loading

bmarty left a comment •

edited

Loading