8000 Add @Result annotation for @OnActivityResult by simonz · Pull Request #1058 · 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.

Add @Result annotation for @OnActivityResult #1058

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

Add @Result annotation for @OnActivityResult #1058

yDelouis merged 3 commits into from
Oct 4, 2014

Conversation

@simonz
Copy link
Contributor
@simonz simonz commented Jun 23, 2014

This PR is implementing #796 which I really wanted to be implemented
@Result is added to retrieve the values from intent

    Bundle extras_ = (((data == null)||(data.getExtras() == null))?new Bundle():data.getExtras());
    switch (requestCode) {
        case  44 :
            {
                int value_ = extras_.getInt("value");
                String s_ = extras_.getString("s");
                Uri uri_ = extras_.getParcelable("uri");
                ArrayList<Uri> uris_ = extras_.getParcelableArrayList("uris");
                String[] strings_ = extras_.getStringArray("strings");
                AwaitingResultActivity_.this.onResultWithResultExtra(resultCode, value_, s_, uri_, uris_, strings_);
            }
            break;
    ......

this is a snippet from a generated code.
extras_ was newly declared and one block was added for variable's scope

yDelouis
yDelouis reviewed Aug 18, 2014
View reviewed changes
...otations/androidannotations-api/src/main/java/org/androidannotations/annotations/Result.java Outdated
Copy link
Contributor
@yDelouis yDelouis Aug 18, 2014

Choose a reason for hiding this comment

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

Please, add some javadoc on the annotation. See existing annotations to have some examples.

@yDelouis
Copy link
Contributor
yDelouis commented Aug 18, 2014

You must create a class ResultHandler to validate the annotation (check that the annotation is used at the right place).
In this class, you could also add a static method to generate the expression to extract the value from the intent (see my comment on code). It will help us if we use this annotation somewhere else.
The name of the generated variable (value_ for example) should be the name of the variable, not the key in the bundle. If you leave thing like this, you will have an error with the code :

@OnActivityResult
public void onOneResult(@Result int value, @Result("value") String str) {
...
}

@simonz
Copy link
Contributor Author
simonz commented Sep 17, 2014

Thank you for your advice. I added some comments and created ResultHandler
But I don't know how to validate for @Result.
Because it's the first annotation of AA for a paramenter, I have no idea of proper validation

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 17, 2014

Ideally you should create a subclass of BaseAnnotationHandler, and register it in the AnnotationHandlers class. Then you could validate the usage of the annotation in the validate() method and process it in the process(), totally separated from the OnActivityResultHandler. But it has at least one problem: it seems ModelExtractor does not extract annotations on method parameters, only annotations on fields and methods themselves. @yDelouis WDYT, should we modify this behavior?

@dodgex
Copy link
Member
dodgex commented Sep 22, 2014

@WonderCsabo @yDelouis while i understand the fact that one handler should only handle one annotation, i wonder if there are not some cases, where it would make sense to handle more.

this case here is imho the same situation as in my @ReceiverAction PR. the values/parameters are directly related to the actual annotation that has to be handled, and it has an direct impact on how the generated code should look.

in such a case, i would not add the @Result as a standalone annotation but as i did an inner class/annotation. this way it should be clearly visible "hey i'm for @OnActivityResult and for @OnActivityResult only!" and it should be easier to handle both annotations to generate correct code.

just my 2¢

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 22, 2014

Yeah, you have a point, although i do not really think ReceiverAction is
the same situation.
I still think we should separate the handlers if it is possible in a clean
way, otherwise we find ourselves in a mess like with the REST annotations.
But in this case we might have an exception, since this is the first param
annotation and it is not handled in the extractor.

@dodgex
Copy link
Member
dodgex commented Sep 22, 2014

since this is the first param annotation

ReceiverAction has a param annotation too. it is used to map extras like DownloadManager.EXTRA_DOWNLOAD_ID (sure you could use the actual string value extra_download_id of this constant but that should not be the way to go) to a paremter.

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 22, 2014

Yeah, i just found that, i did not notice when i opened the diff on my mobile phone. You are right, this is the same case, but i am not sure @yDelouis will not tell you the same when he will review your PR. :)

I think this is an important design decision, @yD 8000 elouis, @DayS WDYT?

@yDelouis
Copy link
Contributor
yDelouis commented Sep 22, 2014

This is indeed an important decision because it is the first time we deal with a param annotation and it will happen more and more.

  • I think that we should keep creating one handler for one annotation because it's easier to understand and the change would have an impact too important on the architecture.
  • I also think @dodgex did the right way creating a child annotation for @ReceiverAction.Extra. This way, it's clear that the annotation @ReceiverAction.Extra can be used only with @ReceiverAction.
  • As we have to validate and process this annotation too, we have to create an handler for it. It could be a standalone class or an inner class of ReceiverActionHandler (I vote for the inner class). It has to be registered to AnnotationHandlers separately.
    We probably will have to create a parent class for handlers handling a param annotation.

@WonderCsabo
Copy link
Member
WonderCsabo commented Sep 22, 2014

I agree, we should prepare the framework to handle param annotations. We should create a new parent class for these kind of handlers. Also we have to modify ModelExtractor to include these annotations, so the handler is indeed called.

@WonderCsabo WonderCsabo mentioned this pull request Sep 28, 2014
Closed
@yDelouis yDelouis mentioned this pull request Sep 29, 2014
Merged
@yDelouis yDelouis merged commit 0dd1eae into androidannotations:develop Oct 4, 2014
@yDelouis yDelouis added this to the 3.2 milestone Oct 4, 2014
@yDelouis
Copy link
Contributor
yDelouis commented Oct 4, 2014

Thanks.
I moved @Result to @OnActivityResult.Extra to be consistent with @ReceiverAction.Extra.
I also added validation and did some refactorization.

We have to update the wiki (list of all annotations and onActivityResult page) about that. @simonz, could you clone the wiki and update it in your fork so that we will be able to pull it ?

@WonderCsabo
Copy link
Member
WonderCsabo commented Oct 6, 2014

I added the doc.

@dodgex dodgex mentioned this pull request Oct 21, 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.

4 participants

@simonz @yDelouis @WonderCsabo @dodgex
0