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.

Sign up for GitHub

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

Jump to bottom

Add a contact add/remove button in chat. #120

Closed
wants to merge 1 commit into from
Closed

Add a contact add/remove button in chat. #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion static/js/directives/chat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
*/
define(['underscore', 'text!partials/chat.html', 'text!partials/chatroom.html'], function(_, templateChat, templateChatroom) {

return ["$compile", "safeDisplayName", "mediaStream", "safeApply", "desktopNotify", "translation", "playSound", "fileUpload", "randomGen", "buddyData", "appData", "$timeout", "geolocation", function($compile, safeDisplayName, mediaStream, safeApply, desktopNotify, translation, playSound, fileUpload, randomGen, buddyData, appData, $timeout, geolocation) {
return ["$compile", "safeDisplayName", "mediaStream", "safeApply", "desktopNotify", "translation", "playSound", "fileUpload", "randomGen", "buddyData", "appData", "$timeout", "geolocation", "contacts", function($compile, safeDisplayName, mediaStream, safeApply, desktopNotify, translation, playSound, fileUpload, randomGen, buddyData, appData, $timeout, geolocation, contacts) {

var displayName = safeDisplayName;
var group_chat_id = "";
Expand Down Expand Up @@ -191,6 +191,7 @@ define(['underscore', 'text!partials/chat.html', 'text!partials/chatroom.html'],
var options = $.extend({}, opts);
var subscope = controller.rooms[id];
var index = controller.visibleRooms.length;
var buddy = buddyData.lookup(id);
if (!subscope) {
console.log("Create new chatroom", [id]);
controller.visibleRooms.push(id);
Expand All @@ -208,6 +209,17 @@ define(['underscore', 'text!partials/chat.html', 'text!partials/chatroom.html'],
subscope.p2pstate = false;
subscope.active = false;
subscope.pending = 0;
subscope.isuser = buddy && buddy.session && buddy.session.Userid;
subscope.iscontact = (function() {
if (subscope.isgroupchat) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm reading this wrong, checking for group chat status here ought never be required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are correct, removed.

if (buddy && buddy.contact) {
return true;
} else {
return false;
}
})();
if (!subscope.isgroupchat) {
buddyData.push(id);
}
Expand Down Expand Up @@ -322,6 +334,38 @@ define(['underscore', 'text!partials/chat.html', 'text!partials/chatroom.html'],
subscope.doClear = function() {
subscope.$broadcast("clear");
};
subscope.toggleContact = function() {
if (!buddy) {
return;
}
// add
if (!buddy.contact) {
subscope.$emit("requestcontact", subscope.id, {
restore: true
});
// remove
} else {
contacts.remove(buddy.contact.Userid);
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use two separate event handlers, e.g. addContact and removeContact. Functions that both add and remove are almost never a good idea, and it does not simplify the view at all to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to addContact and removeContact.

var updateContactStatus = function(event, data) {
if (!scope.currentRoom || scope.currentRoom.isgroupchat) {
return;
}
var bd = buddyData.get(scope.currentRoom.id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that by using currentRoom here, we're not going to update iscontact correctly for chats which are not in the foreground. All of this should use subscope instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

if (!bd) {
return;
}
if (data.Userid === bd.session.Userid) {
if (event.type === "contactadded") {
subscope.iscontact = true;
} else {
subscope.iscontact = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is highly redundant, just say

subscope.iscontact = (event.type === "contactadded");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

}
};
contacts.e.on("contactadded", updateContactStatus);
contacts.e.on("contactremoved", updateContactStatus);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These handlers need to be removed when the subscope is destroyed in killRoom(). Bad enough that we're adding potentially quite a few event handlers, but far worse to leak them...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now removing handlers at killRoom().

//console.log("Creating new chat room", controller, subscope, index);
subscope.$on("submit", function(event, input) {
subscope.seen();
Expand Down
2 changes: 2 additions & 0 deletions static/partials/chatroom.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
<button ng-if="!isgroupchat" class="btn btn-sm btn-primary" title="{{_('Start video call')}}" ng-click="doCall()"><i class="fa fa-phone fa-fw"></i></button>
<button class="btn btn-sm btn-primary btn-fileupload" title="{{_('Upload files')}}"><i class="fa fa-upload fa-fw"></i></button>
<button class="btn btn-sm btn-primary" title="{{_('Share my location')}}" ng-click="shareGeolocation()"><i class="fa fa-location-arrow fa-fw"></i></button>
<button ng-if="!isgroupchat && isuser && !iscontact" class="btn btn-sm btn-primary" title="{{_('Add to contacts')}}" ng-click="toggleContact()"><i class="fa fa-star-o"></i></button>
<button ng-if="!isgroupchat && isuser && iscontact" class="btn btn-sm btn-primary" title="{{_('Remove from contacts')}}" ng-click="toggleContact()"><i class="fa fa-star"></i></button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !isgroupchat && isuser condition should be combined into a single variable, e.g. canAddContact

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added canAddContact flag. This is much easier to read.

</div>
<div class="btn-group pull-right">
<button class="btn btn-sm btn-default" title="{{_('Clear chat')}}" ng-click="doClear()"><i class="fa fa-eraser fa-fw"></i></button>
Expand Down