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

Support new onAttach(Context) on Fragment's lifecycle #64

Merged
merged 3 commits into from
Nov 8, 2016

Conversation

NikoYuwono
Copy link
Contributor

Add onAttach(Context) on Fragment's lifecycle since onAttach(Activity) is deprecated since API 23

Reference : https://developer.android.com/reference/android/app/Fragment.html#onAttach(android.app.Activity)

Copy link
Contributor

@glung glung left a comment

Choose a reason for hiding this comment

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

I am not sure what it does not appear here but a test failed : com.soundcloud.lightcycle.LightCycleProcessorTest > shouldGenerateInjectorForParameterizeDispatcher FAILED

The fix is to add

    @Override
    public void onAttach(T fragment, Context context) { }

to lightcycle/lightcycle-processor/src/test/resources/com/soundcloud/lightcycle/FragmentLightCycleDispatcher.java

I know the setup is not ideal (and maybe reworked later(.

public interface FragmentLightCycle<T extends Fragment> {
void onAttach(T fragment, Activity activity);
@TargetApi(23) void onAttach(T fragment, Context context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you break the line after @TargetApi(23), please ?

Not your fault, we did not setup a proper code formatter and checker.
#65

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, Fixed!

@glung
Copy link
Contributor

glung commented Nov 8, 2016

Thank your for your contributions !

@NikoYuwono
Copy link
Contributor Author

NikoYuwono commented Nov 8, 2016

You are welcome! About the lightcycle-processor's test, I added the function per your request.
But every test did not pass in my machine, the error says file can't be found, I guess the test class can't found the file in resources correctly. Is there any special configuration for it?

The problem might be my environment though...

@glung
Copy link
Contributor

glung commented Nov 8, 2016

Unfortunately, they do not run in the IDE only in the command line (It used to work in AS, tho).

@michaelengland mentioned looking into it.

@glung
Copy link
Contributor

glung commented Nov 8, 2016

I think the tests dno not past because of lightcycle/lightcycle-processor/src/test/resources/com/soundcloud/lightcycle/FragmentLightCycleDispatcher.java (cf comment above)

@NikoYuwono
Copy link
Contributor Author

Sorry forgot to push!

I see, I think the other alternative is to rewrite the test to separate test per usage (Activity, Fragment, SupportFragment) and write the source directly in the test file.
I'll try to create pull request when I have the time!

@glung
Copy link
Contributor

glung commented Nov 8, 2016

Still does not build. The import is missing

you can run them with ./gradlew test

@NikoYuwono
Copy link
Contributor Author

Added the import for Context, and then the build was successful in my machine. Hope this can solve the problem.

@glung glung merged commit ff018b5 into soundcloud:master Nov 8, 2016
glung pushed a commit that referenced this pull request Nov 10, 2016
Fix the lightcycle-processor test to run in the IDE.

- Remove unnecessary header
- Inline Java source string constants

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

Successfully merging this pull request may close these issues.

3 participants