Chat State Notifications (XEP-0085) #431

sualko · 2017-01-05T14:09:33Z

Improve #424

@mmoqui Do you want to test this?

TODO

Add general option
Add user option

mmoqui · 2017-01-05T15:10:49Z

Fine work. Much more better than my own work ;-)
Ok for testing it. Nevertheless you've forgotten to include the chatstate Stophe plugin.

sualko · 2017-01-05T15:13:41Z

Ok for testing it

Great. Can you report your results? Because if you find no issues, I will merge this.

you've forgotten to include the chatstate Stophe plugin

I don't think so, because it also revert the revert of your pr. On the command line I see your commit and there should be also the plugin entry. We will see how this will work 😉

sualko · 2017-01-05T15:15:36Z

We will see how this will work

It will not work 😆. I manually have to add your changes.

mmoqui · 2017-01-05T16:11:52Z

src/jsxc.lib.xmpp.chatState.js

+   if (usersComposing.length === 0) {
+      var duration = parseFloat(el.css('transition-duration'));
+
+      if (el.css('transition-duration').match(/s$/)) {


el.css('transition-duration') returns sometimes undefined in the browser (both Chrome and Firefox) which throws an error when calling the match method on it. (This error then broke the composition notification.)
It is possible this error occurs when the .jsxc_composing div isn't more rendering (after a pausing and a timeout triggering the div removing)

Just for information, I detected also a bug with the translations: the $t.() function doesn't take into account the variables and then we can see the name of the variable (_name_ for example) when a translated text is displayed (like in the desktop notifications)

I will have a look into the duration issue. Could you open another issue for the translation problem?

done. I just submitted an issue about the translation problem

ChristianTacke · 2017-01-07T11:11:39Z

Gruntfile.js

@@ -26,7 +26,7 @@ module.exports = function(grunt) {
         main: {
            files: [{
               expand: true,
-               src: ['lib/i18next/i18next.min.js', 'lib/jquery-i18next/jquery-i18next.min.js', 'lib/magnific-popup/dist/*.js', 'lib/favico.js/favico.js', 'lib/emojione/lib/js/*.js', 'lib/emojione/assets/svg/*.svg', 'lib/strophe.js/strophe.js', 'lib/strophe.x/*.js', 'lib/strophe.bookmarks/*.js', 'lib/strophe.vcard/*.js', 'lib/strophe.jinglejs/*-bundle.js', 'lib/otr/build/**', 'lib/otr/lib/dsa-webworker.js', 'lib/otr/lib/sm-webworker.js', 'lib/otr/lib/const.js', 'lib/otr/lib/helpers.js', 'lib/otr/lib/dsa.js', 'lib/otr/vendor/*.js', 'lib/*.js', 'LICENSE', 'img/**', 'sound/**'],
+               src: ['lib/i18next/i18next.min.js', 'lib/jquery-i18next/jquery-i18next.min.js', 'lib/magnific-popup/dist/*.js', 'lib/favico.js/favico.js', 'lib/emojione/lib/js/*.js', 'lib/emojione/assets/svg/*.svg', 'lib/strophe.js/strophe.js', 'lib/strophe.x/*.js', 'lib/strophe.bookmarks/*.js', 'lib/strophe.chatstates/*.js', 'lib/strophe.vcard/*.js', 'lib/strophe.jinglejs/*-bundle.js', 'lib/otr/build/**', 'lib/otr/lib/dsa-webworker.js', 'lib/otr/lib/sm-webworker.js', 'lib/otr/lib/const.js', 'lib/otr/lib/helpers.js', 'lib/otr/lib/dsa.js', 'lib/otr/vendor/*.js', 'lib/*.js', 'LICENSE', 'img/**', 'sound/**'],


Hi,
could you consider wrapping this over multiple lines? This would improve readability of the diff/patch in the future.

Good point. I will try if this is possible with grunt-jsbeautifier.

That looks already a lot better.

ChristianTacke · 2017-01-07T11:15:30Z

template/settings.html

+         </div>
+      </div>
+   </fieldset>
+</form>


I haven't yet seen, how this will look like, but the current (old) settings window is already taking the whole screen on a 1024x1024 browser window. Are jquery ui tabs an option here?

We definitely have to improve the settings dialog, but I think this is worthy a second pr 😉

Should I file an issue requesting that?

Yes, that would be nice.

ChristianTacke · 2017-01-08T12:20:29Z

src/jsxc.lib.xmpp.chatState.js

@@ -130,9 +130,10 @@ jsxc.xmpp.chatState.onPaused = function(ev, jid) {
   }

   if (usersComposing.length === 0) {
-      var duration = parseFloat(el.css('transition-duration'));
+      var durationValue = el.css('transition-duration') || '0';


Why not || '0s'? I think that's the more or less offiecial default, according to https://developer.mozilla.org/en-US/docs/Web/CSS/transition-duration#Summary

I think it doesn't matter. 0s would be maybe cleaner, but the result is the same 😄

ChristianTacke · 2017-01-08T12:22:39Z

src/jsxc.lib.xmpp.chatState.js


-      if (el.css('transition-duration').match(/s$/)) {
+      if (durationValue.match(/s$/)) {


I don't know, what values you expect anyway, but as I understand the above mentioned spec, there's also the option of "ms". So maybe: .match(/[^m]s$/)?

Great catch. Will fix it asap.

sualko · 2017-01-09T09:11:49Z

Thanks to everyone you reviewed or contributed to this pr. It is really fun to work with you guys.

ChristianTacke · 2017-01-09T15:51:10Z

Thanks! Yes it's cool to work with all of you!

sualko added 2 commits January 4, 2017 16:10

beautify pr

bbe73ee

Fix chat state notifications

6de578a

sualko added enhancement Testing WIP and removed Testing labels Jan 5, 2017

sualko added 2 commits January 5, 2017 15:39

add option to enable/disable chat state notifications

28c3898

Merge branch 'master' into feature-chatstate

805e325

sualko added Testing and removed WIP labels Jan 5, 2017

add strophe.chatstates as dependency

7e2a00a

sualko modified the milestone: 3.1 Jan 5, 2017

mmoqui reviewed Jan 5, 2017

View reviewed changes

ChristianTacke reviewed Jan 7, 2017

View reviewed changes

fix undefined transition duration

d236ac0

ChristianTacke reviewed Jan 8, 2017

View reviewed changes

fix transition duration regex

5b58045

sualko merged commit 8fc5f04 into master Jan 9, 2017

sualko deleted the feature-chatstate branch January 9, 2017 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chat State Notifications (XEP-0085) #431

Chat State Notifications (XEP-0085) #431

sualko commented Jan 5, 2017 •

edited

Loading

mmoqui commented Jan 5, 2017

sualko commented Jan 5, 2017

sualko commented Jan 5, 2017

mmoqui Jan 5, 2017 •

edited

Loading

mmoqui Jan 5, 2017 •

edited

Loading

sualko Jan 8, 2017

mmoqui Jan 9, 2017

ChristianTacke Jan 7, 2017

sualko Jan 8, 2017

sualko Jan 9, 2017

ChristianTacke Jan 9, 2017

ChristianTacke Jan 7, 2017

sualko Jan 8, 2017

ChristianTacke Jan 8, 2017

sualko Jan 9, 2017

ChristianTacke Jan 9, 2017

ChristianTacke Jan 8, 2017

sualko Jan 9, 2017

ChristianTacke Jan 8, 2017

sualko Jan 9, 2017

sualko commented Jan 9, 2017

ChristianTacke commented Jan 9, 2017


		if (el.css('transition-duration').match(/s$/)) {
		if (durationValue.match(/s$/)) {

Chat State Notifications (XEP-0085) #431

Chat State Notifications (XEP-0085) #431

Conversation

sualko commented Jan 5, 2017 • edited Loading

mmoqui commented Jan 5, 2017

sualko commented Jan 5, 2017

sualko commented Jan 5, 2017

mmoqui Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

mmoqui Jan 5, 2017 • edited Loading

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

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

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

sualko commented Jan 9, 2017

ChristianTacke commented Jan 9, 2017

sualko commented Jan 5, 2017 •

edited

Loading

mmoqui Jan 5, 2017 •

edited

Loading

mmoqui Jan 5, 2017 •

edited

Loading