Kegbot / kegbot-android

Android beer kegerator frontend and controller app. Works with a Kegbot server to make your kegerator awesome.
http://kegbot.org/docs/android/
GNU General Public License v2.0
42 stars 53 forks source link

Lint cleanup, refactor Clock to the DateUtils class #60

Closed tmertens closed 10 years ago

tmertens commented 10 years ago

Clean up a few lint warnings and deprecations much older than the minimum Android version.

mik3y commented 10 years ago

Pull request now has a bunch of other random changes in it.

I'm just curious, what's your goal with your fork?

tmertens commented 10 years ago

Ah sorry, I must have merged those into the same branch on my end. I will submit a new PR with the original lint changes.

Re: our fork, our present intent is to modify Kegbot-android to work with Kegbot-server for registering pours without a flow controller (with the expectation that it should also still work with a flow controller when installed). How this might look when finished may be, in its first iteration, simply a static pour volume that is registered when a user logs in and initiates a pour. The ideal final solution would be to provide an interface to select a glass type and possibly approximate pour volume (1/4, 1/2, 3/4 full, etc) which would then calculate an approximate volume for the pour. Bottom line, you could set up kegbot server and android together in a working state with no flow controller and then add a FC later if desired. Initially we thought this would be a relatively simple task but after some investigation of the existing code base it has proved to be a much larger task than originally thought.

If you look at the current state of the fork (see the pour-model branch), I am currently attempting to decouple a Pour model from the Flow. The Flow class presently consumes a lot of behavior which does/should not belong to it and it is very tightly coupled to the PourInProgressActivity. This is currently in an exploratory state as I do not have a full understanding of all of the inner workings of kegbot-android and am not entirely sure whether it will be successful at this point.

We could use help from someone who has a full kegbot server/flow controller setup to ensure things continue to work correctly as the core is refactored. All new models and behavior have tests added with full coverage of all public methods (a couple of new models are missing tests at the moment but they will be added shortly). All previously existing tests are being maintained and are in a passing state.

For more information on the principals behind some of the refactoring changes, see: http://en.wikipedia.org/wiki/Single_responsibility_principle http://robots.thoughtbot.com/sandi-metz-rules-for-developers

mik3y commented 10 years ago

Ah sorry, I must have merged those into the same branch on my end. I will submit a new PR with the original lint changes.

No problem. And don't worry too much about re-sending it; I've made some similar changes in my own tree (thanks for the motivation -- warning-free is bliss..)

Re: our fork, our present intent is to modify Kegbot-android to work with Kegbot-server for registering pours without a flow controller

Thanks for sharing! I'm always on the lookout for new ways people are using Kegbot.

I did something similar once when we didn't have time to build a controller, a sort of "honesty bot". It was little more than KegbotApiImpl with a few buttons (but, not integrated in the main app).

The Flow class presently consumes a lot of behavior which does/should not belong to it and it is very tightly coupled to the PourInProgressActivity.

I beg to differ. Flow is hardly coupled with PourInProgressActivity, though that activity certainly knows how to render one -- that's, well, its job. Flow is a dumb data object encapsulating all state about.. an active flowsensor-driven flow. (OK, arguably, shout & image state should hang off the activity rather than the flow, but those are rather small transgressions..)

Since you're interested in recording new drinks that do not come from live flows, I'd say you actually have little reason to touch Flow, nor PourInProgressActivity for that matter. Just build a RecordDrinkRequest in a new activity (after showing the "select a size" UI, or whatever) and pass it off to the KegbotApi instance when ready.

(Of course, my opinion only matters if you intend on upstreaming it!)

cheers, mike

tmertens commented 10 years ago

@mik3y Another question for you - I'm trying to understand what the protobuffers are doing. As far as I could tell from my limited understanding of kegbot-android thus far, the messages to the server are being constructed into protobuffer models, then deconstructed back into plain text http messages before sending the requests, rather than actually sending the protobufs over the wire. I'm curious what the reasoning was to do it this way.

tmertens commented 10 years ago

I did something similar once when we didn't have time to build a controller, a sort of "honesty bot".

We have no need to track usage for any purpose other than fun and social reasons, so an 'honesty bot' meets our needs quite well.

just build a RecordDrinkRequest in a new activity (after showing the "select a size" UI, or whatever) and pass it off to the KegbotApi instance when ready.

This is one possible option I have considered, but would prefer to reuse as much of the behavior in the PourInProgressActivity as possible (e.g. picture-taking, shout, user auth) without simply copy+pasting it. I would like to actually improve the code base and not simply add to it :smiley:. Really, there is little to no reason that the PourInProgressActivity needs to be tightly coupled to the Flow. Looking at the UI for the activity, it seems that it primarily cares about receiving messages which provide it updates about the volume of the pour ( and, possibly, a notification of when the pour is complete.

My primary target at this time is to provide a variation of the PourStatusFragment to accomplish our goals, but this would still require decoupling of the PourInProgressActivity from the live flow and I have to determine what difficulties, if any, this may present. In any case, I feel we have a slow journey ahead.

tmertens commented 10 years ago

@mik3y Are there any plans to merge these changes in? I merged in your most recent changes so this should be ready to go.

I have some other refactors and new unit tests that I have been working on and would like to begin submitting in bite-size chunks once these changes get merged in. Right now I've decided my primary focus should be refactoring and adding test coverage without changing/adding new features.

mik3y commented 10 years ago

Thanks for making these changes! The commit list looks huge, but the actual changes are now quite small; would you mind squashing them into a commit or two?

And, help with test coverage and other things would be awesome! I'll try to get back to pulls quickly -- feel free to pop into #kegbot on freenode if you'd like to discuss anything first.

mik3y commented 10 years ago

Closing as obsolete: some of the cleanups have already been applied (Http.java, CameraFragment.java, possibly others). An injectable clock source would still be a welcome change, although the move to studio/gradle means it needs to be rebased against master.