Skip to content
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 chatBox to directly reply. #45

Closed
wants to merge 21 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@kunall17
Copy link
Collaborator

kunall17 commented Apr 8, 2016

Features covered in this PR (#8)-

  • Send message without dialog box
  • Tap on the chat message textView to select the subject or person in the chatBox and send message
  • Tap on the header tile to narrow and select the subject or person in the chatBox
  • Switch views for the Stream and Private.

ezgif-4210950663

@smarx

This comment has been minimized.

Copy link

smarx commented Apr 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.

Copy link
Contributor

niftynei commented May 10, 2016

@kunall17 this looks really cool. if you can get it rebased to master, I'll give it a look. 👍

@kunall17 kunall17 force-pushed the kunall17:patch-3 branch from 184016b to 92bcbdc May 18, 2016

@kunall17

This comment has been minimized.

Copy link
Collaborator Author

kunall17 commented May 18, 2016

@niftynei sorry for the late rebase, did it now..:)


if (isCurrentModeStream()) {
if (TextUtils.isEmpty(streamActv.getText().toString())) {
streamActv.setError("No Stream specified.");

This comment has been minimized.

@niftynei

niftynei May 22, 2016

Contributor

Any user message strings belong in R.strings. Same for other error messages in this PR.

switchToStream();
streamActv.setText(message.getStream().getName());
topicActv.setText(message.getSubject());
if (message.getSubject().equals("")) topicActv.requestFocus();

This comment has been minimized.

@niftynei

niftynei May 22, 2016

Contributor

is it also possible for the subject to be null? note that a more Java-safe way to write this is:

if ("".equals(message.getSubject()) ...

This comment has been minimized.

@kunall17

kunall17 May 23, 2016

Author Collaborator

Subject shouldn't be null here, as in the else case this is for stream's anyways I'll then also replace the code! 👍

classpath 'com.android.tools.build:gradle:2.1.0-rc1'
classpath 'com.google.gms:google-services:2.1.0-rc1'
classpath 'com.android.tools.build:gradle:2.1.0'
classpath 'com.google.gms:google-services:2.1.0'

This comment has been minimized.

@niftynei

niftynei May 22, 2016

Contributor

👍

import android.widget.AutoCompleteTextView;
import android.widget.FilterQueryProvider;
import android.widget.ImageView;
import android.widget.Toast;

This comment has been minimized.

@niftynei

niftynei May 22, 2016

Contributor

can you merge this commit in with the commits that introduced these imports?


<!-- The navigation drawer -->

This comment has been minimized.

@niftynei

niftynei May 22, 2016

Contributor

This comment may be worth preserving?

android:layout_alignEnd="@+id/list_fragment_container"
android:layout_alignRight="@+id/list_fragment_container"
android:layout_alignTop="@+id/message_et"
android:src="@drawable/ic_send_black_24dp"

This comment has been minimized.

@niftynei

niftynei May 22, 2016

Contributor

it's best practice to introduce the drawable in the same commit that you use it in, or at least before the commit that you use the drawable in. that makes it easier to roll back the change if need be.

if (isCurrentModeStream()) {
togglePrivateStreamBtn.setImageDrawable(ContextCompat.getDrawable(this, R.drawable.ic_action_bullhorn));
streamActv.setText(null);
streamActv.setHint("Person");

This comment has been minimized.

@niftynei

niftynei May 22, 2016

Contributor

Any user visible text belongs in R.strings

AsyncSend sender = new AsyncSend(that, msg);
sender.setCallback(new ZulipAsyncPushTask.AsyncTaskCompleteListener() {
public void onTaskComplete(String result, JSONObject jsonObject) {
Toast.makeText(ZulipActivity.this, "Message Sent!", Toast.LENGTH_SHORT).show();

This comment has been minimized.

@niftynei

niftynei May 22, 2016

Contributor

Any user visible text belongs in R.strings.

@kunall17 kunall17 force-pushed the kunall17:patch-3 branch from 92bcbdc to c32bd78 May 23, 2016

@kunall17

This comment has been minimized.

Copy link
Collaborator Author

kunall17 commented May 23, 2016

Did the edits..:)

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented May 24, 2016

I finally got around to running it. It took some getting used to, but I'm pretty into this. Here's a few things I noticed:

  1. the reply box is showing right when I log in. Can it be hidden until the stream finishes loading?
  2. Clicking on a message to reply fills in the boxes nicely and moves my cursor but doesn't bring up the keyboard to start typing.
  3. When pushing the up navigation arrow, the reply box stays shown with the last stream's topic and info. Can we clear out the stream,topic and message if a user presses up or back, especially if the message reply box is empty? If there's text, we might show a confirm dialog (and let them know that going back will delete their unsent message)
  4. The Person/Topic message toggle on the reply box is nice, but is incorrectly showing the Topic icon when it first opens; toggling it a few times seems to unstick it.
  5. Toggling between a person and a topic erases the stream, but not the topic. (see below)
  6. There's no immediate feedback that a message has been posted. I can't tell whether my message got sent or not; I ended up posting a message twice on accident because of this. The current message dialog boxes grey out the text and show a progress spinner -- this should do something similar.
  7. The action bar still has the private message/stream message buttons in it. This is kind of confusing. Is there a reason you didn't get rid of them?
  8. This is a nitpick, but can we make the Person edittext fill the space (see below)?

screenshot_20160524-091313
screenshot_20160524-091200

streamActv.setHint(R.string.hint_stream);
topicActv.setVisibility(View.VISIBLE);
textView.setVisibility(View.VISIBLE);
textView.setText(" > ");

This comment has been minimized.

@niftynei

niftynei May 24, 2016

Contributor

Might be worth moving to R.strings? This is optional.

@@ -50,5 +50,8 @@
<string name="username_required">Username required</string>
<string name="server_domain_required">Server domain required</string>
<string name="invalid_domain">Invalid domain</string>
<string name="hint_person">Person</string>
<string name="hint_subject">Subject</string>
<string name="hint_stream">Stream</string>

This comment has been minimized.

@niftynei

niftynei May 24, 2016

Contributor

👍

@kunall17 kunall17 force-pushed the kunall17:patch-3 branch 3 times, most recently from 2f4f8c0 to 384fd7b May 27, 2016

@kunall17

This comment has been minimized.

Copy link
Collaborator Author

kunall17 commented May 27, 2016

@niftynei updated this branch!
Now it does

  • Brings up the keyboard on tapping message
  • Fixed switching icon Bug.
  • Now stores the previous stream name while switching.
  • Shown Loading screen as the compose dialog box.
  • Person EditText space is now filled.
  • When Navigation Up arrow is pressed it clears only if the message is Empty
    I had to discuss on this, Should we display a confirm dialog if message box is not empty?
    This can be annoying sometimes to the user!

For the Compose dialog icons in the action Bar I thought let it be there, if someone prefers that way they can go for it. But basically both are same.
Should I remove them?

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented May 31, 2016

yeah, let's remove the compose dialog icons. 👍

}

private void sendingMessage(Boolean isSending) {

This comment has been minimized.

@niftynei

niftynei May 31, 2016

Contributor

this can be the lower case boolean

composeStatus.setVisibility(View.GONE);
}

SimpleCursorAdapter streamActvAdapter;

This comment has been minimized.

@niftynei

niftynei May 31, 2016

Contributor

best practice is to put these declarations at the top of the file with all other member variables.

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented May 31, 2016

@kunall17 this looks great. I am +1 on leaving out the clear dialog on back press. There's a few more things that we might be able to do to make it a bit more user friendly, but nothing that's blocking. They're also the sort of thing that we can fix/modify later.

I just noticed that there's an extra amount of padding around the message list. can you get rid of this?

screen shot 2016-05-31 at 13 22 16

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented Jun 1, 2016

This crash occurred while starting the app (clicking icon from launcher)

06-01 10:56:46.231 E/AndroidRuntime( 7435): java.lang.RuntimeException: Unable to start activity ComponentInfo{com.zulip.android.dev/com.zulip.android.activities.ZulipActivity}: java.lang.NullPointerException: Attempt to invoke virtual method 'android.text.Editable android.widget.EditText.getText()' on a null object reference
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2416)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2476)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.app.ActivityThread.-wrap11(ActivityThread.java)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1344)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.os.Handler.dispatchMessage(Handler.java:102)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.os.Looper.loop(Looper.java:148)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.app.ActivityThread.main(ActivityThread.java:5422)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at java.lang.reflect.Method.invoke(Native Method)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
06-01 10:56:46.231 E/AndroidRuntime( 7435): Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'android.text.Editable android.widget.EditText.getText()' on a null object reference
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at com.zulip.android.activities.ZulipActivity.clearChatBox(ZulipActivity.java:723)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at com.zulip.android.activities.MessageListFragment.onDetach(MessageListFragment.java:264)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1202)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.support.v4.app.FragmentManagerImpl.removeFragment(FragmentManager.java:1349)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.support.v4.app.BackStackRecord.run(BackStackRecord.java:699)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.support.v4.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:1617)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.support.v4.app.FragmentManagerImpl.executePendingTransactions(FragmentManager.java:570)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at com.zulip.android.activities.ZulipActivity.pushListFragment(ZulipActivity.java:747)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at com.zulip.android.activities.ZulipActivity.onCreate(ZulipActivity.java:418)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.app.Activity.performCreate(Activity.java:6251)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1107)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2369)
06-01 10:56:46.231 E/AndroidRuntime( 7435):     ... 9 more

@kunall17 kunall17 force-pushed the kunall17:patch-3 branch from af2639e to 7243391 Jun 2, 2016

@niftynei niftynei closed this Jun 3, 2016

@niftynei niftynei reopened this Jun 3, 2016

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented Jun 4, 2016

@kunall17 this looks great, but I'm having trouble sending messages with it. The "Send" button seems to get stuck on disabled from time to time. If you can get this fixed up, we'll get this merged in.

@kunall17

This comment has been minimized.

Copy link
Collaborator Author

kunall17 commented Jun 19, 2016

@niftynei Updated with the Tests..:)

@kunall17 kunall17 force-pushed the kunall17:patch-3 branch 2 times, most recently from afbb0bc to 05c8dac Jun 20, 2016

@kunall17 kunall17 force-pushed the kunall17:patch-3 branch from 05c8dac to 49dc18c Jun 20, 2016

@kunall17

This comment has been minimized.

Copy link
Collaborator Author

kunall17 commented Jun 21, 2016

@niftynei Fixed the bug and tested upon this one now it doesn't blocks!

@niftynei

This comment has been minimized.

Copy link
Contributor

niftynei commented Jun 21, 2016

Tests are flakey, but implementation looks good to me. 👍

@niftynei niftynei closed this Jun 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.