PR for 30 location picker - Phase 1

[lemonboston]

For #30.

This first phase of the location picker screen contains the replacement of the Google provider location picker UI overlay with our own minimal screen.

@dmfs Please review.

Things left from implementing the full location picker (we will create new stories where appropriate as agreed):

• Event list screen: drop down arrow instead of location menu icon
• Location picker screen:
• 'x' cancel button at the right side of EditText (or using SearchView which has it but harder to customize)
• icons next to suggestions and icon to the search bar (?)
• Current location
• Recent locations
• Top city suggestions

Technical notes:

• The second Place API call to get the actual Place by the id is now wired back to the controller that handles the list, it could be implemented as loader microfragment possibly. Or we could also consider prefetching them from the suggestions, but it is probably not worth it or not that important.
  (Error handling flow would need to be considered as well with the loader fragment.)
• The errors related to Places API are not handled currently, I suggest a new story for them. There are TODOs left in the code, 2 for the tasks and one more difficult for the google api client error handling.
• The new GeoPlace type is now composed of NamedPlace and GeoLocation, it could also just extend them. I started to refactor that way but realized that GeoLocation equals() is used in EventListControllerImpl, so that would mean we may need to provide equals() in all GeoPlace or maybe use a Equalable decorator within the controller? It seems like a little bit to me that there might be some general conflict or difficulty with Value objects and polymorphism because of the need to always provide equals().
  Anyway, please check if you think GeoPlace would be better as extending the interfaces instead of composed from them. I have a feeling that that might be a better design.

About implementing the list modules in the next phase:
I have a plan already in mind, it would be like something as follows (names are just indications):
Each module will have a PlaceListController that receives the query text changes and can initiate background tasks and update the list. They will each have a ListItems kind of object which they can add and remove items to and from, it will be a proxy to the real ListItems that will hold the full list. That will be a Sectionable extension, so each proxy can have an id automatically assigned for them for example and this SectionableListItems can add/remove items to sections based on these ids. Or there is another alternative as well, but basically the important part would be that the module Controllers will not need to know anything about the full list.
The fragment could be parameterized if needed by sending in factories that create these controllers.
The current callback on the PlaceListController would be removed, any action would be initialized by the controllers themselves, for example in this case possibly loading a loader microfragment for the place by id call as mentioned earlier.
(As you can see I changed the names from Provider to Controller for this components for the lists, I think they fit better to what they do now.)

[dmfs]

No code comments yet, just one about the UX so far. Please remove the flicker when a new character is entered. At present the items in the list flicker when the new results are available. I would expect a proper animation that removes all elements, that are no longer among the results. Just like with the original Google picker.

[lemonboston]

I didn't notice it, but yes, that clearly wasn't nice that way.

I've made the update to animate the changes and also started to introduce a general design for list change updates, and I really like it at the moment, I think it may be very useful in the future.
It is inspired in part by what you mentioned once about creating a change set and then applying it.
Calculating changes in the background might need to be figured out later, but here it is fine to make it on main thread because there are only 6 items in the list.

edit: (Calculating in background will have challenges on synchronizing to the list, or having some queue to not allow the next calculation begin until the previous is finished and applied on the main thred.)

[dmfs]

I have a few general concerns regarding the architecture, but I would rather refactor the entire thing once the other modules have been implemented than doing this now.

[dmfs]

class not final?

[dmfs]

Copyright date reverted

[dmfs]

The year is still not updated.

[dmfs]

class not final

[dmfs]

Copyright date reverted

[dmfs]

Which interface does this implement?

We talked about this before (in the "smart views" vs. "presenters" discussion).
This View is called LocationSuggestionItemView, which is a very specific name. Why has it methods like setTitle and setSubTitle with very generic names? You could should call it TwoLineItemView instead (or give the methods more specific names).

However, I'd prefer the "smart view" version which could implement an interface like this

public interface SmartView<T>
{
   void  update(T data);
}

With T being NamedPlace in that case.

[lemonboston]

You are right that it wasn't clear how I would approach this smart or dumb view for the list items, because I felt that the ListItem itself usually need to hold the data anyway, item views are also recycled, so it might be better to keep data and logic there. But I like that SmartView interfaces as well, also to be coherent with other (not list item) views, this would be a clearer approach.
I've added it and I see that it can lead to more generalization of the ListItems, I will look into that in the next round. Namely to check if ListItem<V extends View> can be changed to ListItem<V extends SmartView> and also to check how to handle callback if needed.
A SmartView can of course execute stuff itself, but I am not sure if it should execute something async and then act on the result, that clearly seems too much, so in some cases we might still need callback from it. It depends also on whether that loading can be done in a new MicroFragment, then it's not a problem. For now, I left the callbacks in, which also meant that PlaceSuggestionItem is still very specific.

About making PlaceSuggestionItemView TwoLineItemView, I think the layout will be usually specific to the actual place of usage, so my feeling is that it's probably not worth trying to create something general, it might just cause problem if we used something like that for multiple places and couple them that way, we couldn't change one without changing the other.

[dmfs]

Did you consider to create AnywhereNamedPlace lazily? You would have to store an ApplicationContext but that's not an issue IMO.

[lemonboston]

You are right, keeping app context should not be a problem, so we can possibly use that inside lazy declarations more. I've updated it.

[dmfs]

class not final

[dmfs]

Always call abstract classes like Abstract…, i.e. AbstractTextWatcher or AbstractDefaultTextWatcher

[dmfs]

Make this lazy. Pass an ApplicationContext and let name() return context.getString(R.string.schedjoules_default_location_placeholder_name).

[lemonboston]

done

[lemonboston]

I've made the requested changes.

[dmfs]

I've spotted a few more minor issues.

[dmfs]

The year is still not updated.

[dmfs]

Another wrong year.

[dmfs]

Please make inner classes final too.

[dmfs]

Please make it final

[dmfs]

Make this class final.

[dmfs]

Make this class final.

[dmfs]

Any reason this is not final?

[lemonboston]

No, I actually remembered that I missed it but forgot to go back, thanks for spotting it, I've made it final.

[lemonboston]

I've change all classes to final in the codebase that hadn't been and updated all copyrights to 2017.