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

The implementation of the location pinning app #2

Merged
merged 35 commits into from
Aug 27, 2022
Merged

The implementation of the location pinning app #2

merged 35 commits into from
Aug 27, 2022

Conversation

shubertm
Copy link
Member

All the implementation has been made, the app allows a user to enter coordinates manually, using a link and getting sharing intent from Google Maps and OpenStreetMap. It also allows the user to choose a folder to save location files in JSON format. The selection of the folder uses ARK-FilePicker. It is also in light and dark mode.

@gitcoinbot gitcoinbot mentioned this pull request Jul 22, 2022
@kirillt
Copy link
Member

kirillt commented Jul 23, 2022

@ShubertMunthali hello and thank you so much for the PR!

Unfortunately, here is come confusion happened! Yes, I did create bounty for the task. But I have another developer approved for it. The problem is that somebody faked my bounty for unknown reason, I really don't understand why anyone would need it.

Here is my original bounty with payment in ZIL: https://gitcoin.co/issue/29079
Here is fake bounty with payment in XTZ: https://gitcoin.co/issue/29095

I'm contacting GitCoin support so they will remove fake bounty.

What about the PR: you are very welcome and I don't want this work be in vane. Please, contact me in Telegram for discussing details about what we can do. My handle is @kirill_taran there. Another dev, which was approved in original bounty, is working on this task and he is close to finish. He took a little bit different approach but maybe we could combine your efforts.

@kirillt
Copy link
Member

kirillt commented Jul 23, 2022

Also if you want me to test this app, please enable GitHub Actions and create CI pipeline, so it would be possible to just download APK. Here are examples in our repos:
https://github.com/ARK-Builders/ARK-Shelf/blob/main/.github/workflows/build.yml
https://github.com/ARK-Builders/ARK-Navigator/blob/main/.github/workflows/build.yml
https://github.com/ARK-Builders/ARK-Retouch/blob/main/.github/workflows/build.yml

@shubertm
Copy link
Member Author

Alright, that's ok. I have contacted you on Telegram.
I think you should test it. I will do as you specified.
How could I not know that it's fake?... Goooosh!

@kirillt
Copy link
Member

kirillt commented Aug 14, 2022

Certainly possible to fix before merging:

  • Location coordinates are round up too much: e.g. 42.5046727 becomes 42.5.
    The app must not lose precision.
  • If path /a/b/c is selected for storage, locations should be saved into it.
    At the moment, they are saved into /a/b/c/My Locations.
  • If storage path changes, the app must display locations from it without app restart.
  • Actually, we don't need [save] button.
    Let's just immediately write locations on disk upon [add] clicked.
  • Please, make [description] field optional.

Bonus points:
- [ ] Interactive items in the locations list.
We can open the location in external app (Google Maps or OSM) when it's clicked.
(Moved to separate issue: #3)
- [ ] Automatic filling-in of location name (if there is any).
(Moved to separate issue: #4)

Copy link
Member

@mdrlzy mdrlzy left a comment

Choose a reason for hiding this comment

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

A few controversial points that can be done later

Comment on lines 75 to 83
val locationName: EditText = view.findViewById(R.id.locationName)
val locationDesc: EditText = view.findViewById(R.id.locationDesc)
val urlText: EditText = view.findViewById(R.id.urlText)
val addButton: Button = view.findViewById(R.id.addButton)
val saveButton: FloatingActionButton = view.findViewById(R.id.saveButton)
val layoutManager = LinearLayoutManager(requireContext())
val recyclerView: RecyclerView = view.findViewById(R.id.recycler_view)
longitude = view.findViewById(R.id.longitude)
latitude = view.findViewById(R.id.latitude)
Copy link
Member

Choose a reason for hiding this comment

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

it's better to use view binding instead of findViewById
Here is a good library for this
https://github.com/androidbroadcast/ViewBindingPropertyDelegate

Comment on lines 11 to 23
private val coordinateList:
MutableLiveData<MutableList<Coordinates>> by lazy{
MutableLiveData<MutableList<Coordinates>>().also{
it.value = mutableListOf()
}
}

private val locationList:
MutableLiveData<MutableList<Location>> by lazy{
MutableLiveData<MutableList<Location>>().also{
it.value = mutableListOf()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use StateFlow since we use coroutines

Comment on lines 10 to 14

class URLParser {
companion object{
private const val GOOGLE_MAPS = ".google."
private const val GOOGLE_MAPS_SHORT = "goo.gl"
Copy link
Member

@mdrlzy mdrlzy Aug 18, 2022

Choose a reason for hiding this comment

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

It's better to add DI and use the repository pattern instead of companion object
Same for JSONFile, JSONParser

app/src/main/kotlin/com/ark/globe/jsonprocess/JSONFile.kt Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@shubertm
Copy link
Member Author

Certainly possible to fix before merging:

  • Location coordinates are round up too much: e.g. 42.5046727 becomes 42.5.
    The app must not lose precision.
  • If path /a/b/c is selected for storage, locations should be saved into it.
    At the moment, they are saved into /a/b/c/My Locations.
  • If storage path changes, the app must display locations from it without app restart.
  • Actually, we don't need [save] button.
    Let's just immediately write locations on disk upon [add] clicked.
  • Please, make [description] field optional.

Number 1, we discussed the problem. Found out it was not the coordinate fetching mechanism

@kirillt kirillt requested a review from mdrlzy August 24, 2022 07:49
@kirillt kirillt merged commit 9c87501 into ARK-Builders:main Aug 27, 2022
@kirillt kirillt mentioned this pull request Nov 11, 2022
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.

3 participants