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

Add a GPS Service. #373

Merged
merged 4 commits into from
Jun 2, 2024
Merged

Add a GPS Service. #373

merged 4 commits into from
Jun 2, 2024

Conversation

alejandrocalles
Copy link
Contributor

@alejandrocalles alejandrocalles commented May 31, 2024

Closes #318.

@alejandrocalles alejandrocalles self-assigned this May 31, 2024
@alejandrocalles alejandrocalles added the enhancement New feature or request label May 31, 2024
@violoncelloCH violoncelloCH added this to the Milestone 4 milestone May 31, 2024
@violoncelloCH violoncelloCH self-requested a review May 31, 2024 20:03
Copy link
Member

@violoncelloCH violoncelloCH left a comment

Choose a reason for hiding this comment

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

Nice, thanks for working on this! Really clean code and a nice structure with the interface, implementation and hilt module!

I just have a concern regarding the use o the proprietary google play library. Since we so far managed to avoid dependencies on any proprietary library, this makes the app require the presence of Googles proprietary services. It will not work on devices not having them installed like Huaweis more recent Smartphone, Amazon's Android devices, Windows new capability for running Android apps as well as privacy friendly Android variants coming without Google Services by design.
The solution for this would be to uses pure Android APIs for the location as detailed e.g. in this blog post: https://fobo66.dev/post/play-services-location-migration/#how-to-request-current-location-with-locationmanager
However given the approaching deadline, I'm not sure if it's reasonable to attempt this now. What do you think?

@alejandrocalles
Copy link
Contributor Author

alejandrocalles commented Jun 1, 2024

It's a good point, and I hadn't really thought about it.Unfortunately, I don't think it would be reasonable to make those changes in this PR. However, I do believe it would be better to stay away from proprietary libraries, so I would consider creating an issue in case this project is continued in the future :)

Copy link
Contributor

@srsingh04 srsingh04 left a comment

Choose a reason for hiding this comment

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

Nice and clean code! Good job, LGTM: ready to be merged.

@alejandrocalles alejandrocalles force-pushed the feature/location-service branch 2 times, most recently from 1945dac to de0e38d Compare June 2, 2024 14:26
Copy link
Member

@violoncelloCH violoncelloCH left a comment

Choose a reason for hiding this comment

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

as already said, nice implementation! thanks for the massive work!
I'll create an issue as discussed for the dependency concern :)

Copy link

sonarcloud bot commented Jun 2, 2024

@alejandrocalles alejandrocalles merged commit faf3a1e into main Jun 2, 2024
2 checks passed
@alejandrocalles alejandrocalles deleted the feature/location-service branch June 2, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done in sprint 9
Development

Successfully merging this pull request may close these issues.

Implement a GPS service to get the user position
3 participants