10BC0 Implement @ReceiverAction for issue #851 by dodgex · Pull Request #1092 · androidannotations/androidannotations · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Feb 26, 2023. It is now read-only.

Implement @ReceiverAction for issue #851 #1092

Merged
yDelouis merged 10 commits into from
Oct 3, 2014
Merged

Implement @ReceiverAction for issue #851 #1092

yDelouis merged 10 commits into from
Oct 3, 2014

Conversation

@dodgex
Copy link
8000 Member
@dodgex dodgex commented Aug 15, 2014

As i got some free time today, i decided to do another contribution to AA. :)

I implemented the @ReceiverAction annotation suggested in #851.

The code is based on the ServiceActionHandler.

This class

@EReceiver
public class MyReceiver extends BroadcastReceiver {

    @Override
    public void onReceive(Context context, Intent intent) {

    }

    @ReceiverAction("MyAction")
    public void doSomething() {
        // Your code ...
    }

    @ReceiverAction(DownloadManager.ACTION_DOWNLOAD_COMPLETE)
    public void onDownloadComplete(@ReceiverAction.Extra(DownloadManager.EXTRA_DOWNLOAD_ID) long extraDownloadId) {
        // Your code ...
    }

    @ReceiverAction("anotherAction")
    public void helloWorld(@ReceiverAction.Extra("hello_world") String heyWorldyWorld) {
        // Your code ...
    }
}

genereates the following code

public final class MyReceiver_
    extends MyReceiver
{

    public final static String ACTION_HELLO_WORLD = "anotherAction";
    private Context context_;
    public final static String HELLO_WORLD_EXTRA = "hello_world";
    public final static String ACTION_ON_DOWNLOAD_COMPLETE = "android.intent.action.DOWNLOAD_COMPLETE";
    public final static String EXTRA_DOWNLOAD_ID_EXTRA = "extra_download_id";
    public final static String ACTION_DO_SOMETHING = "MyAction";

    @Override
    public void onReceive(Context context, Intent intent) {
        context_ = context;
        init_();
        super.onReceive(context, intent);
        String action = intent.getAction();
        if (ACTION_HELLO_WORLD.equals(action)) {
            Bundle extras = intent.getExtras();
            if (extras!= null) {
                String hello_worldExtra = extras.getString(HELLO_WORLD_EXTRA);
                super.helloWorld(hello_worldExtra);
            }
            return ;
        }
        if (ACTION_ON_DOWNLOAD_COMPLETE.equals(action)) {
            Bundle extras = intent.getExtras();
            if (extras!= null) {
                long extra_download_idExtra = extras.getLong(EXTRA_DOWNLOAD_ID_EXTRA);
                super.onDownloadComplete(extra_download_idExtra);
            }
            return ;
        }
        if (ACTION_DO_SOMETHING.equals(action)) {
            super.doSomething();
            return ;
        }
    }

    private void init_() {
    }

}

@WonderCsabo
Copy link
Member
WonderCsabo commented Aug 15, 2014

I am confused. We already have @receiver for this purpose.

@dodgex
Copy link
Member Author
dodgex commented Aug 15, 2014

Yeah, but that is for receiver inside of annotations. This works also for receiver that are registred via Manifest

@dodgex
Copy link
Member Author
dodgex commented Aug 15, 2014

Argh annotations = acticities (or similar)

@WonderCsabo
Copy link
Member
WonderCsabo commented Aug 15, 2014

I see. @yDelouis WDYT?

@yDelouis
Copy link
Contributor
yDelouis commented Aug 17, 2014

The idea seems legit seems we added this feature to IntentServices.
Then, I agree it's confusing with @Receiver. We will have to be fully clear in the wiki.
About the code itself, I will review it later.

WonderCsabo
WonderCsabo reviewed Aug 22, 2014
View reviewed changes
...ns/functional-test-1-5/src/main/java/org/androidannotations/test15/ereceiver/MyReceiver.java Outdated
Copy link
Member
@WonderCsabo WonderCsabo Aug 22, 2014

Choose a reason for hiding this comment

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

@yDelouis will review your code later, until that i just have on note: i think this is not a functional test. In the functional test project, every annotation should have been tested to see it really does what we think it does. For example in this case we should broadcast an event and check that the annotated methods are indeed called. Also i think we should create another class for these. I know the @Receiver annotation by @yDelouis has similar "functional" test. Actually if we just want to be sure that an annotated code compiles, we should write a compile time test in the androidannotations/src/test folder.

Copy link
Member Author
@dodgex dodgex Aug 22, 2014

Choose a reason for hiding this comment

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

i can confirm that this is not a functional test. i added these methods but as the class (in functional-tests-1-5-tests) that should test this class is missing, i haven't added tests yet. actually it was planned but i forgot about it.

i'll come back to these asap.

@dodgex
Copy link
Member Author
dodgex commented Aug 23, 2014

tests added.

@WonderCsabo
Copy link
Member
WonderCsabo commented Aug 23, 2014

Some nitpick: you do not have to create a separate commits for each file change (but it can be a good thing, if it is really needed). 8000 Read some simple rules about grouping commits. Also do not use too long commit messages and add a detailed description if needed, like this. I will add this information to the contribution page.

@dodgex
Copy link
Member Author
dodgex commented Aug 23, 2014

@WonderCsabo not sure what exactly you want to tell me.
should i do more detailed (smaller) commits or bigger commits?

you do not have to create a separate commits for each file change

reads like "do not commit the files seperate. put them togehter"

