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

Email sender - why not support attaching files #558

Closed
MFlisar opened this issue Mar 9, 2017 · 21 comments
Closed

Email sender - why not support attaching files #558

MFlisar opened this issue Mar 9, 2017 · 21 comments

Comments

@MFlisar
Copy link

MFlisar commented Mar 9, 2017

Any reason for that?

I've written an EmailFileIntentSender that extracts data from custom fields and uses them (currently it extracts the file provider name for the files + file names that should be attached). It would be nice to add some official setup fields for this setup to acra and add this sender to the library.

Would you be interested in something like that?

@F43nd1r
Copy link
Member

F43nd1r commented Mar 9, 2017

I generally support the idea of additional binary attachments, as it is the most requested feature as of now. However I think it should be supported by all integrated senders, not only emails. I like the idea of using a file provider. Any thoughts @william-ferguson-au ?

@MFlisar
Copy link
Author

MFlisar commented Mar 9, 2017

You don't have a chance to share files reliably without a file provider, so that's a must. It can be reused for all senders as well of course without additional code nor setup...

@F43nd1r
Copy link
Member

F43nd1r commented Mar 9, 2017

@MFlisar can I view your sender/provider-setup somewhere?

@MFlisar
Copy link
Author

MFlisar commented Mar 9, 2017

I've currently most functionality in my own classes, but I would add a pull request if this feature would be accepted... Therefore I would have to rewrite it a little bit...

I use this functionality already for quite some time in a feedback utility class I've written to send feedback mails with a few files appended...

@MFlisar
Copy link
Author

MFlisar commented Mar 9, 2017

The base cache provider is here: https://gist.github.com/MFlisar/fc8689ec47e6adef86d120884a7fa7db It's very simple and basic...

Implementation looks like following:

public class CachedFileProvider extends ...CachedFileProvider {

	public static String AUTHORITY = BuildConfig.APPLICATION_ID + ".CachedFileProvider";

	@Override
	protected String getAuthority() {
		return AUTHORITY;
	}
}

This provider is added to the manifest and from then on it works. The sender then simple creates an email intent and addes content uris of the cached file provider to it and sends the mail...

@william-ferguson-au
Copy link
Member

As long as it is kept simple and doesn't place requirements on those that don't need attachments, sure.

@F43nd1r
Copy link
Member

F43nd1r commented Mar 10, 2017

My current draft for this is here https://github.com/F43nd1r/acra/tree/file
Email attachments are working, but I'm currently struggling to determine a good way to configure where files should be sent in case of a httpsender

@F43nd1r
Copy link
Member

F43nd1r commented Mar 11, 2017

@MFlisar I'd appreciate it if you tried to use #560 for your usecase.

@MFlisar
Copy link
Author

MFlisar commented Mar 13, 2017

@F43nd1r

I've checked your code a little bit. Is following correct:

  • I must provide URIs, I can't provide paths
  • I must provide my own provider

For my use case I want following:

  • an empty mail (with a custom text defineable body maybe)
  • an attachment with all ACRA content (I don't want the buildBody() content of the mail intent sender in my email but in an attachment)
  • a custom file attachment, in my use case, a file must be copyied from a defined path to the cache folder when a crash happens (my custom log file)

I personally just think an empty mail with attachments is much more user friendly, I personally don't want to fill out my feedback in a dialog but want to write it in a mail and therefore, all extra infos (all technical infos, which is not interesting for most people) should not be visible in the mail, it's just distracting...

I think, those things could be combined with your implementation by adding an acra field like .setAttachAcraData(boolean) or similar to attach the content as a file instead of writing it as content and something like .setPrepareCrashReport(IPrepare) or similar where I can pass in a custom interface implementation in which I can prepare data (like writing and copying files to the cache directory for example so that the files that are mapped to the URIs are updated in the cache directory).

@F43nd1r
Copy link
Member

F43nd1r commented Mar 13, 2017

I must provide URIs, I can't provide paths

You can provide file uris

I must provide my own provider

The UriProvider is optional, it can be used if you want to provide Uris at runtime instead of compile time. A ContentProvider is also optional, as e.g. file uris are resolved by the android system.

Moving the report to an attachment is a different change which can be built upon this one, but it is not an integral part of this. I'll start with that as soon as this is stable.

@MFlisar
Copy link
Author

MFlisar commented Mar 25, 2017

Just one comment: file uris may be resolved by the android system, but permissions will be missing, so you must provide a ContentProvider. I.e. sharing a file from your cache directory to gmail will fail, if you share the file uri only...

@MFlisar
Copy link
Author

MFlisar commented Mar 25, 2017

Still 2 thoughts:

  • have you thought about that the AUTHORITY must be unique? Consider two apps using acra on the same device. This will result in install exceptions...
  • does your content provider allow access to ANY file the app can access?

@F43nd1r
Copy link
Member

F43nd1r commented Mar 25, 2017

does your content provider allow access to ANY file the app can access?

It is able to, but it is only accesible with uri permissions, which aren't granted for any files but the ones configured.

have you thought about that the AUTHORITY must be unique? Consider two apps using acra on the same device. This will result in install exceptions...

No, good point.

@MFlisar
Copy link
Author

MFlisar commented Mar 25, 2017

OK

Here's my example of a content provider:

 <provider
        android:name="CachedFileProvider"
        android:grantUriPermissions="true"
        android:authorities="${applicationId}.CachedFileProvider" />

And following:

 public class CachedFileProvider {

	@Override
	protected String getAuthority() {
		return BuildConfig.APPLICATION_ID + ".CachedFileProvider";
	}
}

This one is reusable...

@F43nd1r
Copy link
Member

F43nd1r commented Mar 25, 2017

BuildConfig.APPLICATION_ID cannot be used, as the buildconfig class is unknown. However, context.getPackageName provides the same value.

@MFlisar
Copy link
Author

MFlisar commented Mar 25, 2017

OK, that's why I always sub classed my base class I posted on top (didn't remember that). You're right, package name should do it as well

@MFlisar
Copy link
Author

MFlisar commented Apr 13, 2017

I now tested this and it works fine. But it's not what I personally want.

Could we extend that to support following:

  • append a text file with the stacktrace
  • append another text file with the custom log file
  • append a third text file with all acra fields

This could be done via a setup flag or similar...

@F43nd1r
Copy link
Member

F43nd1r commented Apr 13, 2017

@MFlisar I think this is not in the scope of the default sender, and should be implemented in a custom sender. However, I've improved the extensibility of the default email sender, so you should be able to reuse most of the code. You'll have to override fillAttachmentList. See #577

As for the custom log file, just specify it as attachment instead of as custom log file.

@MFlisar
Copy link
Author

MFlisar commented Apr 14, 2017

Thanks, the new sender is really better.

One small thing though: imho, it would make sense to have a function to convert a path to a valid AcraContentProvider uri:

 Uri uri = Uri.parse("content://" + getPackageName() + ".acra/root" + path);

This way I can easily add my log file as attachment via setAttachmentUris...

@F43nd1r
Copy link
Member

F43nd1r commented Apr 14, 2017

Added that in #577

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants