Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

PT-422: fix system animation warnings #119

Merged
merged 6 commits into from
Jan 31, 2017

Conversation

tasomaniac
Copy link
Contributor

@tasomaniac tasomaniac commented Jan 30, 2017

Problem

In a previous PR #107, we decoupled the extension from the tasks.

But to keep backward compatible, we kept accessing the extension from the tasks as last resort, and we have a big warning if such case occurs.

Then, we forgot to update our own systemAnimation tasks. Since it uses 6 adb command, this 4 line warning was printed 6 times on each run.

Solution

This PR is fixing that by attaching adb and other fields to all AdbTask tasks by finding them later in the plugin. Using this method project.tasks.withType(AdbTask).

Note: this new attachDefaults(AdbTask) method can also be used by the users to prevent this warning now. Should we update the sample's and README with this info?

@@ -109,7 +109,7 @@ android {
}
}

task listDevices << {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also updated because Gradle had a warning saying that the leftShift operator on Tasks API is deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -13,7 +13,7 @@ public class AndroidCommandPlugin implements Plugin<Project> {
throw new StopExecutionException("The 'android' plugin is required.")
}

def extension = project.android.extensions.create("command", AndroidCommandPluginExtension, project)
AndroidCommandPluginExtension extension = project.android.extensions.create("command", AndroidCommandPluginExtension, project)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use single quotes while we're at it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure ddb3a80

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -109,7 +109,7 @@ android {
}
}

task listDevices << {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


project.tasks.withType(AdbTask) { task ->
extension.attachDefaults(task)
}
Copy link
Contributor

@devisnik devisnik Jan 31, 2017

Choose a reason for hiding this comment

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

Would that be the same here?

project.tasks.withType(AdbTask).all(extension.&attachDefaults)

Not sure it's better though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking if it's possible to do it in 1 line, but I didn't figure it out when I was writing it. I think 3 lines is clear tho. What do you think?

@devisnik
Copy link
Contributor

I don't think we need to emphasize attachDefaults() as public API.
We just apply it to every AdbTask, that should be good enough.

@tasomaniac
Copy link
Contributor Author

tasomaniac commented Jan 31, 2017

@devisnik I thought about it a lot. I went back and forth. I agree with you. I think it is not really mandatory.
We will maybe even get rid of this attaching need in the future.

@tobiasheine tobiasheine merged commit 023d238 into develop Jan 31, 2017
@tobiasheine tobiasheine deleted the PT-422/fix-system-animation-warnings branch January 31, 2017 16:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants