New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented RecyclerView inplace of ListView + UI updated #81

Merged
merged 35 commits into from Aug 8, 2016

Conversation

Projects
None yet
3 participants
@kunall17
Collaborator

kunall17 commented Jul 8, 2016

Replaced the current listview with RecyclerView

Screenshot:
screenshot from 2016-07-07 18-53-38

GIF: [ I had no internet in this recording of the Gif so the gravatars are not loaded and they are shown as placeholders! ]
ezgif-4169072805

@smarx

This comment has been minimized.

smarx commented Jul 8, 2016

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 11, 2016

@kunall17 I haven't had much of a chance to look at the code today, but I'm having trouble seeing all of my messages. The app doesn't scroll all the way down, it seems to stop early. Cause uncertain.

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 11, 2016

Functional issues aside, It looks beautiful!

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 11, 2016

Replicated the issue by scrolling back fairly far and then scrolling down to the bottom again.

@niftynei

This comment has been minimized.

Contributor

niftynei commented Jul 12, 2016

Found another small problem: I can't refresh the list by pulling on it anymore. 🚨

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Jul 13, 2016

I was not able to replicate the issue in my build i have 500+ messages and on scrolling to the top of the recyclerView loads more messages.
I guess a swipeToRefresh layout will be perfect in this one, that should solve the problem of manually checking the scroll position and a toast message of no new messages would be great for the UX perspective!

public class MessageAdapter extends RecyclerView.Adapter<RecyclerView.ViewHolder> {
public static final int VIEWTYPE_MESSAGEHEADER = 1;

This comment has been minimized.

@niftynei

niftynei Jul 14, 2016

Contributor

This would be easier to read as VIEWTYPE_MESSAGE_HEADER

MessageAdapter(List<Message> messageList, final Context context, boolean startedFromFilter) {
super();
items = new ArrayList<>();
setupHeaderAndFooterViews();

This comment has been minimized.

@niftynei

niftynei Jul 14, 2016

Contributor

Unless there's a reason for doing this before, it's best practice to initialize fields before calling any methods. (Moving this line below line 69 would fix this).

app.setPointer(mID);
}
app.markMessageAsRead(message);

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

It looks like you've removed the functionality that marks messages as read on scroll. This is probably behavior worth preserving.

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

Update: I found the place you moved this to, see comment there.

android:layout_height="fill_parent" >
</ListView>
android:layout_height="fill_parent"

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

Committed as is, this changeset will break the app (cause runtime failures). Wherever possible, it's good practice to keep XML changes in the same commit as the Java code changes that you make as well.

MessageHeaderParent messageHeaderParent = new MessageHeaderParent((message.getStream() == null) ? null : message.getStream().getName(), message.getSubject(), message.getIdForHolder());
messageHeaderParent.setMessageType(message.getType());
messageHeaderParent.setDisplayRecipent(message.getDisplayRecipient(zulipApp));
if (message.getType() == MessageType.STREAM_MESSAGE)

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

This looks like it's missing the { } for the if statement. I know syntactically this is ok, but this block is a bit hard to read. It'd be more concise with them in this case.

Generally, any if statement that spans multiple lines should have brackets.

recipientsCache = new Person[ids.length];
for (int i = 0; i < ids.length; i++) {
recipientsCache[i] = Person.getById(app,
Integer.parseInt(ids[i]));

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

Integer.parseIntthrows NumberFormatException. It's unlikely, but it's a good idea to catch it.

View messageView = LayoutInflater.from(parent.getContext()).inflate(R.layout.message_tile, parent, false);
MessageHolder messageHolder = new MessageHolder(messageView);
return messageHolder;
}

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

Is there a default case? A case statement should always have a default.

public int getItemViewType(int position) {
if (items.get(position) instanceof MessageHeaderParent)
return VIEWTYPE_MESSAGE_HEADER;
else if (items.get(position) instanceof Message)

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

what's the difference between items.get(position) and getItem(postion)?

This comment has been minimized.

@kunall17

kunall17 Jul 31, 2016

Collaborator

No difference this function I created because this items list is being used in ZulipActivity.java

public void onCreateContextMenu(ContextMenu menu, View v, ContextMenu.ContextMenuInfo menuInfo) {
Message msg = onItemClickListener.getMessageAtPosition(getAdapterPosition());
onItemClickListener.setContextItemSelectedPosition(getAdapterPosition());
if (msg == null) {

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

is this a loggable offense?

This comment has been minimized.

@kunall17

kunall17 Jul 31, 2016

Collaborator

loggable offense , Didn't get you?

android:layout_height="wrap_content"
android:background="@drawable/triangle"
android:padding="4dp"
android:text=" " />

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

Is this necessary?

messageHolder.messageTile.setBackgroundColor(mDefaultPrivateMessageColor);
}
break;
}

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

What's the default case?

This comment has been minimized.

@kunall17

kunall17 Jul 31, 2016

Collaborator

default case won't be needed because in getItemViewType(int position) function we are throwing an exception if the viewType is not known!
https://github.com/kunall17/zulip-android/blob/patch-9-recyclerview/app/src/main/java/com/zulip/android/activities/RecyclerMessageAdapter.java#L181-L183

final MessageHeaderParent.MessageHeaderHolder messageHeaderHolder = ((MessageHeaderParent.MessageHeaderHolder) holder);
if (messageHeaderParent.getMessageType() == MessageType.STREAM_MESSAGE) {
messageHeaderHolder.streamTV.setText(messageHeaderParent.getStream());

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

This is very small thing. The abbreviation TV here is a bit confusing. Can we change it to TextView.

This comment has been minimized.

@kunall17

kunall17 Jul 31, 2016

Collaborator

Done..:)

messageHeaderHolder.streamTV.setBackgroundColor(messageHeaderParent.getColor());
if (messageHeaderParent.isMute())
messageHeaderHolder.muteMessageImage.setVisibility(View.VISIBLE);

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

Should wrap mulit-line if statements with {}.

}
int getItemIndex(Message message) {

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

is this public or private?

return items.indexOf(message);
}
Object getItem(int position) {

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

is this public or private?

public void onViewRecycled(RecyclerView.ViewHolder holder) {
super.onViewRecycled(holder);
if (holder.getItemViewType() == VIEWTYPE_MESSAGE)
markThisMessageAsRead((Message) getItem(holder.getAdapterPosition()));

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

This has the potential to cause bugs. views can be inflated & recycled without being shown in order to calculate the layout size / scroll distance.

Let's find a better trigger to mark messages as read.

This comment has been minimized.

@kunall17

kunall17 Jul 31, 2016

Collaborator

Changed to onViewAttachedToWindow(Holder) now it should function correctly!

this.startedFromFilter = startedFromFilter;
narrowListener = (NarrowListener) context;
this.startedFromFilter = startedFromFilter;

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

this line is now repeated (see line 58)

try {
int mID = message.getID();
if (!startedFromFilter && zulipApp.getPointer() < mID) {
Log.i("scrolling", "Now at " + mID);

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

This log message is outdated -- it's now in a recycle function instead of a scrolling function.

Recycle and scroll are tightly correlated but not exactly.

headerView = LayoutInflater.from(parent.getContext()).inflate(R.layout.list_loading, parent, false);
LoadingHolder headerLoadingHolder = new LoadingHolder(headerView);
isHeaderShowing(false);
return headerLoadingHolder;

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

What is the default case?

This comment has been minimized.

@kunall17

kunall17 Jul 31, 2016

Collaborator

default case won't be needed because in getItemViewType(int position) function we are throwing an exception if the viewType is not known!
https://github.com/kunall17/zulip-android/blob/patch-9-recyclerview/app/src/main/java/com/zulip/android/activities/RecyclerMessageAdapter.java#L181-L183

@@ -247,5 +274,35 @@ Object getItem(int position) {
return items.get(position);
}
private int getItemCount(boolean excludeFooter) {
if (excludeFooter) return getItemCount();

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

this logic seems backwards. If excludeFooter should return the whole count minus 1 (the footer).

Is there any guarantee that there's only 1 footer? If not, it might be best to write a function to return the number of footers.

This comment has been minimized.

@kunall17

kunall17 Jul 31, 2016

Collaborator

Yeah, footer and header are only one each which is the loading View!

else return getItemCount() - 1;
}
void isFooterShowing(boolean show) {

This comment has been minimized.

@niftynei

niftynei Jul 18, 2016

Contributor

is__ style functions typically return a boolean (ask a question). A better name for this function might be setFooterVisibility.

@kunall17

This comment has been minimized.

Collaborator

kunall17 commented Aug 3, 2016

@niftynei I was not able to recreate the problem (before or after the fix), but I identified an error which can cause these errors if there are many muted topics/streams. Hopefully this will fix this issue!

  • Rebased to current master as well!
@niftynei

This comment has been minimized.

Contributor

niftynei commented Aug 8, 2016

This looks great! I'm going to go ahead and merge it in.

There's a few small visual bugs I found:

  • If I add a new message to a stream, the sidebar color doesn't fill in until I scroll up and back.
  • Sometimes when scrolling, both the FAB and the reply message box show up.

@niftynei niftynei merged commit cd4b4d3 into zulip:master Aug 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment