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

Fix the known bug where a state notification reopens a chat box #357

Merged
merged 1 commit into from Apr 1, 2015
Merged

Fix the known bug where a state notification reopens a chat box #357

merged 1 commit into from Apr 1, 2015

Conversation

floriancargoet
Copy link
Contributor

When a state notification is received, a chat box is opened. If the notification is the result of the other person closing the chat box, it's annoying. User 1 closes a box, user 2 sees a box auto opening. User 2 closes the new box, user 1 sees a box auto opening. This has no end.

It's a known bug since there's a FIXME comment in the code.

I propose this fix: only open a chat box when the message has a body (i.e. a real message).

@jcbrand
Copy link
Member

jcbrand commented Apr 1, 2015

Thanks a lot for your pull requests.

I like the fact that you've come up with a simple solution to solve this particular issue. However there is a bigger architectural issue that I want to resolve which should then also fix this bug.

At the moment, as soon as a chat box is created, it's also opened and appears to the user. I want to change the code so that creating a chat box doesn't automatically open it as well. This should resolve certain issues with the API as well.

I'll first try that and see how it goes before deciding whether it then still makes sense to merge this particular pull request or not.

@jcbrand
Copy link
Member

jcbrand commented Apr 1, 2015

OK, after doing some refactoring as explained above, I realize that your change is still perfectly valid and compatible.

jcbrand added a commit that referenced this pull request Apr 1, 2015
…eopen-box

Fix the known bug where a state notification reopens a chat box
@jcbrand jcbrand merged commit a902ba2 into conversejs:master Apr 1, 2015
jcbrand added a commit that referenced this pull request Apr 2, 2015
This caused a bug whereby a chat box would open only on chat state
notifications.

Also refactored the chats.open and chats.get methods so that they now reuse the
same map function and so that chats.get can now return any chat box and not
just already open ones.

Updated the tests to properly test this and updated the docs.
@floriancargoet
Copy link
Contributor Author

Thank you for merging these PRs so quickly.

@floriancargoet floriancargoet deleted the fix/state-notifications-reopen-box branch April 2, 2015 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants