Lots of CR feedback

assignment-client/src/avatars/AvatarMixer.cpp

@@ -110,6 +110,10 @@ void AvatarMixer::broadcastAvatarData() {
     const float PREVIOUS_FRAMES_RATIO = 1.0f - CURRENT_FRAME_RATIO;
 
     // only send extra avatar data (avatars out of view, ignored) every Nth AvatarData frame
+    // Extra avatar data will be sent (AVATAR_MIXER_BROADCAST_FRAMES_PER_SECOND/EXTRA_AVATAR_DATA_FRAME_RATIO) times
+    // per second.
+    // This value should be a power of two for performance purposes, as the mixer performs a modulo operation every frame
+    // to determine whether the extra data should be sent.
     const int EXTRA_AVATAR_DATA_FRAME_RATIO = 16; 
     
     // NOTE: The following code calculates the _performanceThrottlingRatio based on how much the avatar-mixer was

interface/resources/qml/controls-uit/CheckBox.qml

@@ -27,9 +27,7 @@ Original.CheckBox {
     readonly property int checkRadius: 2
 
     style: CheckBoxStyle {
-        indicator:
+        indicator: Rectangle {
-
-        Rectangle {
             id: box
             width: boxSize
             height: boxSize

interface/resources/qml/hifi/Pal.qml

@@ -42,6 +42,9 @@ Item {
     property int actionButtonWidth: 75
     property int nameCardWidth: width - actionButtonWidth*(iAmAdmin ? 4 : 2) - 4
     property var myData: ({displayName: "", userName: "", audioLevel: 0.0}) // valid dummy until set
+    // FIXME: Need to determine & handle when this list gets reset.
+    property var ignored: ({}); // Keep a local list of ignored avatars & their data. Necessary because HashMap is slow to respond after ignoring.
+    property var userModelData: [] // This simple list is essentially a mirror of the userModel listModel without all the extra complexities.
     property bool iAmAdmin: false
 
     // This contains the current user's NameCard and will contain other information in the future
@@ -187,7 +190,8 @@ Item {
         // This Item refers to the contents of each Cell
         itemDelegate: Item {
             id: itemCell
-            property bool isCheckBox: typeof(styleData.value) === 'boolean'
+            property bool isCheckBox: styleData.role === "personalMute" || styleData.role === "ignore"
+            property bool isButton: styleData.role === "mute" || styleData.role === "kick"
             // This NameCard refers to the cell that contains an avatar's
             // DisplayName and UserName
             NameCard {
@@ -196,7 +200,7 @@ Item {
                 displayName: styleData.value
                 userName: model ? model.userName : ""
                 audioLevel: model ? model.audioLevel : 0.0
-                visible: !isCheckBox
+                visible: !isCheckBox && !isButton
                 // Size
                 width: nameCardWidth
                 height: parent.height
@@ -204,7 +208,7 @@ Item {
                 anchors.left: parent.left
             }
             
-            // This CheckBox belongs in the columns that contain the action buttons ("Mute", "Ban", etc)
+            // This CheckBox belongs in the columns that contain the stateful action buttons ("Mute" & "Ignore" for now)
             // KNOWN BUG with the Checkboxes: When clicking in the center of the sorting header, the checkbox
             // will appear in the "hovered" state. Hovering over the checkbox will fix it.
             // Clicking on the sides of the sorting header doesn't cause this problem.
@@ -225,16 +229,20 @@ Item {
                         newValue = false
                     }
                     userModel.setProperty(model.userIndex, styleData.role, newValue)
+                    userModelData[model.userIndex][styleData.role] = newValue // Defensive programming
                     if (styleData.role === "personalMute" || styleData.role === "ignore") {
                         Users[styleData.role](model.sessionId, newValue)
                         if (styleData.role === "ignore") {
                             userModel.setProperty(model.userIndex, "personalMute", newValue)
+                            userModelData[model.userIndex]["personalMute"] = newValue // Defensive programming
+                            if (newValue) {
+                                ignored[model.sessionId] = userModelData[model.userIndex]
+                            } else {
+                                delete ignored[model.sessionId]
+                            }
                         }
                     } else {
-                        Users[styleData.role](model.sessionId)
+                        console.log("User clicked on an unknown checkbox.");
-                        // Just for now, while we cannot undo things:
-                        userModel.remove(model.userIndex)
-                        sortModel()
                     }
                     // http://doc.qt.io/qt-5/qtqml-syntax-propertybinding.html#creating-property-bindings-from-javascript
                     // I'm using an explicit binding here because clicking a checkbox breaks the implicit binding as set by
@@ -242,6 +250,29 @@ Item {
                     checked = Qt.binding(function() { return (model && model[styleData.role]) ? model[styleData.role] : false})
                 }
             }
+            
+            // This Button belongs in the columns that contain the stateless action buttons ("Silence" & "Ban" for now)
+            HifiControls.Button {
+                id: actionButton
+                color: 2 // Red
+                visible: isButton
+                anchors.centerIn: parent
+                width: 24
+                height: 24
+                onClicked: {
+                    if (styleData.role === "mute" || styleData.role === "kick") {
+                        Users[styleData.role](model.sessionId)
+                        if (styleData.role === "kick") {
+                            // Just for now, while we cannot undo "Ban":
+                            userModel.remove(model.userIndex)
+                            delete userModelData[model.userIndex] // Defensive programming
+                            sortModel()
+                        }
+                    } else {
+                        console.log("User clicked on an unknown checkbox.");
+                    }
+                }
+            }
         }
     }
     // Refresh button
@@ -363,9 +394,10 @@ Item {
         }
     }
 
-    function findSessionIndexInUserModel(sessionId) { // no findIndex in .qml
+    function findSessionIndex(sessionId, optionalData) { // no findIndex in .qml
-        for (var i = 0; i < userModel.count; i++) {
+        var data = optionalData || userModelData, length = data.length;
-            if (userModel.get(i).sessionId === sessionId) {
+        for (var i = 0; i < length; i++) {
+            if (data[i].sessionId === sessionId) {
                 return i;
             }
         }
@@ -375,38 +407,30 @@ Item {
         switch (message.method) {
         case 'users':
             var data = message.params;
-            var myIndex = -1;
+            var index = -1;
-            for (var i = 0; i < data.length; i++) {
+            index = findSessionIndex('', data);
-                if (data[i].sessionId === "") {
+            if (index !== -1) {
-                    myIndex = i;
-                    break;
-                }
-            }
-            if (myIndex !== -1) {
                 iAmAdmin = Users.canKick;
-                myData = data[myIndex];
+                myData = data[index];
-                data.splice(myIndex, 1);
+                data.splice(index, 1);
             } else {
                 console.log("This user's data was not found in the user list. PAL will not function properly.");
             }
-            userModel.clear();
+            userModelData = data;
-            var userIndex = 0;
+            for (var ignoredID in ignored) {
-            data.forEach(function (datum) {
+                index = findSessionIndex(ignoredID);
-                function init(property) {
+                if (index === -1) { // Add back any missing ignored to the PAL, because they sometimes take a moment to show up.
-                    if (datum[property] === undefined) {
+                    userModelData.push(ignored[ignoredID]);
-                        datum[property] = false;
+                } else { // Already appears in PAL; update properties of existing element in model data
-                    }
+                    userModelData[index] = ignored[ignoredID];
                 }
-                ['personalMute', 'ignore', 'mute', 'kick'].forEach(init);
+            }
-                datum.userIndex = userIndex++;
-                userModel.append(datum);
-            });
             sortModel();
             break;
         case 'select':
             var sessionId = message.params[0];
             var selected = message.params[1];
-            var userIndex = findSessionIndexInUserModel(sessionId);
+            var userIndex = findSessionIndex(sessionId);
             if (userIndex != -1) {
                 if (selected) {
                     table.selection.clear(); // for now, no multi-select
@@ -427,11 +451,12 @@ Item {
                 myData.userName = userName;
                 myCard.userName = userName; // Defensive programming
             } else {
-                // Get the index in userModel associated with the passed UUID
+                // Get the index in userModel and userModelData associated with the passed UUID
-                var userIndex = findSessionIndexInUserModel(userId);
+                var userIndex = findSessionIndex(userId);
                 if (userIndex != -1) {
                     // Set the userName appropriately
                     userModel.setProperty(userIndex, "userName", userName);
+                    userModelData[userIndex].userName = userName; // Defensive programming
                 }
             }
             break;
@@ -443,9 +468,10 @@ Item {
                     myData.audioLevel = audioLevel;
                     myCard.audioLevel = audioLevel; // Defensive programming
                 } else {
-                    var userIndex = findSessionIndexInUserModel(userId);
+                    var userIndex = findSessionIndex(userId);
                     if (userIndex != -1) {
                         userModel.setProperty(userIndex, "audioLevel", audioLevel);
+                        userModelData[userIndex].audioLevel = audioLevel; // Defensive programming
                     }
                 }
             }
@@ -455,15 +481,10 @@ Item {
         }
     }
     function sortModel() {
-        var sortedList = [];
-        for (var i = 0; i < userModel.count; i++) {
-            sortedList.push(userModel.get(i));
-        }
-
         var sortProperty = table.getColumn(table.sortIndicatorColumn).role;
         var before = (table.sortIndicatorOrder === Qt.AscendingOrder) ? -1 : 1;
         var after = -1 * before;
-        sortedList.sort(function (a, b) {
+        userModelData.sort(function (a, b) {
             var aValue = a[sortProperty].toString().toLowerCase(), bValue = b[sortProperty].toString().toLowerCase();
             switch (true) {
             case (aValue < bValue): return before;
@@ -472,24 +493,26 @@ Item {
             }
         });
         table.selection.clear();
-        var currentUserIndex = 0;
+
-        for (var i = 0; i < sortedList.length; i++) {
+        userModel.clear();
-            function init(prop) {
+        var userIndex = 0;
-                if (sortedList[i][prop] === undefined) {
+        userModelData.forEach(function (datum) {
-                    sortedList[i][prop] = false;
+            function init(property) {
+                if (datum[property] === undefined) {
+                    datum[property] = false;
                 }
             }
-            sortedList[i].userIndex = currentUserIndex++;
             ['personalMute', 'ignore', 'mute', 'kick'].forEach(init);
-            userModel.append(sortedList[i]);
+            datum.userIndex = userIndex++;
-        }
+            userModel.append(datum);
-        userModel.remove(0, sortedList.length);
+            console.log('appending to userModel:', JSON.stringify(datum));
+        });
     }
     signal sendToScript(var message);
     function noticeSelection() {
         var userIds = [];
         table.selection.forEach(function (userIndex) {
-            userIds.push(userModel.get(userIndex).sessionId);
+            userIds.push(userModelData[userIndex].sessionId);
         });
         pal.sendToScript({method: 'selected', params: userIds});
     }

libraries/networking/src/Node.cpp

@@ -82,11 +82,11 @@ void Node::updateClockSkewUsec(qint64 clockSkewSample) {
 }
 
 void Node::parseIgnoreRequestMessage(QSharedPointer<ReceivedMessage> message) {
+    bool addToIgnore;
+    message->readPrimitive(&addToIgnore);
     while (message->getBytesLeftToRead()) {
         // parse out the UUID being ignored from the packet
         QUuid ignoredUUID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID));
-        bool addToIgnore;
-        message->readPrimitive(&addToIgnore);
 
         if (addToIgnore) {
             addIgnoredNode(ignoredUUID);

libraries/networking/src/NodeList.cpp

@@ -790,9 +790,9 @@ void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) {
             // create a reliable NLPacket with space for the ignore UUID
             auto ignorePacket = NLPacket::create(PacketType::NodeIgnoreRequest, NUM_BYTES_RFC4122_UUID + sizeof(bool), true);
 
+            ignorePacket->writePrimitive(ignoreEnabled);
             // write the node ID to the packet
             ignorePacket->write(nodeID.toRfc4122());
-            ignorePacket->writePrimitive(ignoreEnabled);
 
             qCDebug(networking) << "Sending packet to" << (destinationNode->getType() == NodeType::AudioMixer ? "AudioMixer" : "AvatarMixer") << "to"
                 << (ignoreEnabled ? "ignore" : "unignore") << "node" << uuidStringWithoutCurlyBraces(nodeID);
@@ -801,16 +801,17 @@ void NodeList::ignoreNodeBySessionID(const QUuid& nodeID, bool ignoreEnabled) {
             sendPacket(std::move(ignorePacket), *destinationNode);
         });
 
-        QReadLocker ignoredSetLocker { &_ignoredSetLock }; // write lock for insert and unsafe_erase
-        QReadLocker personalMutedSetLocker{ &_personalMutedSetLock }; // write lock for insert and unsafe_erase
-
         if (ignoreEnabled) {
+            QReadLocker ignoredSetLocker{ &_ignoredSetLock }; // read lock for insert
+            QReadLocker personalMutedSetLocker{ &_personalMutedSetLock }; // read lock for insert 
             // add this nodeID to our set of ignored IDs
             _ignoredNodeIDs.insert(nodeID);
             // add this nodeID to our set of personal muted IDs
             _personalMutedNodeIDs.insert(nodeID);
             emit ignoredNode(nodeID);
         } else {
+            QWriteLocker ignoredSetLocker{ &_ignoredSetLock }; // write lock for unsafe_erase
+            QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; // write lock for unsafe_erase
             _ignoredNodeIDs.unsafe_erase(nodeID);
             _personalMutedNodeIDs.unsafe_erase(nodeID);
             emit unignoredNode(nodeID);
@@ -838,20 +839,21 @@ void NodeList::personalMuteNodeBySessionID(const QUuid& nodeID, bool muteEnabled
                 // setup the packet
                 auto personalMutePacket = NLPacket::create(PacketType::NodeIgnoreRequest, NUM_BYTES_RFC4122_UUID + sizeof(bool), true);
 
+                personalMutePacket->writePrimitive(muteEnabled);
                 // write the node ID to the packet
                 personalMutePacket->write(nodeID.toRfc4122());
-                personalMutePacket->writePrimitive(muteEnabled);
 
                 qCDebug(networking) << "Sending Personal Mute Packet to" << (muteEnabled ? "mute" : "unmute") << "node" << uuidStringWithoutCurlyBraces(nodeID);
 
                 sendPacket(std::move(personalMutePacket), *audioMixer);
 
-                QReadLocker personalMutedSetLocker{ &_personalMutedSetLock }; // write lock for insert and unsafe_erase
 
                 if (muteEnabled) {
+                    QReadLocker personalMutedSetLocker{ &_personalMutedSetLock }; // read lock for insert 
                     // add this nodeID to our set of personal muted IDs
                     _personalMutedNodeIDs.insert(nodeID);
                 } else {
+                    QWriteLocker personalMutedSetLocker{ &_personalMutedSetLock }; // write lock for unsafe_erase
                     _personalMutedNodeIDs.unsafe_erase(nodeID);
                 }
             }
@@ -879,6 +881,9 @@ void NodeList::maybeSendIgnoreSetToNode(SharedNodePointer newNode) {
             // setup a packet list so we can send the stream of ignore IDs
             auto personalMutePacketList = NLPacketList::create(PacketType::NodeIgnoreRequest, QByteArray(), true);
 
+            // Force the "enabled" flag in this packet to true
+            personalMutePacketList->writePrimitive(true);
+
             // enumerate the ignored IDs and write them to the packet list
             auto it = _personalMutedNodeIDs.cbegin();
             while (it != _personalMutedNodeIDs.end()) {
@@ -903,6 +908,9 @@ void NodeList::maybeSendIgnoreSetToNode(SharedNodePointer newNode) {
             // setup a packet list so we can send the stream of ignore IDs
             auto ignorePacketList = NLPacketList::create(PacketType::NodeIgnoreRequest, QByteArray(), true);
 
+            // Force the "enabled" flag in this packet to true
+            ignorePacketList->writePrimitive(true);
+
             // enumerate the ignored IDs and write them to the packet list
             auto it = _ignoredNodeIDs.cbegin();
             while (it != _ignoredNodeIDs.end()) {
@@ -1008,7 +1016,7 @@ void NodeList::processUsernameFromIDReply(QSharedPointer<ReceivedMessage> messag
 }
 
 void NodeList::setRequestsDomainListData(bool isRequesting) {
-    // Tell the avatar mixer whether I want to receive any additional data to which I might be entitiled .
+    // Tell the avatar mixer whether I want to receive any additional data to which I might be entitled
     if (_requestsDomainListData == isRequesting) {
         return;
     }