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

387 offline mappacks #417

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from
Draft

387 offline mappacks #417

wants to merge 18 commits into from

Conversation

jezhiggins
Copy link
Member

Downloading and managing map packs within the main app.

package net.cyclestreets.api

data class Map(
val id: String,
Copy link
Member

@oliverlockwood oliverlockwood Jun 5, 2020

Choose a reason for hiding this comment

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

picky, but 8 characters indentation seems excessive.

Also I wonder if we would be better off naming this class differently - e.g. MapPack - so we don't confuse it with the native Map class in Kotlin and/or Java.
If you agree with this we could probably update the naming of the Maps class, and possible the getMaps method also.

Copy link
Member

Choose a reason for hiding this comment

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

I now see it can't be MapPack as that would conflict.
What about MapReference or something similar; as effectively it acts as a reference to a downloadable map pack?

Copy link
Member Author

Choose a reason for hiding this comment

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

I started with MapPack, realised, changed to Map as a holding name, now considering DownloadableMap

return Maps(
featureCollection.features
.map { f -> toMap(f) }
.filter { m -> isBritainOrIreland(m) }
Copy link
Member

Choose a reason for hiding this comment

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

I agree with this restriction as a starting point. We can consider adding others subsequently as part of #46.


class MapPack private constructor(
private val map: Map
) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this would read easier as a one-liner rather than these 3 lines, but maybe that's just me.


class Maps(
private val packs: Collection<Map>
): Iterable<Map> {
Copy link
Member

Choose a reason for hiding this comment

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

I think this would read easier as a one-liner rather than these 3 lines, but maybe that's just me.

@oliverlockwood
Copy link
Member

I see this is still in partial state at present (clearly some more work to be done in terms of actually getting hold of the map pack, etc!) but hopefully my comments may still have some value.

@jezhiggins
Copy link
Member Author

2020-09-03 16 45 53

It doesn't look like much, but this a rendering a vector map that the app has downloaded from the CycleStreets server, using the Android DownloadManager.

There's still some more work to do on the UI elements - should probably ask if it's ok to download when not on Wifi, a way to uninstall them, etc, and on background updating - but moving in the right direction.

@oliverlockwood
Copy link
Member

Good work @jezhiggins! 🙌
Let me know when you'd like me to do some code reviewing and I'll jump on it.

jezhiggins added a commit that referenced this pull request Sep 10, 2020
It's causing crashes on some phones, and will be replaced with a new
mechanism soon, see PR #417.
oliverlockwood pushed a commit that referenced this pull request Sep 11, 2020
It's causing crashes on some phones, and will be replaced with a new
mechanism soon, see PR #417.
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