search

City-of-Bloomington / open311-android

GeoReporter Android source code. Native Android smartphone client app for Open311 API civic issue reporting.
http://open311.org
Other
31 stars 36 forks source link

Enhancement #77

Closed beerdotpy closed 10 years ago

beerdotpy commented 10 years ago

Changed the UI of report submission. Instead of going to another activity for entering data now user can do the same on single screen.

inghamn commented 10 years ago

I have built your enhancement branch and tried it out on my phone. I had a few crashes, that, I think, were related to networking. But the current version also has networking crashes, so I'm not thinking it was related to your code. It's just another thing that makes it hard to review.

I live the idea and want to implement the single activity report when I can get a chance. These days I am working on multiple server-side projects, so I'm not sure when I can get around to it.

All that being said, the pull request does present some challenges to review and merge. NewReportFragment replaces ReportFragment, which I understand, but it would be nice to see that reflected in the Git log, instead of just commenting out all of the code. (My personal preference is to never have commented out code. Preserving past code is what Git is for. We can look it up in the history.)

NewReportFragment.onCreateView() is now a monstrous function, clocking in around 350 loc. That's just really hard to follow and understand. It would be better to split it up into smaller functions, ideally identifying the things that each of the views has in common.

One thing that might work well is to pull out all the OnClickListeners() that are currently anonymous, and turning them into full methods of the class.

In the iOS version, we ended up implementing it as a tablelayout, with each field being one cell in the table. This gave us a place to provide the layout rendering seperate from the handling of values entered by the user. Still around 100 loc for the cellForRowAtIndexPath() but easier to follow.

If you are interested, you might take a read through the iOS implementation. We did this type of Report view a while ago.

I know, the Android version has just been sitting there forever. I just have't had time to get back to updating it. It's almost getting to the point where a new full rewrite might be in order. At this point, we should do away with the Sherlock libraries. And the latest android HTTP and JSON libraries are much better, so I don't need to include my own versions.

beerdotpy commented 10 years ago

I tested the code and was working fine here.

I'll try pulling out the common functions and make oncreateView() compact and easy to understand. But because attributes contain different layouts so it makes difficult to contain them in single function.

I agree with a new full rewrite of the app is needed using the external libraries instead of using the whole json parser as i see in the current code.