Read some simple rules about grouping commits.

reads like "do as small commits as possible"

about the commit messages i think i understood. :)

thank you for your hints.

@WonderCsabo
Copy link
Member
WonderCsabo commented Aug 23, 2014

The main rule is: each commit should have contain one and only one logical change. For example if you add a method in a file, and use it in another file, you should do in the same commit (except if the method is big and/or will be used many other places). But you also should make your commits small. You have to find the balance. :) 6 commits for this feature is clearly too much.

@dodgex
Copy link
Member Author
dodgex commented Aug 23, 2014

okay.
so

  • new method in class x and used once: 1 commit
  • new method that is big and/or used in many other places: 2+ commits

makes sense so far.

i think we should find a better place for these talks. this is nothing that actually should be in a PR and/or a Issue. what about G+ Hangout?

@yDelouis
Copy link
Contributor
yDelouis commented Aug 23, 2014

i think we should find a better place for these talks. this is nothing that actually should be in a PR and/or a Issue. what about G+ Hangout?

I agree ;)

@dodgex
Copy link
Member Author
dodgex commented Aug 25, 2014

ModelProcessor.java#L169

any idea what to do with BroadcastReceiver here? as extending BroadcastReceiver requires to implement void onReceive(Context context, Intent intent) there has either to be an empty implementation or the class has to be abstract what results in no code being generated...

@dodgex
Copy link
Member Author
dodgex commented Aug 25, 2014

ok. missing feature: allow the Context being passed to the receiver action.

will do that tomorrow!

@WonderCsabo
Copy link
Member
WonderCsabo commented Aug 26, 2014

any idea what to do with BroadcastReceiver here?

We have a similar situation with @EIntentService, about the abstract onHandleIntent(Intent) method. In that case we require clients to declare an empty implementation. We can do the same here.

Actually we could traverse the class hierarchy and find the children of the abstract class, if it has no children we could generate the AA class for it even the class is abstract. But that would look a quiet weird... @yDelouis WDYT?

@yDelouis
Copy link
Contributor
yDelouis commented Aug 26, 2014

It seems complicated, the more so as with incremental builds because we have access to modified class only.
So we should do the same than for @EIntentService.

@dodgex dodgex mentioned this pull request Sep 8, 2014
Closed
@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 20, 2014

@yDelouis do you plan to review this?

@dodgex
Copy link
Member Author
dodgex commented Sep 20, 2014

rebased on current develop.

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 20, 2014

Something went wrong during the rebase, because there is a compilation error.

@dodgex
Copy link
Member Author
dodgex commented Sep 20, 2014

strange... somehow an import got lost. but it should be fixed now.

@dodgex dodgex mentioned this pull request Sep 22, 2014
Merged
@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 28, 2014

@yDelouis do you plan to review this?

@yDelouis
Copy link
Contributor
yDelouis commented Sep 29, 2014

I have started. I think there should be a handler for the annotation @ReceiverAction.Extra.
I want to test how to handle param annotations.
See this comment.

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 29, 2014

I know about that comment, that is way i opened #1169.

@yDelouis yDelouis merged commit bbed33f into androidannotations:develop Oct 3, 2014
yDelouis added a commit that referenced this pull request Oct 3, 2014
@yDelouis
Merge pull request #1092 from dodgex/ReceiverAction
c478f13 
Conflicts:
	AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/handler/InstanceStateHandler.java
yDelouis added a commit that referenced this pull request Oct 3, 2014
@yDelouis
Merge pull request #1092 from dodgex/ReceiverAction
2773a13 
Conflicts:
	AndroidAnnotations/androidannotations/src/main/java/org/androidannotations/handler/InstanceStateHandler.java
@yDelouis
Copy link
Contributor
yDelouis commented Oct 3, 2014

Thanks for your great work.
I completed this PR by adding ReceiverActionExtraHandler, adding a validation for ReceiverActionHandler and refactoring some code.

@yDelouis yDelouis added this to the 3.2 milestone Oct 3, 2014
@dodgex dodgex deleted the ReceiverAction branch October 3, 2014 15:46
@dodgex
Copy link
Member Author
dodgex commented Oct 3, 2014

I'm glad to see this one merged. :D no need for a custom build anymore.... for now ;)

@yDelouis
Copy link
Contributor
yDelouis commented Oct 3, 2014

I also changed a bit how the annotation works. Indeed, you can't let the user put any object in the method because some objects are not Serializable or Parcelable and the generated code won't compile.
So we only accept Context, Intent and parameters annotated with @ReceiverAction.Extra and the handler of @ReceiverAction.Extra will validate the fact that the paremeter is indeed "Bundle" compliant.

Could you please update the wiki page of @EReceiver for this and add a link in the available annotations page ?
You can get inspiration from the page EIntentService.

@dodgex
Copy link
Member Author
dodgex commented Oct 3, 2014

wiki updated. but while writing i asked myself if it makes sense to add dataSchemes to this annotation similar to @Receiver (#1096)

@yDelouis
Copy link
Contributor
yDelouis commented Oct 3, 2014

Please open another issue and explain what would be the generated code.

@dodgex
Copy link
Member Author
dodgex commented Oct 3, 2014

opened #1176

@WonderCsabo
Copy link
Member
WonderCsabo commented Oct 5, 2014

Thanks, wiki merged.

@dodgex dodgex mentioned this pull request Nov 13, 2014
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.2

Development

Successfully merging this pull request may close these issues.

3 participants

@dodgex @WonderCsabo @yDelouis
0