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 to get android device name or hostname for linux and windows #630

Merged
merged 16 commits into from
Jun 19, 2021

Conversation

ljnath
Copy link
Contributor

@ljnath ljnath commented Jun 19, 2021

This support is for adding a new feature (plyer.devicename) which will allow user to get the device name in case of an android device.
This is also supported for linux and windows where it will return the hostname of the environment.

I also wanted to added support for ios ; but I don't have environment to test those.

Please review this PR.

PS: testing screenshot attached for windows and linux
Validated the functionality of socket.gethostname in MacOSX and added its support as well.

image_2021-06-19_150212
image_2021-06-19_150219

Copy link
Member

@tshirtman tshirtman left a comment

Choose a reason for hiding this comment

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

Looks good to me :), i think the socket implementation will likely work just the same on osx, as it's an Unix as well, for ios i don't know, there is probably a way with pyobjus, but that's fine, they can be added later if some one needs them.

Can you add the few feature to the feature table in the README.md ?

plyer/facades/devicename.py Outdated Show resolved Hide resolved
plyer/facades/devicename.py Outdated Show resolved Hide resolved
@ljnath ljnath requested a review from tshirtman June 19, 2021 11:05
@AndiEcker
Copy link

AndiEcker commented Jun 19, 2021 via email

@ljnath
Copy link
Contributor Author

ljnath commented Jun 19, 2021

to prevent getting 'localhost' as device name I'd recommend using Python's platform.node() instead of socket.gethostname() (or socket.getfqdn()). see also ansible/ansible#2663 and the SO answer of https://stackoverflow.com/users/7846592/ninjakang ( https://stackoverflow.com/questions/13931924/whats-the-difference-between-gethostname-and-getfqdn), concluding in:
Finally, python also has platform.node which is basically the same as socket.gethostname on python, though this might be a better choice for multiplatform code.
Am Sa., 19. Juni 2021 um 12:05 Uhr schrieb Lakhya Jyoti Nath < @.>:

@.
* commented on this pull request. ------------------------------ In plyer/facades/devicename.py <#630 (comment)>: > +''' + + +class DeviceName: + ''' + DeviceName facade. + ''' + + @Property + def device_name(self): + ''' + Property that returns the device name of the platform. + ''' + return self.get_device_name() + + def get_device_name(self): Done — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#630 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4U63XPQDHINZP7QAZNB53TTR2W5ANCNFSM4664XBYA .

Thank you for your reply.
socket.gethostname() is suppose to give me the hostname of the environment which cannot be localhost; unless user has specified that.

I also tested with the platform.node() ; it does return the same name.
However I checked the implementation of the method, and it looks like internally it is calling socket.gethostname()
https://github.com/python/cpython/blob/44fb55149934d8fb095edb6fc3f8167208035b96/Lib/platform.py#L571

Copy link
Member

@tshirtman tshirtman 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!

@tshirtman
Copy link
Member

Ok, some styling issues were there before, but if you want to fix the couple ones you added in the test, that'd be welcome.

@ljnath
Copy link
Contributor Author

ljnath commented Jun 19, 2021

Done, fixed the styling issues in my tests.
Is it safe to merge, or should I wait on anything ?

@tshirtman tshirtman merged commit daf23c3 into kivy:master Jun 19, 2021
@tshirtman
Copy link
Member

Merged, thanks :)

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.

None yet

3 participants