Skip to content
This repository has been archived by the owner on Apr 27, 2024. It is now read-only.

Extracts configurable networking layer. #56

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ecgreb
Copy link
Contributor

@ecgreb ecgreb commented Mar 28, 2014

  • Extracts HttpEngine interface.
  • Uses LwHttp as default networking client.
  • Implements alternate networking client using OkHttp.
  • Tests OkHttp integration using MockWebServer.

http://square.github.io/okhttp/

@hjanetzek
Copy link
Member

OkHttp looks very nice. I'm considering to switch over using it as default.

I've added a factory to create an engine for each loader thread. Not quite sure how to fix the tests though

@ecgreb
Copy link
Contributor Author

ecgreb commented Mar 31, 2014

@hjanetzek OkHttp is great. I've found it to be much more reliable and provide better error messaging than HttpURLConnection.

If you push your changes to a branch I can work on getting the tests passing again.

@ecgreb
Copy link
Contributor Author

ecgreb commented Mar 31, 2014

@hjanetzek oh I see the branch now... I'll take a look at this today.

@ecgreb
Copy link
Contributor Author

ecgreb commented Mar 31, 2014

$ ../gradlew clean test
Configuration on demand is an incubating feature.
:vtm-tests:clean
:vtm:compileJava UP-TO-DATE
:vtm:processResources UP-TO-DATE
:vtm:classes UP-TO-DATE
:vtm:jar UP-TO-DATE
:vtm-tests:compileJava UP-TO-DATE
:vtm-tests:processResources UP-TO-DATE
:vtm-tests:classes UP-TO-DATE
:vtm-tests:compileTestJava
:vtm-tests:processTestResources UP-TO-DATE
:vtm-tests:testClasses
:vtm-tests:test

BUILD SUCCESSFUL

Total time: 2.109 secs

ecgreb and others added 11 commits April 2, 2014 06:56
@hjanetzek
Copy link
Member

I've updated my branch. Are you ok with the api?

@ecgreb
Copy link
Contributor Author

ecgreb commented Apr 2, 2014

Looks good to me. I especially like passing a UrlTileSource into the constructor for LwHttp and OkHttpEngine. This is better than passing it on each request. Also it seems good to let the tile source handle formatting the URLs.

We may want to consider transitioning HttpEngine from an interface to an abstract class in the future so the constructor param can be enforced and common functionality can be abstracted.

@ecgreb
Copy link
Contributor Author

ecgreb commented Apr 3, 2014

@hjanetzek I pulled your latest changes and rebased onto master. Also fixed a compile error coming from a change in the jeo API.

@hjanetzek
Copy link
Member

@ecgreb thanks, maybe I should host the latest working jeo and gdx snapshots..

I was already testing the merge in my repository, now pushed to opensciencemap/master. It's no problem to change HttpEngine to be abstract when needed.

@ecgreb
Copy link
Contributor Author

ecgreb commented Apr 4, 2014

Great thanks! We are now using the latest version of vtm with okhttp in our app.

mapzen/open#121

@hjanetzek
Copy link
Member

@ecgreb would you mind updating okhttp to 2.0?

@ecgreb
Copy link
Contributor Author

ecgreb commented Jul 11, 2014

@hjanetzek this is definitely on our road map. Hopefully I'll have some time to work on it soon.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants