Fixed typing status for typing from different device #802

smitbose · 2017-03-04T20:49:35Z

This fixes the bug #628 .

While writing from another device, logged in with the same JID, the current code shows a different user is typing. This commit fixes that to show status message "Typing from another device" and "Stopped typing on the other device" in those cases.
It however leaves the previous behaviour unchanged in case of other users.

smitbose · 2017-03-04T20:50:59Z

Also, @jcbrand, I was unsure of the particular the UI will be modified, so just kept the messages I felt would be suitable into the fix. Do review it and suggest improvements.

jcbrand · 2017-03-08T16:16:13Z

I didn't notice that you worked on the same ticket as @saganshul

I already merged his pull request. However, I like the change you made to show a different status message.

Can you please rebase your commits onto master, check whether the tests pass and then push again?

smitbose · 2017-03-08T16:33:10Z

okay, sure @jcbrand

smitbose · 2017-03-08T20:44:04Z

@jcbrand I've rebased the commits on master...

jcbrand · 2017-03-09T10:24:07Z

No, something went wrong.

If you look at the commits, there are now many more commits and two commits for Fixed typing status for typing from same device.

This is not how it's supposed to look after rebasing. Instead after rebasing there should just be your one commit to be merged into master.

smitbose · 2017-03-09T10:32:23Z

Yes, I also think something went wrong somewhere. Basically, my typing-status branch was outdated, and upstream/master was ahead by several commits because I haven't worked on this branch after making the PR.

So, with my updated master, and while being in typing-status branch, I did git rebase master, followed by a git push -u origin typing-status.

Where did this go wrong?

jcbrand · 2017-03-09T10:49:22Z

The oldest commit in this PR is a merge you made. I think that's the problem. I suspect because of your merge your master was different and so you rebased onto a changed master.

There are a few ways to fix this. I think easiest to explain is to do:

git rebase -i HEAD~13

And then drop all the commits before your last one. Then rebase again and push.

Make sure to back up your work before you do this.

jcbrand · 2017-03-10T13:46:22Z

src/converse-chatview.js

-                        this.showStatusNotification(message.get('fullname')+' '+__('is typing'));
+                        if(message.get('sender') === 'me') {
+                            this.showStatusNotification('Typing from another device');
+                        }


User-facing strings should be wrapped in __( ) so that they will get translated.

jcbrand · 2017-03-10T13:48:11Z

src/converse-chatview.js

@@ -532,7 +542,7 @@
                     * See XEP-0085 Chat State Notifications.
                     */
                    _converse.connection.send(
-                        $msg({'to':this.model.get('jid'), 'type': 'chat'})
+                        $msg({'to':this.model.get('jid'), 'type': 'chat', 'from': _converse.connection.jid})


I don't think this 'from' attribute is necessary because it gets added automatically by strophe.js AFAICR. You can remove it. Unless you have a clear reason why it's needed.

smitbose · 2017-03-12T12:41:47Z

@jcbrand This one is still lying unreviewed. If the changes are ok, I'll move on to writing tests and updating changelogs.

jcbrand · 2017-03-12T18:31:32Z

@smitbose yes please

smitbose · 2017-03-12T20:12:16Z

@jcbrand I was wondering what kind of test sould I write for this one. Basically, the way I'm testing this is openning up chatbox for the same receiver from two different resources and then trying to type something, and checking the typing status rendered. How can I simulate the same behaviour in a spec. Can you help me out here?

jcbrand · 2017-03-13T08:41:49Z

Open a chat box with a contact. Then receive a composing message for the currently logged in user (not from the contact), but from a different resource.

jcbrand · 2017-03-13T08:55:37Z

@smitbose You can copy/paste this test and modify it to send a composing carbon message instead of just a normal carbon message.

smitbose · 2017-03-13T09:40:17Z

ok, right, thanks.

smitbose · 2017-03-13T11:26:47Z

@jcbrand what to do about the changelog, because it says that saganshul worked on the same issue, i.e. #628 . Should I add another line referencing the same issue or should I override that line?

jcbrand · 2017-03-13T12:31:20Z

Add another line referencing the same issue.

smitbose · 2017-03-13T12:59:09Z

@jcbrand PR updated with final modifications, specs and changelog edits. Please review.

jcbrand · 2017-03-13T13:19:16Z

docs/CHANGES.md

@@ -3,6 +3,7 @@
 ## 3.0.1 (Unreleased)

 - #585 Duplicate contact created due to JID case sensivity [saganshul]
+- #628 Fixes typing status for typing from different devices. [smitbose]


This is not descriptive of what was done. It's not a bugfix, you simply changed the message that gets displayed. Also please put it after saganshul's message.

jcbrand · 2017-03-15T08:49:04Z

@smitbose Almost done! A few small changes, then I'll merge.

smitbose · 2017-03-15T17:23:43Z

@jcbrand I don't see any reviews asking for changes. I did the previous changes asked. Can you specify the small changes required?

jcbrand · 2017-03-15T08:45:44Z

spec/chatbox.js

+                    it("can be a composing carbon message that this user sent from a different client, as defined in XEP-0280", mock.initConverse(function (_converse) {
+                        test_utils.createContacts(_converse, 'current');
+                        test_utils.openControlBox();
+                        test_utils.openContactsPanel(_converse);


Lines 1399 and 1400 are not needed AFAIK.

jcbrand · 2017-03-15T08:46:41Z

spec/chatbox.js

+                        var chatbox = _converse.chatboxes.get(recipient_jid);
+                        var chatboxview = _converse.chatboxviews.get(recipient_jid);
+                        expect(chatbox).toBeDefined();
+                        expect(chatboxview).toBeDefined();


The above two checks aren't necessary within this test.

jcbrand · 2017-03-15T08:47:50Z

spec/chatbox.js

@@ -1393,6 +1393,47 @@
                        var $events = chatboxview.$el.find('.chat-event');
                        expect($events.text()).toEqual(mock.cur_names[1] + ' is typing');
                    }));
+
+                    it("can be a composing carbon message that this user sent from a different client, as defined in XEP-0280", mock.initConverse(function (_converse) {


Please remove the , as defined in XEP-0280 part

jcbrand · 2017-03-15T08:47:56Z

spec/chatbox.js

@@ -1477,6 +1518,47 @@
                        var $events = chatboxview.$el.find('.chat-event');
                        expect($events.text()).toEqual(mock.cur_names[1] + ' has stopped typing');
                    }));
+
+                    it("can be a paused carbon message that this user sent from a different client, as defined in XEP-0280", mock.initConverse(function (_converse) {


Please remove the , as defined in XEP-0280 part

jcbrand · 2017-03-15T08:48:11Z

spec/chatbox.js

+                    it("can be a paused carbon message that this user sent from a different client, as defined in XEP-0280", mock.initConverse(function (_converse) {
+                        test_utils.createContacts(_converse, 'current');
+                        test_utils.openControlBox();
+                        test_utils.openContactsPanel(_converse);


Lines 1524 and 1525 aren't needed AFAIK.

jcbrand · 2017-03-15T08:48:26Z

spec/chatbox.js

+                        var chatbox = _converse.chatboxes.get(recipient_jid);
+                        var chatboxview = _converse.chatboxviews.get(recipient_jid);
+                        expect(chatbox).toBeDefined();
+                        expect(chatboxview).toBeDefined();


Unnecessary checks in the context of this test.

jcbrand · 2017-03-16T10:58:17Z

Sorry, I forgot to submit my review.

smitbose · 2017-03-16T15:26:10Z

@jcbrand made final changes. Please check...

jcbrand · 2017-03-16T15:29:05Z

Looks good, thanks!

smitbose force-pushed the typing-status branch from acee363 to 2e582f5 Compare March 9, 2017 11:49

jcbrand reviewed Mar 10, 2017

View reviewed changes

smitbose force-pushed the typing-status branch from 2e582f5 to c600638 Compare March 10, 2017 18:50

smitbose changed the title ~~Fixed typing status for typing from same device~~ Fixed typing status for typing from different device Mar 12, 2017

smitbose force-pushed the typing-status branch from c600638 to 7055b50 Compare March 13, 2017 11:25

jcbrand reviewed Mar 13, 2017

View reviewed changes

jcbrand reviewed Mar 16, 2017

View reviewed changes

smitbose force-pushed the typing-status branch from 1f9f45e to 48ad81b Compare March 16, 2017 15:24

Fixed typing status for typing from different devices

48ad81b

jcbrand merged commit c976f3f into conversejs:master Mar 16, 2017

ge0rg mentioned this pull request Feb 28, 2018

Typing in MUC shows "Typing from another device" #1026

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed typing status for typing from different device #802

Fixed typing status for typing from different device #802

smitbose commented Mar 4, 2017

smitbose commented Mar 4, 2017

jcbrand commented Mar 8, 2017

smitbose commented Mar 8, 2017

smitbose commented Mar 8, 2017

jcbrand commented Mar 9, 2017

smitbose commented Mar 9, 2017 •

edited

jcbrand commented Mar 9, 2017 •

edited

jcbrand Mar 10, 2017

jcbrand Mar 10, 2017

smitbose commented Mar 12, 2017

jcbrand commented Mar 12, 2017

smitbose commented Mar 12, 2017

jcbrand commented Mar 13, 2017

jcbrand commented Mar 13, 2017

smitbose commented Mar 13, 2017

smitbose commented Mar 13, 2017

jcbrand commented Mar 13, 2017

smitbose commented Mar 13, 2017

jcbrand Mar 13, 2017

jcbrand commented Mar 15, 2017

smitbose commented Mar 15, 2017

jcbrand Mar 15, 2017

jcbrand Mar 15, 2017

jcbrand Mar 15, 2017

jcbrand Mar 15, 2017

jcbrand Mar 15, 2017

jcbrand Mar 15, 2017

jcbrand commented Mar 16, 2017

smitbose commented Mar 16, 2017

jcbrand commented Mar 16, 2017

Fixed typing status for typing from different device #802

Fixed typing status for typing from different device #802

Conversation

smitbose commented Mar 4, 2017

smitbose commented Mar 4, 2017

jcbrand commented Mar 8, 2017

smitbose commented Mar 8, 2017

smitbose commented Mar 8, 2017

jcbrand commented Mar 9, 2017

smitbose commented Mar 9, 2017 • edited

jcbrand commented Mar 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitbose commented Mar 12, 2017

jcbrand commented Mar 12, 2017

smitbose commented Mar 12, 2017

jcbrand commented Mar 13, 2017

jcbrand commented Mar 13, 2017

smitbose commented Mar 13, 2017

smitbose commented Mar 13, 2017

jcbrand commented Mar 13, 2017

smitbose commented Mar 13, 2017

Choose a reason for hiding this comment

jcbrand commented Mar 15, 2017

smitbose commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcbrand commented Mar 16, 2017

smitbose commented Mar 16, 2017

jcbrand commented Mar 16, 2017

smitbose commented Mar 9, 2017 •

edited

jcbrand commented Mar 9, 2017 •

edited