First major UI overhaul for 2.0

Updates include:
* Rearrange layouts and base classes to better handle new page types (no title bar, grey title bar, blue title bar, search results)
* New resources, layouts and styles
* Search button added to logo bar
* Search device-button wired up with NPR search box (and default options)
* Search results has search box at the top
* Navigation button on logo bar (nav menu still needs work)
* News story lists now have teaser and image and only show icon when audio exists

[jeremy.wadsack, 2011-02-11 00:00:17]

This is a big patch set including changes to lots of files. We changed the
structure of the app so that there's now a TitleActivity subclassing from
PlayerActivity that adds the title bar. This is now the super class for most
activities.

Other updates include:
* Rearrange layouts and base classes to better handle new page types (no title
bar, grey title bar, blue title bar, search results)
* New resources, layouts and styles
* Search button added to logo bar
* Search device-button wired up with NPR search box (and default options)
* Search results has search box at the top
* Navigation button on logo bar (nav menu still needs work)
* News story lists now have teaser and image and only show icon when audio
exists

[Michael Frederick, 2011-02-16 20:31:29]

Looks great! Mostly stylistic concerns.

Also, where are the images for medium dpi devices? If we want this to be
backwards compatible for older devices, that's necessary.

http://codereview.appspot.com/4178044/diff/15001/Npr/res/layout/search_box.xml
File Npr/res/layout/search_box.xml (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/res/layout/search_box.xm...
Npr/res/layout/search_box.xml:3: <!--
No need to add copyrights to layout files, I think.

http://codereview.appspot.com/4178044/diff/15001/Npr/res/xml/searchable.xml
File Npr/res/xml/searchable.xml (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/res/xml/searchable.xml#n...
Npr/res/xml/searchable.xml:2: 
And here.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/ImageThreadLoader.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/ImageThreadLoader.java:37: private final
HashMap<String, SoftReference<Drawable>> Cache =
Please initialize in the constructor, and for all member vars.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/ImageThreadLoader.java:37: private final
HashMap<String, SoftReference<Drawable>> Cache =
Cache and Queue should be lowercase.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/ImageThreadLoader.java:38: new HashMap<String,
SoftReference<Drawable>>();
Continuation indents should be 4 spaces, not 2.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/ImageThreadLoader.java:77: public void run() {
@Override

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/ImageThreadLoader.java:84: // Use a handler to get
back onto the UI thread for the update
For readability, I would put the main blocks in the if/else body into private
methods.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/Main.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/Main.java:125: params);
Indentation is wrong here also. If you use Eclipse, I provided a convenient
formatter file on the code site.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/NavigationView.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/NavigationView.java:36: private class SubActivity {
Eventually, the Main class can just forward to Top Stories. You don't have to do
that in this review, but the sooner, the better to avoid code duplication.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/NewsListActivity.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/NewsListActivity.java:74: // todo: move this to a
layout?
TODO
makes it easier to grep out.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/NewsListActivity.java:80: *
getResources().getDisplayMetrics().density);  // DIP
Nothing specific to news stories here. Maybe this should get pulled to the super
class?

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/NewsListAdapter.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/NewsListAdapter.java:142: if( cachedImage != null )
{
spacing is weird. Should be:
if (cached ... null) {

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/PlayerActivity.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/PlayerActivity.java:38: *         <p/>
This is weird...

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/PlayerActivity.java:167:
navigationView.setVisibility(View.VISIBLE);
Use braces for all conditional blocks.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/SearchActivity.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/SearchActivity.java:58: if( mgr != null ) {
Fix spacing.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/TitleActivity.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/TitleActivity.java:36: titleText =
(TextView)findViewById(R.id.TitleText);
Space after paren.

http://codereview.appspot.com/4178044/diff/15001/Npr_Test/src/org/npr/android...
File Npr_Test/src/org/npr/android/test/HttpServer.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr_Test/src/org/npr/android...
Npr_Test/src/org/npr/android/test/HttpServer.java:202: try
No newline between try and brace.

[jeremy.wadsack, 2011-02-17 18:07:58]

Thanks for the review. Changes will be applied with next update. 

I'll get MDPI images in there eventually. Currently working on getting layouts
and stuff first, but that's definitely on the punch list.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/NewsListActivity.java (right):

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/NewsListActivity.java:74: // todo: move this to a
layout?
grep -i :)

On 2011/02/16 20:31:29, Michael Frederick wrote:
> TODO
> makes it easier to grep out.

http://codereview.appspot.com/4178044/diff/15001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/NewsListActivity.java:80: *
getResources().getDisplayMetrics().density);  // DIP
This is specific to news stories. If you look at the design spec news stories
(mostly) have a blue title bar while program lists (for example) have a grey
title bar. In any case, we're talking to JP about this because I think the
design needs more consistency across all views.

On 2011/02/16 20:31:29, Michael Frederick wrote:
> Nothing specific to news stories here. Maybe this should get pulled to the
super
> class?

[jeremy.wadsack, 2011-02-28 18:35:45]

Next round of changes

[jeremy.wadsack, 2011-02-28 18:53:27]

This patch covers work-to-date on the 2.0 changes.

Here is the entire change log:

   - Search device-button now opens a search dialog (with default dates)
   - Navigation is from the logo bar menu, rather than the home page
   - Start page is Top News
   - Search shows the search box on the results page
   - Navigation has headers (which are now blue)
   - Search device-button on search page performs the search
   - Story lists now include image, title, teaser and whether or not they
   are playable
   - Audio stories in lists support swipe to add to playlist and long-press
   to play immediately
   - All Programs list shows On Air for programs that are currently being
   broadcast somewhere
   - Player, when docked now shows the currently playing item and the
   play/pause button
   - Expanded player includes play/pause, next track, previous track and rev
   30 seconds and a scrubber to move the playback point for non-streams.
   - Expanded player includes the play list
   - Playlist items show a green dot when not yet played, the player icon
   when playing and no icon when they have been played
   - Touching a playlist item starts it playing
   - Device back button closes the expanded player when open
   - Playlist shows story length
   - Expanded player has buttons to clear all playlist items or clear played
   items
   - About page has links to privacy, terms and feedback
   - Individual program list shows the stories for the program with a button
   to find live streams
   - Individual program has a button to add all stories to the playlist
   - Story pages can be swiped left or right to go back or forwards in the
   list of stories from the previous view
   - The story image for individual stories is inset into the text
   - Stories list the byline, dateline and length for audio stories



--
Jeremy Wadsack


On Mon, Feb 28, 2011 at 10:35 AM, <jeremy.wadsack@gmail.com> wrote:

> Next round of changes
>
> http://codereview.appspot.com/4178044/
>

[Michael Frederick, 2011-03-01 20:33:08]

This change is starting to get unwieldy. I'd like to submit this soon, since I'm
losing track of what the changes are.

Can we back out the major changes to NewsList* here?

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/Constants.java (right):

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/Constants.java:29: public static final String
EXTRA_PUBIC_BROADCASTING_QUERY_URL =
PUBLIC
:-)

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/DownloadDrawable.java (right):

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/DownloadDrawable.java:45: if (USE_BITMAP) {
Huh?

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/DownloadDrawable.java:114: if( is != null )
Weird formatting.

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
File Npr/src/org/npr/android/news/NewsListActivity.java (right):

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/NewsListActivity.java:35: import
org.npr.android.util.DisplayUtils;
I haven't seen this yet... Should it be added to this patch?

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
Npr/src/org/npr/android/news/NewsListActivity.java:37: import
org.npr.android.util.PlaylistRepository;
Or this.

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
File Npr/src/org/npr/android/util/FileUtils.java (right):

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
Npr/src/org/npr/android/util/FileUtils.java:19: 128
This feels like a pretty small value. Any justification for it?

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
File Npr/src/org/npr/android/util/PlaylistEntry.java (right):

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
Npr/src/org/npr/android/util/PlaylistEntry.java:27: public static final String
ENTRY_ID = "ID";
The case for these should be consistent. lower case + camelCase?

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
Npr/src/org/npr/android/util/PlaylistEntry.java:36: int order) {
Lining up params is annoying to maintain. :-)

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
Npr/src/org/npr/android/util/PlaylistEntry.java:50: public PlaylistEntry(Intent
intent) {
Rather than a separate constructor, I'd rather have a static factory method.

[jeremy.wadsack, 2011-03-02 01:42:18]

Michael -

Thank you for taking time to dig through this.

This change is starting to get unwieldy. I'd like to submit this soon,
> since I'm losing track of what the changes are.
>

I don't think we can commit to svn until NPR/Google figure out how to handle
the copyright issue. I'll verify that with JP, though. I would prefer to
commit this in small chunks as well as it's is a huge set of changes.

In fact, it's so big that I didn't do a proper review before sending it,
which I apologize for. Clearly you are missing some files (DisplayUtils,
PlaylistRepository), although I'm not sure why they didn't get added (maybe
I forgot to svn add them). Without those files some of the changes aren't
going to make sense.

I also left some test code in there when I added some of my own code to
DownloadDrawable. I'll remove that and fix the formatting items you
mentioned for another patch.

Can we back out the major changes to NewsList* here?
>

The major changes to NewsListAdapter is adding support for images (which
have to be downloaded on a separate thread and added as received to views
that may or may not be visible and may be cached).

NewsListActivity major changes are all around animations that we talked
through with JP and the NPR designers (stuff beyond the scope of the spec).
Basically a swipe left will add the item to the playlist and a long-press
will play it immediately. The animations are working but not ideal. But I
couldn't figure out ways to make them more efficient. So if you have
experience there, I'd love some pointers.

For "backing out" do you want me to revert these files to the current trunk
for the first review and then have us patch those in later? I guess I'm not
sure what that's gaining us, especially if we have time to review this while
NPR/Google figure out their copyright stuff.


--
Jeremy Wadsack


On Tue, Mar 1, 2011 at 12:33 PM, <mfrederick@google.com> wrote:

> This change is starting to get unwieldy. I'd like to submit this soon,
> since I'm losing track of what the changes are.
>
> Can we back out the major changes to NewsList* here?
>
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
> File Npr/src/org/npr/android/news/Constants.java (right):
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
> Npr/src/org/npr/android/news/Constants.java:29: public static final
> String EXTRA_PUBIC_BROADCASTING_QUERY_URL =
> PUBLIC
> :-)
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
> File Npr/src/org/npr/android/news/DownloadDrawable.java (right):
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
> Npr/src/org/npr/android/news/DownloadDrawable.java:45: if (USE_BITMAP) {
> Huh?
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
> Npr/src/org/npr/android/news/DownloadDrawable.java:114: if( is != null )
> Weird formatting.
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
> File Npr/src/org/npr/android/news/NewsListActivity.java (right):
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
> Npr/src/org/npr/android/news/NewsListActivity.java:35: import
> org.npr.android.util.DisplayUtils;
> I haven't seen this yet... Should it be added to this patch?
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/news...
> Npr/src/org/npr/android/news/NewsListActivity.java:37: import
> org.npr.android.util.PlaylistRepository;
> Or this.
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
> File Npr/src/org/npr/android/util/FileUtils.java (right):
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
> Npr/src/org/npr/android/util/FileUtils.java:19: 128
> This feels like a pretty small value. Any justification for it?
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
> File Npr/src/org/npr/android/util/PlaylistEntry.java (right):
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
> Npr/src/org/npr/android/util/PlaylistEntry.java:27: public static final
> String ENTRY_ID = "ID";
> The case for these should be consistent. lower case + camelCase?
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
> Npr/src/org/npr/android/util/PlaylistEntry.java:36: int order) {
> Lining up params is annoying to maintain. :-)
>
>
>
http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
> Npr/src/org/npr/android/util/PlaylistEntry.java:50: public
> PlaylistEntry(Intent intent) {
> Rather than a separate constructor, I'd rather have a static factory
> method.
>
>
> http://codereview.appspot.com/4178044/
>

[jeremy.wadsack, 2011-03-02 01:43:27]

Some specific line-item comments below.

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
File Npr/src/org/npr/android/util/FileUtils.java (right):

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
Npr/src/org/npr/android/util/FileUtils.java:19: 128
On 2011/03/01 20:33:08, Michael Frederick wrote:
> This feels like a pretty small value. Any justification for it?

 It's used it two places. Reading the API Key and reading the EULA, but both of
those are res/raw resource files, so I agree that this is a bad design. Perhaps
better to create something semantically like a text/string resource that works
like getString. Would it be better to add it to the RootActivity? Following the
Android framwork?

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
File Npr/src/org/npr/android/util/PlaylistEntry.java (right):

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
Npr/src/org/npr/android/util/PlaylistEntry.java:36: int order) {
On 2011/03/01 20:33:08, Michael Frederick wrote:
> Lining up params is annoying to maintain. :-)

Actually, this is just done by the IntelliJ setting, so I'm not bothering to
"maintain" it. But I can change our settings so that we're using standard 4
space indents for wrap here.

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
Npr/src/org/npr/android/util/PlaylistEntry.java:50: public PlaylistEntry(Intent
intent) {
On 2011/03/01 20:33:08, Michael Frederick wrote:
> Rather than a separate constructor, I'd rather have a static factory method.

We can do that. It's actually only used once in the PlaybackService so we could
inline it too. Unless you think it'll be useful in the future for other classes.

[jeremy.wadsack, 2011-03-05 01:57:57]

Latests changes, including added files

[jeremy.wadsack, 2011-03-05 01:58:06]

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
File Npr/src/org/npr/android/util/FileUtils.java (right):

http://codereview.appspot.com/4178044/diff/26001/Npr/src/org/npr/android/util...
Npr/src/org/npr/android/util/FileUtils.java:19: 128
On 2011/03/01 20:33:08, Michael Frederick wrote:
> This feels like a pretty small value. Any justification for it?

Just realized you were referring to the value 128. I added that because the the
BufferedReader throws a warning without a default value. Although the EULA is
larger than 128 bytes. I'll make it 8k which is the default.

[jeremy.wadsack, 2011-03-05 01:59:22]

Actually, this still doesn't have the new files. I'll take care of that
Monday.

--
Jeremy Wadsack


On Fri, Mar 4, 2011 at 5:57 PM, <jeremy.wadsack@gmail.com> wrote:

> Latests changes, including added files
>
>
> http://codereview.appspot.com/4178044/
>

[jeremy.wadsack, 2011-03-08 19:40:13]

Michael - 

This new patch set now has all the files (it looks like) and I spent some time
going through them to verify and review. As I mentioned in the email with Marsh,
this is really too much to look at at once pass. I'm just not sure the best way
to approach this. Here are the options that I see, but let me know your
thoughts.

1. I can submit 5-6 files at a time for you review. Pro: fewer things to review
at once. Con: some references to other files won't be in the review set.

2. We can work with this patch and just address parts of it together. Pro: all
referenced files are there. Con: Hard to remember what we've looked at and what
we haven't reviewed yet.

3. You can review my GitHub log with all commits. Pro: smaller commits. Cons:
There are over 100 commits and some things that were committed over the last
month have been changed, backed out or removed.