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

Jump to bottom

consistent Delete/Copy/Save UX across media types #1105

Merged
merged 2 commits into from
Jul 18, 2016
Merged

consistent Delete/Copy/Save UX across media types #1105

merged 2 commits into from
Jul 18, 2016

Conversation

michaelkirk
Copy link
Contributor

@michaelkirk michaelkirk commented Mar 11, 2016 •
edited
Loading

Delete/Copy/Save media items via long press menu from within conversation view.

Fix for #1103 and #704

The new functionality is

  • delete: video, audio, photo, and animated GIF messages
  • copy: video (as mp4), photo (as jpg), and animated GIF messages (was already possible to copy plain text message)
  • save: video (as mp4), photo (as jpg), and animated GIF messages (intentionally not possible to save plain text message or audio messages)

Motivation

This is in line with how people expect to interact with messages (e.g. see WhatsApp and the Native Messaging app), and we were already doing it with Text messages. But for some reason we were doing these actions differently for images, and not at all for videos.

Previous UX

In order to delete an image, first, you'd tap on the image to bring up the full screen view:

image

Then, tap "Share" button to open an action sheet, then press delete:
image

Proposed UX

Now we're doing this consistently across message types using the edit menu, via long-press:

image

Consequently the "share" icon has been removed from the fullscreen image view:
image

@michaelkirk michaelkirk mentioned this pull request Mar 12, 2016
Closed
@rocknjohn
Copy link
Contributor

Looks nice Michael. The native action sheets look much nicer. The DJW animations had felt unsettling too.

But wouldn't it make sense to have "Save/Copy/Delete" all available when long pressing on an image (#825), instead of only "Delete". These are all common actions.

@michaelkirk
Copy link
Contributor Author

@rocknjohn - I agree with you. Plus that's the behavior of native messaging app.

Here's what would need to be done:

  • Move "save" action from "share" menu to "edit" menu
  • Move the "copy" action from "share" to "edit" menu
  • Kill the "share" menu all together until there is an actual "share" action you can do with it.
  • Add a "copy" action for videos
  • Add a "save" action for videos

I won't get to it until later this week, per other working obligations. Thanks for the feedback!

@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch 2 times, most recently from 62263ae to 9cbe597 Compare March 19, 2016 01:18
@michaelkirk
Copy link
Contributor Author

OK - some progress here. This PR is getting pretty large...

I've killed off the "share" menu all together, since the items in it are more conventionally found in the edit-menu (that's this guy, which you get to by long pressing).

image

The bad news is that until #793 is complete, we won't be able to paste anything into Signal. So, silly enough, you can copy these things, but you can still only paste text into Signal at this point.

The feature feels incomplete without also fixing the paste situation, but this PR is already getting quite big. I'll update the description to reflect the current state of things.

@michaelkirk michaelkirk changed the title consistent "Delete" UX (including all media messages) consistent "Delete"/"Copy"/"Save" UX (including all media messages) Mar 19, 2016
@michaelkirk michaelkirk changed the title consistent "Delete"/"Copy"/"Save" UX (including all media messages) consistent Delete/Copy/Save UX across media types Mar 19, 2016
michaelkirk
michaelkirk reviewed Mar 19, 2016
View reviewed changes
Signal/src/view controllers/TSMessageAdapter.m
if (action == @selector(delete:)) {
DDLogDebug(@"Deleting interaction with uniqueId: %@", self.interaction.uniqueId);
[self.interaction remove];
} else if (action == @selector(copy:)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These long type reflection switch statements are screaming for refactoring based on inheritance, but I was trying not to move too much at once.

@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch 3 times, most recently from 42e0806 to a8e59c6 Compare March 25, 2016 00:46
@pstasiak
Copy link

But wouldn't it make sense to have "Save/Copy/Delete" all available when long pressing on an image (#825), instead of only "Delete". These are all common actions.

Hey, I've just created #1127 with an update of JSQMessagesViewController with copy media functionality.

@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch 7 times, most recently from 0afbff8 to a0b7f98 Compare March 27, 2016 02:22
@rocknjohn rocknjohn mentioned this pull request Mar 28, 2016
Closed
@greycubesgav
Copy link

+1 for this to be merged and committed to the App Store available version.

@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch from 82c7d93 to c177d65 Compare April 8, 2016 12:20
@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch 4 times, most recently from 73d6a87 to a497d8b Compare July 16, 2016 01:33
@michaelkirk michaelkirk mentioned this pull request Jul 17, 2016
Closed
@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch from 1b461dc to a4a9b43 Compare July 17, 2016 16:13
@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch 4 times, most recently from a2c15ff to 966ebd4 Compare July 17, 2016 16:39
@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch from 966ebd4 to c9a33ea Compare July 17, 2016 16:41
@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch 2 times, most recently from f77d98c to d72c190 Compare July 18, 2016 16:18
@michaelkirk
Consistent and efficient media Delete/Copy/Save UX
9db3b0d 
copy/save/delete is accessed via longpress for all media messages, just
like for simple text messages.

Notes
-----
We don't support saving audio attachments as it's not clear where they should go.
(I don't think users expect them to end up in their iTunes library.)

There is still no UX for "pasting" media into Signal.

Removed the now redundant (and confusing) "share" button interface.

//FREEBIE
@michaelkirk michaelkirk force-pushed the fix-cannot-delete-media-messages#704 branch from d72c190 to 9db3b0d Compare July 18, 2016 16:20
@michaelkirk michaelkirk merged commit bd1e990 into signalapp:master Jul 18, 2016
@JohanDoe
Copy link

I am using Signal 2.4.0 from the app store. The share button has indeed disappeared. But I only see copy/delete using the edit menu via long-press on an image.

Is this intentional?

@michaelkirk
Copy link
Contributor Author

michaelkirk commented Jul 28, 2016 •
edited
Loading

@JohanDoe - That's not intentional, and sounds like a bug.

You should be able to save static images, animated GIFs, and video (but not audio or text). Can you file a new bug and include screenshots?

@JohanDoe
Copy link

@michaelkirk strange. Created a new conversation thread for the bug report screenshots and now everything is as it used to be. Can save images, also in old thread.

Can't reproduce right now. Will keep an eye on it and if I can reproduce I will submit a bug report.

@michaelkirk
Copy link
Contributor Author

👀

Thanks @JohanDoe!

This was referenced Sep 29, 2016
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michaelkirk @rocknjohn @pstasiak @greycubesgav @JohanDoe