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

Fix vibration for testapps #2034

Merged
merged 1 commit into from
Dec 9, 2019
Merged

Conversation

opacam
Copy link
Member

@opacam opacam commented Dec 5, 2019

Method vibrate(milliseconds) was deprecated in API level 26,
now we should use vibrate(android.os.VibrationEffect).

I labeled this WIP because I thing that we should consider the possibility to use plyer instead of using the more low level way of pyjnius, because we are repeating the code a lot with all the testapps (except for the flask testapp...which uses the same solution but written slightly differently). But the vibration fix for plyer it was merged recently, and we still not have a release with it, so it will force us to put a github requirement in our testapp setup.py files and pin it to a commit or master branch...so...
@inclement and @AndreMiras, what do you think about this, plyer or pyjnius?

Note:

  • This solution is based on the Fix vibrator, which was not working on Android devices. plyer#523 and was adapted to our needs
  • I tested the testapp_flask and the standard one testapp_sqlite_openssl and this last one, you could also test it, without building it, via the corresponding job, you can download the testapps built with gh-actions via the Artificats button 😄

See also: https://developer.android.com/reference/android/os/Vibrator#vibrate(long)

Method `vibrate(milliseconds)` was deprecated in API level 26,
now we should use ` vibrate(android.os.VibrationEffect)`.

See also: https://developer.android.com/reference/android/os/Vibrator#vibrate(long)
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

LGTM thanks!
Regarding plyer vs pyjnius I haven't dug into it, but if as you said the plyer fix is merged, but not released, then you have you response.
Also I have never played much with the testapp myself, but I see a lot of code smell, just in the context of this diff.
First to me we should be using on device unit testing using for instance Python unittest library.
Second it already has a lot of DRY violation prior your diff that why you ended up having to copy past your fix.
I don't see why it's necessary to test_pyjnius() so often across all the apps. Again I haven't studied the test apps so I may miss something.
So to recap, I think this PR is OK for a merge as it fixes the deprecated behavior you described.
But I think we may want to address this whole testapp thing in the future as it feel a technical debt in our testing tools to me

@opacam opacam changed the title [WIP] Fix vibration for testapps Fix vibration for testapps Dec 9, 2019
@opacam
Copy link
Member Author

opacam commented Dec 9, 2019

Yeah, I agree with you, we should give a little more love to our testing tools, let's merge this for now, since as you said, fixes the deprecated behavior.

Thanks @AndreMiras !! 😆

@opacam opacam merged commit 3a155e3 into kivy:develop Dec 9, 2019
@opacam opacam deleted the hotfix-testapp-vibration branch December 9, 2019 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants