943 release openhelpermanager #1245

jiongxuan · 2014-11-16T15:52:57Z

Implement #943 issue.

Please review my code. Thx!

jiongxuan · 2014-11-16T16:12:12Z

I should only beautify my own code. Revert other beautify code.
Sorry about that!

The logic be Ok, not need to modify.

WonderCsabo · 2014-11-16T16:14:19Z

Please rebase this branch as to meld the last two commits into the first.

WonderCsabo · 2014-11-16T17:21:14Z

...tions/androidannotations/src/main/java/org/androidannotations/handler/OrmLiteDaoHandler.java

Unused variable.

WonderCsabo · 2014-11-16T17:51:47Z

Please do not reformat whole classes. We are using tabs for indenting etc. You can use our Eclipse formatter profile, if you save in Eclipse, it automatically formats the code with the proper settings.

WonderCsabo · 2014-11-16T17:55:37Z

BTW i just realized if the client uses daos which uses different helpers, OpenHelperManager will throw an IllegalStateException. I do not think we can validate this, but we should warn users in JavaDoc and wiki.

WonderCsabo · 2014-11-16T17:57:47Z

...tations/androidannotations/src/main/java/org/androidannotations/holder/EComponentHolder.java

Is it possible that the databaseHelper is null at this point?

Oftentimes, not.

But I think this is a good habit that can help us to write more strong code.
I'll change it

It is not a question of "habit". If the reference cannot be null, it is
absolutely unnecessary to use a null check. If the reference can be null,
you must add a null check. Actually you even get warnings if you use
@Nullable/@NotNull annotations and a static analysis tool which
inspects them.

WonderCsabo · 2014-11-16T20:22:24Z

And a small note about commit messages: please carefully write them, check grammar, typos etc. The commit messages should follow the standard git format (we refer to this in the contributing code page.

jiongxuan · 2014-11-17T06:17:43Z

Thanks for your advice!

I rebased some commit and push to AA. That's my first step.
Next, I'll based on your advices and rewrites new codes.

jiongxuan · 2014-11-17T08:29:51Z

From @WonderCsabo :
BTW i just realized if the client uses daos which uses different helpers, OpenHelperManager will throw an IllegalStateException. I do not think we can validate this, but we should warn users in JavaDoc and wiki.

I'm trying to learn English well. But, you know, in such a formal occasion, I still hope you help me write some text to warn users in JavaDoc and wiki.

Thx!

jiongxuan · 2014-11-17T08:53:23Z

From @WonderCsabo :
This is very ugly, and we do not need it. You can modify OrmLiteDaoHandler to use HasReceiverRegistration instead of EComponentHolder, just extend BaseAnnotationHandler<HasReceiverRegistration>. But in that case , you should rename HasReceiverRegistration to use a more general description.

I have been considering this issue long time before start writing the code.

But:
You should rename HasReceiverRegistration to use a more general description.

I'm not rashly to modify the overall framework.
Can you give me some advise to design?

WonderCsabo · 2014-11-17T09:23:45Z

Maye you can create a new interface HasLifecycleMethods which extends GeneratedClassHolder and move the getOnXXX methods there. Then let HasReceiverRegistration extend HasLifecycleMethods. Then you can use HasLifecycleMethods in OrmLiteDaoHandler.

WonderCsabo · 2014-11-17T09:29:24Z

Please rebase this whole branch into two commits: a code and a test commit.

WonderCsabo · 2014-11-17T09:34:11Z

I'm trying to learn English well. But, you know, in such a formal occasion, I still hope you help me write some text to warn users in JavaDoc and wiki.

No problem, that's actually another issue, i'll open a new ticket for it.

WonderCsabo · 2014-11-17T09:37:45Z

...tional-test-1-5/src/test/java/org/androidannotations/test15/ormlite/OrmLiteActivityTest.java

Since we do not actually use this test, please remove any changes from here.

jiongxuan · 2014-11-17T11:34:57Z

Push again.

Maye you can create a new interface HasLifecycleMethods which extends GeneratedClassHolder and move the getOnXXX methods there. Then let HasReceiverRegistration extend HasLifecycleMethods. Then you can use HasLifecycleMethods in OrmLiteDaoHandler.

I'll solute this issue tomorrow :)

WonderCsabo · 2014-11-17T11:37:41Z

...tions/androidannotations/src/main/java/org/androidannotations/handler/OrmLiteDaoHandler.java

Remove all changes from this class, if you do not add anything interesting.

jiongxuan · 2014-11-17T11:41:52Z

Rebase has some error on it. I'll check again.

Sorry about that.

WonderCsabo · 2014-11-17T11:52:42Z

...ional-test-1-5/src/test/java/org/androidannotations/test15/ormlite/OrmLiteOnDestroyTest.java

Use staticField for static fields.

jiongxuan · 2014-11-17T14:50:39Z

I'm terribly sorry. I have no idea to avoid the "instanceof" condition.

Because the OrmliteDaoHandler can handle two scenarios: with an onDestory method or not.
We cannot create getOnDestroy into HasLifecycleMethods, thus will be ignore annother scenario.

Can u give me some advices? :)

WonderCsabo · 2014-11-17T15:06:07Z

Yep, this is a tricky one. You can create the similar solution to the receiver registration. Move the release helper method invocation to a separate class. Then in EActivityHolder, EFragmentHolder and EServiceHolder, override the setDatabaseHelperRef, which wich will call super and also delegates to the newly created class which will do the releaseHelper invocation.

yDelouis · 2014-11-17T15:41:38Z

...tations/androidannotations/src/main/java/org/androidannotations/holder/EComponentHolder.java

I think you wanted to write this instanceof HasLifecycleMethods, right ?

No, the logic is OK. But we should refactor this.

jiongxuan · 2014-11-18T01:57:01Z

Yep, this is a tricky one. You can create the similar solution to the receiver registration. Move the release helper method invocation to a separate class. Then in EActivityHolder, EFragmentHolder and EServiceHolder, override the setDatabaseHelperRef, which wich will call super and also delegates to the newly created class which will do the releaseHelper invocation.

Sounds exciting!
I see your point. Please review my code.

I'm sorry to trouble you a lot.

WonderCsabo · 2014-11-18T08:02:23Z

...ons/androidannotations/src/main/java/org/androidannotations/holder/EIntentServiceHolder.java

You should not do anything here, since it is already done in EServiceHolder. This way you will add two releaseHelper invocations.

Yep.

I'll create new test classes for Service and Fragment to ensure there is no problem.

You can leave that as is, the current test is enough for now.
On Nov 18, 2014 9:21 AM, "Jiongxuan Zhang" notifications@github.com wrote:

In
AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/holder/EIntentServiceHolder.java:

@@ -76,4 +79,12 @@ private void createOnHandleIntent() {
JInvocation getActionInvocation = JExpr.invoke(onHandleIntentIntent, "getAction");
onHandleIntentIntentAction = onHandleIntentBody.decl(classes().STRING, "action", getActionInvocation);
}
+

@OverRide

protected JFieldVar setDatabaseHelperRef(TypeMirror databaseHelperTypeMirror) {

Yep.

I'll create new test classes for Service and Fragment to ensure there is
no problem.

Reply to this email directly or view it on GitHub
https://github.com/excilys/androidannotations/pull/1245/files#r20491186.

I'm almost finished writing it :)

Okay, I'm fixed it.

WonderCsabo · 2014-11-18T08:46:42Z

...nnotations/androidannotations/src/main/java/org/androidannotations/helper/OrmLiteHelper.java

I still think it is not really nice if we use HasReceiverRegistration, because this code has nothing to do with receivers. I suggest to reintroduce the interface that i mentioned, and use it there instead of HasReceiverRegistration.

HasLifecycleMethods?

Are you sure that the interface should have onDestroyBeforeSuperBlock method?
If not, how can I insert OpenHelperManager.release in the onDestroy method?

Do you think that HasLifecycleMethods should not have onDestroyBeforeSuperBlock? Why?

Because the OrmliteDaoHandler can handle two scenarios: with an onDestory method or not.
We cannot create getOnDestroy into HasLifecycleMethods, thus will be ignore annother scenario.

@EBean and @EActivity can use it well. But @EBean does not have a onDestroy method

You mean we should only add onDestroyXXX in the HasLifecycleMethods?
So that it conforms to the ISP? :)

Don't get me wrong, I just confirm that I understand

OK, it would be seen as redundant to you, because basically everyting which is a HasLifecycleMethods is HasReceiverRegistration, too. But i still think this is better design. This is what i propose:

create a new HasLifecycleMethods interface:

public interface HasLifecycleMethods extends GeneratedClassHolder { JBlock getOnCreateAfterSuperBlock(); JBlock getOnDestroyBeforeSuperBlock(); JBlock getOnStartAfterSuperBlock(); JBlock getOnStopBeforeSuperBlock(); JBlock getOnResumeAfterSuperBlock(); JBlock getOnPauseBeforeSuperBlock(); JBlock getOnAttachAfterSuperBlock(); JBlock getOnDetachBeforeSuperBlock(); }

Let HasReceiverRegistration extend HasLifecycleMethods, and remove redundant method declarations:

public interface HasReceiverRegistration extends HasLifecycleMethods { JExpression getContextRef(); JFieldVar getIntentFilterField(String[] actions, String[] dataSchemas); }

Use HasLifecycleMethods in OrmLiteHelper:

public static void injectReleaseInDestroy(JFieldVar databaseHelperRef, HasLifecycleMethods lifecycleMethodsHolder, ProcessHolder.Classes classes) { ... }

Yep. You're right!

In fact, the most perfect solution i think is split into four interface: HasLifecycleMethods / HasStartAndStopMethods / HasAttachableMethods / HasReceiverRegistration.
There is no connection between them.

EActivityHolder and EFragmentHolder need implement: HasLifecycleMethods,HasStartAndStopMethods, HasAttachableMethods and HasReceiverRegistration
EServiceHolder need implement: HasLifecycleMethods and HasReceiverRegistration

In ReceiverHandler.registerAndUnregisterReceiver method, we can add two method with the same name.
These methods are respectively corresponding to these Interface.

However, this is a huge change. I think to do as you say.

Yes, we could split it even further, but i think my proposal will suffice
for now.

Remove redundant method declarations in HasReceiverRegistration class.

WonderCsabo · 2014-11-19T10:47:13Z

@yDelouis I think this code is finally OK. I would merge.

dodgex · 2014-11-19T12:27:21Z

LGTM. Nice work. :)

jiongxuan · 2014-11-19T12:42:25Z

Thanks. :)

Release database helper with @OrmLiteDao

WonderCsabo · 2014-11-20T19:25:02Z

Thanks!

WonderCsabo reviewed Nov 16, 2014
View reviewed changes

...tions/androidannotations/src/main/java/org/androidannotations/handler/OrmLiteDaoHandler.java Outdated

Copy link

Member

WonderCsabo Nov 16, 2014

Choose a reason for hiding this comment

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

Unused variable.

WonderCsabo reviewed Nov 16, 2014
View reviewed changes

jiongxuan force-pushed the 943_release_openhelpermanager branch from 0f3b459 to 9d8e982 Compare November 17, 2014 06:08

jiongxuan force-pushed the 943_release_openhelpermanager branch from f16c550 to 50868a8 Compare November 17, 2014 08:37

WonderCsabo reviewed Nov 17, 2014
View reviewed changes

jiongxuan force-pushed the 943_release_openhelpermanager branch from 50868a8 to cd34694 Compare November 17, 2014 11:32

WonderCsabo reviewed Nov 17, 2014
View reviewed changes

jiongxuan force-pushed the 943_release_openhelpermanager branch from cd34694 to 3c197d3 Compare November 17, 2014 11:51

WonderCsabo reviewed Nov 17, 2014
View reviewed changes

jiongxuan force-pushed the 943_release_openhelpermanager branch from 3c197d3 to dcee2d3 Compare November 17, 2014 12:50

Add tests for release OpenHelperManager in onDestroy with Ormlite

ecfa454

jiongxuan force-pushed the 943_release_openhelpermanager branch 2 times, most recently from 2e009cc to d5012ee Compare November 17, 2014 14:45

yDelouis reviewed Nov 17, 2014
View reviewed changes

jiongxuan force-pushed the 943_release_openhelpermanager branch from d5012ee to 701606e Compare November 18, 2014 01:55

WonderCsabo reviewed Nov 18, 2014
View reviewed changes

Release OpenHelperManager in onDestroy with Ormlite

1d0f891

jiongxuan force-pushed the 943_release_openhelpermanager branch from 701606e to 1d0f891 Compare November 18, 2014 08:36

WonderCsabo reviewed Nov 18, 2014
View reviewed changes

Add HasLifecycleMethods interface

ee9e1a8

Remove redundant method declarations in HasReceiverRegistration class.

WonderCsabo added a commit that referenced this pull request Nov 20, 2014

Merge pull request #1245 from jiongxuan/943_release_openhelpermanager

1dc834d

Release database helper with @OrmLiteDao

WonderCsabo merged commit 1dc834d into androidannotations:develop Nov 20, 2014

WonderCsabo added this to the 3.3 milestone Nov 20, 2014

943 release openhelpermanager #1245

943 release openhelpermanager #1245

Uh oh!

Conversation

jiongxuan commented Nov 16, 2014

Uh oh!

jiongxuan commented Nov 16, 2014

Uh oh!

WonderCsabo commented Nov 16, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WonderCsabo commented Nov 16, 2014

Uh oh!

WonderCsabo commented Nov 16, 2014

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!

WonderCsabo commented Nov 16, 2014

Uh oh!

jiongxuan commented Nov 17, 2014

Uh oh!

jiongxuan commented Nov 17, 2014

Uh oh!

jiongxuan commented Nov 17, 2014

Uh oh!

WonderCsabo commented Nov 17, 2014

Uh oh!

WonderCsabo commented Nov 17, 2014

Uh oh!

WonderCsabo commented Nov 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiongxuan commented Nov 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiongxuan commented Nov 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiongxuan commented Nov 17, 2014

Uh oh!

WonderCsabo commented Nov 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiongxuan commented Nov 18, 2014

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!