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.

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

Large attachments #4019

Closed

Conversation

mcginty
Copy link
Contributor

@mcginty mcginty commented Sep 5, 2015

This change includes asynchronously resolving slide attachment objects since we need to calculate the total size of the media as well as alerting the user about size issues before the media is attempted to be sent.

It's a net loss diff since there was a good amount of redundant code that was easy to simplify in the process.

Audio and Video sizes are raised to 100MB, gif raised to 5MB.

@mcginty mcginty force-pushed the feature/big-push-attachment-limits branch from 452dd4f to ca81649 Compare September 5, 2015 00:58
@mcginty mcginty changed the title Feature/big push attachment limits Large attachments Sep 5, 2015
@mcginty mcginty force-pushed the feature/big-push-attachment-limits branch from ca81649 to 7ee3c69 Compare September 5, 2015 01:02
: MediaConstraints.MMS_CONSTRAINTS;

if (!constraints.isSatisfied(this, masterSecret, slide.getPart()) && !constraints.canResize(slide.getPart())) {
Toast.makeText(this, R.string.ConversationActivity_attachment_exceeds_size_limits, Toast.LENGTH_LONG).show();
Copy link
Contributor

Choose a reason for hiding this comment

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

very strange side effect for this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i was being lazy, fixed

@mcginty
Copy link
Contributor Author

mcginty commented Sep 5, 2015

comments addressed, updated

@mcginty mcginty force-pushed the feature/big-push-attachment-limits branch from a13b45f to 22b69a8 Compare September 5, 2015 01:22
Toast.makeText(context,
R.string.ConversationActivity_sorry_there_was_an_error_setting_your_attachment,
Toast.LENGTH_SHORT).show();
} else if (attachmentListener.isAttachmentAllowed(slide)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

listeners should be one way, not bidirectional

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@mcginty mcginty force-pushed the feature/big-push-attachment-limits branch from 22b69a8 to eaadb1d Compare September 5, 2015 01:25
long size = 0;
byte[] buffer = new byte[512];
int read;
protected static long getMediaSize(Context context, MasterSecret masterSecret, Uri uri) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love that this method way down here has the side effect of doing potentially long running I/O. Also, why protected? Might be better to make the caller pass in the size, rather than having it happen implicitly down here?

@richo
Copy link
Contributor

richo commented Sep 5, 2015

Updates #700 (Pinging for anyone else who tries the same stunt I did)

@mcginty
Copy link
Contributor Author

mcginty commented Sep 7, 2015

Media size is calculated and passed into Slide constructor, so the constructor itself is no longer a blocking operation. This also was nice in that it got rid of a couple of objects holding onto the MasterSecret.

Log.w("ComposeMessageActivity", e);
}
private void setAttachment(Uri uri, MediaType mediaType) {
setMedia(uri, mediaType, false);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we need separate methods for these

@mcginty
Copy link
Contributor Author

mcginty commented Sep 8, 2015

feedback addressed, unspited variables

@mcginty mcginty force-pushed the feature/big-push-attachment-limits branch from 8bfe323 to 49ed4d0 Compare September 8, 2015 01:41
mcginty added a commit that referenced this pull request Sep 8, 2015
@moxie0
Copy link
Contributor

moxie0 commented Sep 8, 2015

in 2.27.0

@moxie0 moxie0 closed this Sep 8, 2015
mcginty added a commit that referenced this pull request Sep 17, 2015
BLeQuerrec pushed a commit to SilenceIM/Silence that referenced this pull request Sep 21, 2015
BLeQuerrec pushed a commit to SilenceIM/Silence that referenced this pull request Oct 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants