ipince / codepath

Repo to hold codepath bootcamp code.
0 stars 0 forks source link

[Android Bootcamp] Review TwitterClient, part 2 #5

Open ipince opened 10 years ago

ipince commented 10 years ago

Hello.

I've been very short on time this week, so here I only have the very very bare minimum, but I wanted to submit something.

I'll keep working on it for another hour or so, so maybe it'll be less sucky by the time you get to review it :)

/cc @nesquena

nesquena commented 10 years ago

:+1: nice work overall. A few notes after checking out the code:

Here's a detailed Project 4 Feedback Guide here which covers the most common issues with this submitted project. Read through the feedback guide point-by-point to determine how you might be able to improve your submission.

This week (Week 5), we are going to cover the last major piece to the Android puzzle and that is using the hardware and SDK components such as the camera, photo gallery, location, maps, etc. After that, Week 6 and week 7 we will be covering a few important intermediate topics such as more about styling and animation as well as testing.

Week 9 (March 10th), we are going to have a demo day to celebrate the progress you've all made with our next batch of Android students and multiple companies attending to see the group projects that you all have built. We are going to help however we can over the next few weeks to get the team project apps in shape for that.

I've been very short on time this week, so here I only have the very very bare minimum, but I wanted to submit something.

For what it's worth you did a good job even though you don't have many optional features. Hope you get a chance to do the optional tasks and UI chroming at some point though which provides a lot of learning opportunities.

ipince commented 10 years ago

Thanks for the comments!

I'm actually in the middle of adding progress bars and have a question. I know a Fragment can add items to the action bar, but it seems it cannot set the progress bar directly. The straightforward solution is to go through getActivity(). While we want to steer away from getActivity(), using it for this seems OK since setProgressBarVisible() is a method from Activity, so there's no "extra knowledge" the Fragment must know about its parent activity. Let me know if there's a better way.

Also, I was wondering what to do when multiple requests are in progress (e.g. in the Profile view, there may be 1 request for the user details and one for the user timeline). In that case, we'd prefer a counter on the progress bar, which makes the progress bar be enabled if the counter > 0. Is there a better way? (I didn't see any built-in functionality for this).

Thanks!

Hmm.. I might as well ask another question while I'm here: did you see the note in my TimelineFragment?: https://github.com/ipince/codepath/blob/master/TwitterClient/src/com/ipince/android/twitter/fragment/TimelineFragment.java#L81

Basically:

In my Activity, I want to call fragment.getAdapter().addListener(). However, the adapter is only created after Fragment.onCreate() is called. I couldn't find a place to put my code in the Activity that would be guaranteed to be called after all its children fragments had been created (sort of like html's "onload"). This was also trickier due to the tabbing; e.g. the Mentions fragment will not ever call onCreate() until after the user actually clicks on the tab, at which point the Activity has no interaction with it (even trying to call it inside onTabSelected() is too early). Thoughts?

nesquena commented 10 years ago

While we want to steer away from getActivity(), using it for this seems OK since setProgressBarVisible() is a method from Activity, so there's no "extra knowledge" the Fragment must know about its parent activity. Let me know if there's a better way.

I'm glad you caught this. The "right" way is probably the fragment listener where the fragment fires a onProgressStateChange event since not all activities have the progress bar feature enabled or might want to display the progress bar in different ways depending on the activity. Another "right" way is not to use the actionbar and embed the progressbar as a view as a part of the fragment for example as a footer in the listview. Either of these approaches would be preferred.

Also, I was wondering what to do when multiple requests are in progress (e.g. in the Profile view, there may be 1 request for the user details and one for the user timeline). In that case, we'd prefer a counter on the progress bar, which makes the progress bar be enabled if the counter > 0. Is there a better way?

You are quite right. One easy way to do this (there are many approaches) is store the number of requests in the "tag" of the progress bar. Each time decreasing the tag when a request completes and incrementing when one is sent. If the requests are > 0 then don't hide the bar when the request finishes. You could also maintain that count elsewhere.

In my Activity, I want to call fragment.getAdapter().addListener(). Thoughts?

There's a few ways to deal with this. I am curious why the activity is adding the listener? Nonetheless, the best way to deal with this is manually using a fragment listener. This is good to learn because its the best way to communicate to the activity. You could have a onLoaded event fired after everything is ready.

Let me know if that helps.

ipince commented 10 years ago

Thanks for the quick reply. Yes, using fragment listeners would work in these cases.

I am curious why the activity is adding the listener?

I just didn't want my Adapter/Fragment to know about any other activity. Instead it would just propagate the onClick event up the stack and let its parent(s) handle it (hierarchy being Activity->Fragment->Adapter). This also allows me to avoid the recursive profile view problem by having ProfileActivity simply not attach a listener.

nesquena commented 10 years ago

I just didn't want my Adapter/Fragment to know about any other activity. Instead it would just propagate the onClick event up the stack and let its parent(s) handle it (hierarchy being Activity->Fragment->Adapter). This also allows me to avoid the recursive profile view problem by having ProfileActivity simply not attach a listener.

I see, that can make sense. listener should solve these use cases perfectly.