Skip to content

Add offline plugin#72

Merged
tobrun merged 1 commit into
masterfrom
tvn-offline-plugin
Dec 1, 2017
Merged

Add offline plugin#72
tobrun merged 1 commit into
masterfrom
tvn-offline-plugin

Conversation

@tobrun
Copy link
Copy Markdown
Member

@tobrun tobrun commented Aug 3, 2017

WIP offline plugin, closes #19

@tobrun tobrun self-assigned this Aug 3, 2017
@tobrun tobrun mentioned this pull request Aug 14, 2017
Closed
@tobrun tobrun mentioned this pull request Sep 12, 2017
Closed
@tobrun tobrun force-pushed the tvn-offline-plugin branch 2 times, most recently from a6bb22b to 4066863 Compare September 18, 2017 12:37
@tobrun tobrun force-pushed the tvn-offline-plugin branch 2 times, most recently from 1c848a8 to 1001ebf Compare September 27, 2017 12:04
@tobrun
Copy link
Copy Markdown
Member Author

tobrun commented Sep 27, 2017

@zugaldia would you able to review current state? would love to merge this initial version and continue iterating on it with different issues and PRs. (will start working on ticketing out the issues).

@tobrun tobrun requested a review from zugaldia September 27, 2017 12:07
@tobrun tobrun mentioned this pull request Sep 28, 2017
Merged
@tobrun
Copy link
Copy Markdown
Member Author

tobrun commented Oct 4, 2017

@zugaldia this PR is ready for review

zugaldia
zugaldia requested changes Oct 4, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@zugaldia zugaldia left a comment

Choose a reason for hiding this comment

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

Small comments. Also, for some strange reason, notifications aren't working for me. When I click to download a region I can see in the logs that tiles are being downloaded, but I don't see any progress/notification on the UI. I also see this logs being constantly repeated:

10-04 15:51:38.587 29189-29189/com.mapbox.mapboxsdk.plugins.testapp W/Notification: Use of stream types is deprecated for operations other than volume control
10-04 15:51:38.587 29189-29189/com.mapbox.mapboxsdk.plugins.testapp W/Notification: See the documentation of setSound() for what to use instead with android.media.AudioAttributes to qualify your playback use case

Comment thread plugins/build.gradle Outdated
repositories {
jcenter()
maven { url 'https://maven.google.com' }
maven { url "http://oss.sonatype.org/content/repositories/snapshots/" }
Copy link
Copy Markdown
Member

@zugaldia zugaldia Oct 4, 2017

Choose a reason for hiding this comment

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

@tobrun We should make a note to remove this before final release (ticket?).

Copy link
Copy Markdown
Member Author

@tobrun tobrun Oct 11, 2017

Choose a reason for hiding this comment

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

I consider above comment a great note for that ;)

Comment thread ...ns/locationlayer/src/main/java/com/mapbox/mapboxsdk/plugins/locationlayer/LocationLayer.java Outdated

GeoJsonSource locationSource = new GeoJsonSource(LocationLayerConstants.LOCATION_SOURCE, emptyFeature);
mapboxMap.addSource(locationSource);
if (mapboxMap.getSource(LocationLayerConstants.LOCATION_SOURCE) == null) {
Copy link
Copy Markdown
Member

@zugaldia zugaldia Oct 4, 2017

Choose a reason for hiding this comment

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

Are these changes required by 5.2? If so, 👍

Copy link
Copy Markdown
Member Author

@tobrun tobrun Oct 11, 2017

Choose a reason for hiding this comment

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

yes, it seems so, will fix this up in a seperate PR

Comment thread plugins/offline/src/main/java/com/mapbox/mapboxsdk/plugins/offline/NotificationOptions.java Outdated
private int smallIconRes;
private String returnActivity;
private String contentTitle = "Offline download";
private String contextText = "Downloading..";
Copy link
Copy Markdown
Member

@zugaldia zugaldia Oct 4, 2017

Choose a reason for hiding this comment

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

Missing period.

Comment thread plugins/offline/src/main/java/com/mapbox/mapboxsdk/plugins/offline/OfflineDownloadOptions.java

// restrict visibility, only libs allowed to invoke building the OfflineDownload
OfflineDownload build() {
//TODO add validation
Copy link
Copy Markdown
Member

@zugaldia zugaldia Oct 4, 2017

Choose a reason for hiding this comment

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

Ticket?

Copy link
Copy Markdown
Member Author

@tobrun tobrun Oct 11, 2017

Choose a reason for hiding this comment

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

#132

Comment thread plugins/offline/src/main/java/com/mapbox/mapboxsdk/plugins/offline/OfflineDownload.java
return definition;
}

byte[] getMetadata() {
Copy link
Copy Markdown
Member

@zugaldia zugaldia Oct 4, 2017

Choose a reason for hiding this comment

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

What if we exposed a higher level API for the metadata (can be ticketed separately). Instead of exposing the byte[] separately, we'd expose a POJO with, say, title and description, that we serialize/deserialize as part of this plugin?

Copy link
Copy Markdown
Member

@zugaldia zugaldia Oct 4, 2017

Choose a reason for hiding this comment

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

Just saw OfflineUtils, it simplifies things but I'd even go further than that and never expose that directly.

Copy link
Copy Markdown
Member Author

@tobrun tobrun Oct 11, 2017

Choose a reason for hiding this comment

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

Yeah, initially I went with String name and afterwards went back for byte[]. I'm liking the idea of a POJO. To make that work, we will need an object that is Parceable as we need to be able to push that into a Bundle for Intent communication.

Copy link
Copy Markdown
Member

@zugaldia zugaldia Oct 11, 2017

Choose a reason for hiding this comment

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

@tobrun Or a small POJO easily convertible to JSON with Gson and use the String representation? Or would that potentially be too large for a Bundle?

@tobrun tobrun mentioned this pull request Oct 11, 2017
Closed
danesfeder
danesfeder reviewed Nov 30, 2017
View reviewed changes
Comment thread plugins/offline/src/main/java/com/mapbox/mapboxsdk/plugins/offline/NotificationOptions.java Outdated
return this;
}

public String getContextText() {
Copy link
Copy Markdown
Contributor

@danesfeder danesfeder Nov 30, 2017

Choose a reason for hiding this comment

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

@tobrun did you mean getContentText() here?

@tobrun tobrun force-pushed the tvn-offline-plugin branch 2 times, most recently from 251aaae to 97c6f26 Compare December 1, 2017 10:13
@tobrun tobrun force-pushed the tvn-offline-plugin branch from 97c6f26 to 5e32ddc Compare December 1, 2017 12:10
@tobrun
Copy link
Copy Markdown
Member Author

tobrun commented Dec 1, 2017

@zugaldia I updated this PR to conform to the new project structure in #144. I would love to merge this to master so we can start building snapshots and iterate on it (will create some follow up issues so someone else on the team can pick them up)

@zugaldia
Copy link
Copy Markdown
Member

zugaldia commented Dec 1, 2017

I would love to merge this to master so we can start building snapshots and iterate on it

Let's do it, just went ahead and approved the PR.

CI is complaining about the following:

* What went wrong:
A problem occurred evaluating script.
> java.lang.UnsupportedClassVersionError: org/sonarqube/gradle/SonarQubePlugin : Unsupported major.minor version 52.0

@cammace Do you know why? Is this because of the changes in #144?

@cammace
Copy link
Copy Markdown

cammace commented Dec 1, 2017

The Bitrise script is still in this repo and ran correctly through all the same checks that circle ci would have. It's safe to ignore the fact that circleci is failing.

@tobrun tobrun merged commit f1cacd3 into master Dec 1, 2017
@tobrun tobrun deleted the tvn-offline-plugin branch December 1, 2017 17:51
@langsmith langsmith mentioned this pull request Dec 2, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zugaldia zugaldia zugaldia approved these changes

+1 more reviewer

@danesfeder danesfeder danesfeder left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@tobrun tobrun

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Offline Plugin

4 participants

@tobrun @zugaldia @cammace @danesfeder