Display beacons on map #96
Conversation
There was a problem hiding this comment.
The code is nice and well commented, well done. There is one thing. I think, we will have to adapt the way we are adding beaoncs to the map, as I did something diferent with the map ui issue #86, but it's more a detail. Also, maybe, but it might be another issue, and it's more UI related, we could try to have a different symbol for the beacon, or at least a different color, not just the plain red marquer. And also, try to show them only at a certain zoom level, to not overflow the map with it, when we will have a lot of them.
But overalll good job 👍
|
I tried adding custom icons but it's quite a hassle, but adding custom colors is pretty straightforward. Regarding the zoom level, I was thinking that we could give beacons a ranking, and only display those of a certain rank at a given zoom level, in order to have a smooth transition, but I'll create another issue for that. |
Yes I agree, this can be a future Issue |
app/src/androidTest/java/ch/epfl/cs311/wanderwave/ui/MapTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ch/epfl/cs311/wanderwave/model/data/Beacon.kt
Outdated
Show resolved
Hide resolved
…play-beacons-on-map
…eacons-on-map # Conflicts: # app/src/main/java/ch/epfl/cs311/wanderwave/model/data/Beacon.kt # app/src/main/java/ch/epfl/cs311/wanderwave/model/data/Location.kt # app/src/main/java/ch/epfl/cs311/wanderwave/ui/screens/MapScreen.kt
…play-beacons-on-map
app/src/androidTest/java/ch/epfl/cs311/wanderwave/model/BeaconConnectionTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ch/epfl/cs311/wanderwave/model/data/Beacon.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ch/epfl/cs311/wanderwave/model/data/Beacon.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ch/epfl/cs311/wanderwave/model/data/Profile.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ch/epfl/cs311/wanderwave/model/remote/BeaconConnection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ch/epfl/cs311/wanderwave/model/remote/TrackConnection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/ch/epfl/cs311/wanderwave/ui/screens/MapScreen.kt
Outdated
Show resolved
Hide resolved
|
Thanks for the comments @joriba, I think the best thing to do would be to merge my pull request and then I can modify your branch directly to modify your changes. This would prevent the most merge conflits I think. |
|
I'll wait for #92 to be merged because some of the tests I pulled from it do not pass yet. |
# Conflicts: # app/src/main/java/ch/epfl/cs311/wanderwave/ui/screens/MapScreen.kt
…play-beacons-on-map
|
I merged everything from main, and refactored my code to accommodate the changes. Als looks like @KlaKalma made the requested changes so I'll go ahead and mark the comments as resolved. |
|
I think you'll have to wait for #88 to be pulled in and then again resolve a lot of merge conflicts (very annoying ones, sorry...). The problem is that Clarence pulled in your earlier changes into his branch and then merged them into main. I took them from there and pulled them into my branch, resolving the merge. |
app/src/androidTest/java/ch/epfl/cs311/wanderwave/model/BeaconConnectionTest.kt
Show resolved
Hide resolved
# Conflicts: # app/src/main/java/ch/epfl/cs311/wanderwave/ui/screens/MapScreen.kt
Adapted tests Finally solved all issues
|
Redid my work on a new branch due to some weird dependencies issues I was experiencing.
Closes #34