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

QuickContact in GroupMembersDialog #3033

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@chr1shaefn3r
Contributor

chr1shaefn3r commented Apr 22, 2015

Added onClickListener to the GroupMembersDialog which gives the same functionality as taps on avatars.

Fixes #2837

@mcginty

This comment has been minimized.

Contributor

mcginty commented Apr 29, 2015

At least in Gingerbread, clicking on a group contact results in a NPE so marking this as higher priority as it solves a crash bug.

@mcginty mcginty added this to the 2.14.0 milestone Apr 29, 2015

}
}
GroupMembers groupMembers = new GroupMembers(context, members);

This comment has been minimized.

@moxie0

moxie0 Apr 29, 2015

Member

i think it's fine the way it is, we don't need a whole new class that is another way to represent recipients

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Apr 30, 2015

Contributor

The problem I needed to solve was to match the index I get from the Dialog via the onClick callback to the correct recipient (this was not the case with the current implementation as the String[] was disconnect from the List).
Easy solution would be to not put 'Me' on top, but I liked that feature.
Therefore somebody needs to handle the reordering of the List, giving out the proper String[] and then be able to retrieve the correct Recipient via index.
To me this represents one responsiblity, so I put it in a new class.
(I must admit, this looks like an awful lot of code, for just implementing the onClick)

AlertDialogWrapper.Builder builder = new AlertDialogWrapper.Builder(context);
builder.setTitle(R.string.ConversationActivity_group_members);
builder.setIconAttribute(R.attr.group_members_dialog_icon);
builder.setCancelable(true);
builder.setItems(recipientStrings.toArray(new String[]{}), null);
builder.setItems(groupMembers.getRecipientStrings(), new GroupMembersOnClickListener(context, groupMembers));

This comment has been minimized.

@moxie0

moxie0 Apr 29, 2015

Member

make the listener an inner class

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Apr 30, 2015

Contributor

done

@@ -89,6 +91,10 @@ public void onFailure(Throwable error) {
this.contactPhoto = contactPhoto;
}
public synchronized boolean isAPhonebookContact() {
return (this.contactUri != null);
}

This comment has been minimized.

@moxie0

moxie0 Apr 29, 2015

Member

take this out

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Apr 30, 2015

Contributor

done

intent.putExtra(ContactsContract.Intents.Insert.PHONE, getNumber());
intent.setType(ContactsContract.Contacts.CONTENT_ITEM_TYPE);
return intent;
}

This comment has been minimized.

@moxie0

moxie0 Apr 29, 2015

Member

this is a model, it shouldn't know anything about intents

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r Apr 30, 2015

Contributor

agreed, moved to RecipientUtil to keep the creation of this intent in one place.

*
* @author: Christoph Haefner
*/
public class GroupMembers {

This comment has been minimized.

@moxie0

moxie0 Apr 30, 2015

Member

make it an inner class

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r May 1, 2015

Contributor

done

public class GroupMembers {
private static final String TAG = GroupMembers.class.getSimpleName();
private LinkedList<Recipient> sortedMembers;

This comment has been minimized.

@moxie0

moxie0 Apr 30, 2015

Member

final and initialize

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r May 1, 2015

Contributor

done

} else {
sortedMembers.add(recipient);
}
}

This comment has been minimized.

@moxie0

moxie0 Apr 30, 2015

Member

put this in the constructor

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r May 1, 2015

Contributor

In my opinion its not particular obvious what this specific combination of List.push and List.add does, hence the method with the descriptive name 'sortMeOnTop'.
I'm curious: What is your reasoning behind making sortedMembers final and initialize it in the constructor by moving the code from this private method into the constructor?

And of course: done

}
}
private List<String> createListOfRecipientStrings() {

This comment has been minimized.

@moxie0

moxie0 Apr 30, 2015

Member

doesn't look like you need a separate private method for this

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r May 1, 2015

Contributor

done

private class GroupMembers {
private final String TAG = GroupMembers.class.getSimpleName();
private final LinkedList<Recipient> sortedMembers;

This comment has been minimized.

@moxie0

moxie0 May 1, 2015

Member

members = new LinkedList<>();

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r May 4, 2015

Contributor

done

sortedMembers = new LinkedList<>();
List<Recipient> recipientList = recipients.getRecipientsList();
for (Recipient recipient : recipientList) {

This comment has been minimized.

@moxie0

moxie0 May 1, 2015

Member

for (Recipient recipient : recipients.getRecipientsList())

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r May 4, 2015

Contributor

done

return sortedMembers.get(index);
}
private List<String> createListOfRecipientStrings() {

This comment has been minimized.

@moxie0

moxie0 May 1, 2015

Member

you don't need a private method for this

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r May 4, 2015

Contributor

done

/**
* @author Christoph Häfner
*/
public class RecipientUtil {

This comment has been minimized.

@moxie0

moxie0 May 1, 2015

Member

we don't need a whole new class for this

This comment has been minimized.

@chr1shaefn3r

chr1shaefn3r May 4, 2015

Contributor

I'm out of idea where to put this, therefore I removed it.

@rhodey

This comment has been minimized.

Contributor

rhodey commented May 7, 2015

clicking on a group contact still results in an NPE, Nexus 5 Lollipop 5.1, built from today's master 356d994. higher priority still for sure.

05-07 10:45:56.609  23925-23925/org.thoughtcrime.securesms.sushi D/AndroidRuntime﹕ Shutting down VM
05-07 10:45:56.609  23925-23925/org.thoughtcrime.securesms.sushi E/AndroidRuntime﹕ FATAL EXCEPTION: main
    Process: org.thoughtcrime.securesms.sushi, PID: 23925
    java.lang.NullPointerException: Attempt to invoke interface method 'void com.afollestad.materialdialogs.MaterialDialog$ListCallback.onSelection(com.afollestad.materialdialogs.MaterialDialog, android.view.View, int, java.lang.CharSequence)' on a null object reference
            at com.afollestad.materialdialogs.MaterialDialog.onItemClick(MaterialDialog.java:169)
            at android.widget.AdapterView.performItemClick(AdapterView.java:305)
            at android.widget.AbsListView.performItemClick(AbsListView.java:1146)
            at android.widget.AbsListView$PerformClick.run(AbsListView.java:3053)
            at android.widget.AbsListView$3.run(AbsListView.java:3860)
            at android.os.Handler.handleCallback(Handler.java:739)
            at android.os.Handler.dispatchMessage(Handler.java:95)
            at android.os.Looper.loop(Looper.java:135)
            at android.app.ActivityThread.main(ActivityThread.java:5254)
            at java.lang.reflect.Method.invoke(Native Method)
            at java.lang.reflect.Method.invoke(Method.java:372)
            at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:903)
            at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:698)
05-07 10:45:57.770  23925-23985/org.thoughtcrime.securesms.sushi W/WebSocketConnection﹕ Sending keep alive...

@moxie0 moxie0 closed this in 417a4b8 May 7, 2015

pR0Ps added a commit to SilenceIM/Silence that referenced this pull request May 8, 2015

@chr1shaefn3r chr1shaefn3r deleted the chr1shaefn3r:feature/QuickContactInGroupMembersDialog branch Jul 1, 2015

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