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

Task/prefer physical device if connected #25

Conversation

johanlantz
Copy link
Contributor

@johanlantz johanlantz commented Jun 13, 2018

If there is a physical android device connected, use it

Includes #24

@johanlantz johanlantz closed this Jun 13, 2018
@johanlantz johanlantz reopened this Oct 1, 2018
@janpio
Copy link
Member

janpio commented Oct 1, 2018

If you hope for passing tests: #34 has to be merged and this branch then rebased onto master after that unfortunately ;)

@johanlantz
Copy link
Contributor Author

Ok, all the changes are rather small but helped a lot with getting Android to work better.

It might be faster for you to just look at the last PR and if you like the accumulated changes, simply copy paste them?

@janpio
Copy link
Member

janpio commented Oct 1, 2018

Ah, now I see that these are staggered pull requests - each one adding another feature. I will edit your description to reflect that.

@janpio
Copy link
Member

janpio commented Oct 2, 2018

This PR is currently missing the implementation of Util.getAndroidPhysicalDevice.

@janpio
Copy link
Member

janpio commented Oct 2, 2018

Ah, was added in your fork in a later commit as well: koa-health@66b6b27#diff-c25f5436d57d8f3c5b863535e37855d4

Could you please cherry pick this commit and also the following one into this branch?

koa-health@9f0dafe#diff-c25f5436d57d8f3c5b863535e37855d4

That should make the file correct.

@janpio
Copy link
Member

janpio commented Oct 2, 2018

I think #46 is exactly what I asked for in my previous comment. So it probably makes sense to not merge this one, and just merge the other one. Correct?

Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

util.getAndroidPhysicalDevice is missing here

#46 does have it (and more)

@johanlantz
Copy link
Contributor Author

Sorry about the staggered PR's. I was not sure which was the best way to send the PR's so I just took the PR's I had and created new ones back from the fork. I understand this was not optimal but hopefully the last one that contains all changes is small enough to decide if you want to integrate the changes or not.

@janpio
Copy link
Member

janpio commented Oct 3, 2018

It is small enough, but I still want to merge the changes feature by feature.

But I can edit your PR branches, so I should be able to just "mangle" them until they fit our needs ;)

Thanks for opening the PRs, I will see when I can get through them all and let you know in each one what happens (or not).

@janpio janpio closed this Nov 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants