Write to static field ACRAConfiguration.mReportsCrashes from instance method new ACRAConfiguration(ReportsCrashes)

[GoogleCodeExporter]

In ACRAConfiguration:

    public ACRAConfiguration(ReportsCrashes defaults) {
        mReportsCrashes = defaults;
    }

This instance method writes to a static field. This is tricky to get correct if 
multiple instances are being manipulated, and generally bad practice.

I am using the code from r775

Original issue reported on code.google.com by marcoleb...@gmail.com on 12 Feb 2012 at 3:09

[GoogleCodeExporter]

That's right. There's something wrong in the life cycle of this object. I'm 
working on this.

Thanks for reporting.

Kevin

Original comment by kevin.gaudin on 24 Jul 2012 at 1:26

• Changed state: Accepted

[GoogleCodeExporter]

Do you need to manipulate multiple instances of ACRAConfiguration ? I was 
fixing this by changing all getter methods to static (all setters and member 
fields were already static so there was no possible way of handling multiple 
instances of this object containing different values).

Original comment by kevin.gaudin on 24 Jul 2012 at 1:39

[GoogleCodeExporter]

Well I set my mind to a non-static ACRAConfiguration object.
There will be those methods:
ACRA.getConfig() = retrieve the current configuration instance
ACRAConfiguration ACRA.getNewDefaultConfig() = create a new configuration 
instance initialized with the values set in the ReportsCrashes annotation
ACRA.setConfig(ACRAConfiguration) = change the whole ACRA configuration to 
another instance.

This way, we can set individual configuration item values with 
ACRA.getConfig().setXXXX(), and also create various predefined configuration 
instances and set them when desired using a simple 
ACRA.setConfig(myOtherConfig).

I modified the trunk but did not test it at all for the moment...

Original comment by kevin.gaudin on 24 Jul 2012 at 2:05

[GoogleCodeExporter]

I am probably missing something, but the current implementation seems to be 
confusing. If we call ACRA.getConfig() before ACRA.init(), mReportCrashes is 
null and we get essentially an empty config. We then use setXXX() to set some 
options, but not formKey, etc. (because they are in set in the annotation) and 
call ACRA.init(). This doesn't try to 'merge' the attributes from the 
annotation and because formKey is still null, no reporter senders are added 
(and then I spend an afternoon trying to figure out what's wrong :).

If, on the other hand, we call ACRA.init() before ACRA.getConfig() we may get 
an exception because, for example, resToastText is not set (and we cannot set 
it in the annotation, because this is a library project). 

So, either the order should be strictly defined, or ACRA.init() should try to 
'merge 'the annotation attributes into the config (which one has higher 
priority then?). 

The workaround would be to set the formKey using ACRA.getConfig().setFormKey(), 
but we can't really do this from a subclass without manually getting the 
annotation with something like this: 

ReportsCrashes rc = getClass().getAnnotation(ReportsCrashes.class);
ACRA.getConfig().setFormKey(rc.formKey());
ACRA.getConfig().setResToastText(R.string.crash_toast_text);
ACRA.getConfig().setMode(rc.mode());
ACRA.getConfig().setSendReportsInDevMode(true);
ACRA.init(this);

Original comment by nikolay....@gmail.com on 29 Aug 2012 at 7:11

[GoogleCodeExporter]

What would you think about the following changes:
- adding a method : ACRA.getNewDefaultConfigBeforeInit(Application app) which 
would return a new ACRAConfiguration instance initialized with values set in 
the ReportsCrashes annotation associated to the Application app.
- renaming ACRA.getNewDefaultConfig() to ACRA.getNewDefaultConfigAfterInit() 
because its current implementation requires init() to be called if you want it 
to be initialized with annotation values.

OR

- We always provide the Application class to 
ACRA.getNewDefaultConfig(Application)

I prefer the aesthetics of a single method. Basically we would always have to 
call ACRA.getNewDefaultConfig(this) when we need to create a new configuration 
instance with the annotation default values:
- just before calling ACRA.init()
- anytime after init in order to create a second config instance

I also have to think about when calling again ACRA.init() would be necessary 
after changing configuration properties, as it is not supported for the moment. 
I am figuring out for example that changing the formKey/formUri has no effects 
because we create the report sender in init() with the formKey/formUri as a 
constructor parameter.

Original comment by kevin.gaudin on 29 Aug 2012 at 9:39

• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Here is what I just pushed on the trunk:
  * Various changes due to discussion on Issue 111:
    * Creation of a setDefaultReportSenders() method on ErrorReporter. This is the method which is called by ACRA.init() to instanciate the relevant default ReportSenders depending on the provision of mailTo, formUri and formKey parameters.
      The method should now be called whenever you wish to change the configuration by switching from one of these 3 parameter to the others.
    * HTTPPostSender and GoogleFormSender do not take their formURI/formKey as a constructor parameter anymore. They get the value from ACRA.getConfig() instead. If their parameters are changed programmatically, they don't need to be reinstanciated.
    * Addition of a warning message if ACRA.getConfig() is called before ACRA.init(). ACRA.getNewDefaultConfig() should be used instead.
    * ACRA.getNewDefaultConfig() now requires an Application as a parameter. This allows to create a new ACRAConfiguration instance with @ReportsCrashes defined values BEFORE calling ACRA.init()
  * added: new configuration item googleFormUrlFormat to allow, if Google Docs/Drive services evolve, to modify the Form URL structure without releasing a new ACRA version in emergency.

Original comment by kevin.gaudin on 29 Aug 2012 at 11:10

[GoogleCodeExporter]

This mostly looks good, but it makes it harder to have more than one report 
sender (for example I post both to GDocs and Bugsense), since you can no longer 
do 

ACRA.getErrorReporter().addReportSender(new HttpPostSender(otherUrl, null));

There should be some way to have senders that don't depend on the default 
config. Maybe recconfiguring senders manually (by calling a 
setDefaultReportSenders(), etc. )is not such a bad idea, it only needs to 
happen occasionally. 

Original comment by nikolay....@gmail.com on 30 Aug 2012 at 1:48

[GoogleCodeExporter]

BTW, GDocs forms seem to now live at https://docs.google.com/spreadsheet, not 
sure if it is worth changing the default (the old one still works). 

Original comment by nikolay....@gmail.com on 30 Aug 2012 at 1:49

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

It is actually impossible to use two senders right now, because once it finds a 
fromUri, the fromKey is ignored: 

in ErrorReporter.setDefaultReportSenders():

        if (conf.formUri() != null && !"".equals(conf.formUri())) {
            setReportSender(new HttpPostSender(null));
            return;
        }

So maybe keep the constructor parameter and only use the global config if not 
explicitly set?

Original comment by nikolay....@gmail.com on 30 Aug 2012 at 2:20

[GoogleCodeExporter]

Something like this maybe? (see attachments)

Original comment by nikolay....@gmail.com on 30 Aug 2012 at 2:47

Attachments:

• GoogleFormSender.patch
• HttpPostSender.patch

[GoogleCodeExporter]

That definitely makes sense, I just committed your patches with additional 
javadoc update.

If everything looks ok for you I'll publish a release candidate with 1 more 
week before final release.

Thanks a lot for your time Nikolay!

Original comment by kevin.gaudin on 30 Aug 2012 at 6:11

[GoogleCodeExporter]

Looks good and works for me, but definitely more people need to use it, so go 
ahead with the RC. 

Original comment by nikolay....@gmail.com on 30 Aug 2012 at 6:30

[GoogleCodeExporter]

Good

Original comment by humaira....@gmail.com on 5 Oct 2012 at 12:34

[GoogleCodeExporter]

Original comment by kevin.gaudin on 15 Oct 2012 at 9:25

• Changed state: Verified
• Added labels: Milestone-Release4.3.0