From 899dd21328cb790d4c150278e1aadd711474985d Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Tue, 3 Jan 2017 15:43:30 -0800 Subject: [PATCH] Lots of CR feedback --- assignment-client/src/avatars/AvatarMixer.cpp | 4 + .../resources/qml/controls-uit/CheckBox.qml | 4 +- interface/resources/qml/hifi/Pal.qml | 125 +++++++++++------- libraries/networking/src/Node.cpp | 4 +- libraries/networking/src/NodeList.cpp | 22 ++- 5 files changed, 96 insertions(+), 63 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixer.cpp b/assignment-client/src/avatars/AvatarMixer.cpp index 30ce210eb6..3cacfdedf1 100644 --- a/assignment-client/src/avatars/AvatarMixer.cpp +++ b/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 diff --git a/interface/resources/qml/controls-uit/CheckBox.qml b/interface/resources/qml/controls-uit/CheckBox.qml index 503e6b8739..9db4f621f9 100644 --- a/interface/resources/qml/controls-uit/CheckBox.qml +++ b/interface/resources/qml/controls-uit/CheckBox.qml @@ -27,9 +27,7 @@ Original.CheckBox { readonly property int checkRadius: 2 style: CheckBoxStyle { - indicator: - - Rectangle { + indicator: Rectangle { id: box width: boxSize height: boxSize diff --git a/interface/resources/qml/hifi/Pal.qml b/interface/resources/qml/hifi/Pal.qml index 2196692e1c..3c3cde7a67 100644 --- a/interface/resources/qml/hifi/Pal.qml +++ b/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) - // Just for now, while we cannot undo things: - userModel.remove(model.userIndex) - sortModel() + console.log("User clicked on an unknown checkbox."); } // 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 - for (var i = 0; i < userModel.count; i++) { - if (userModel.get(i).sessionId === sessionId) { + function findSessionIndex(sessionId, optionalData) { // no findIndex in .qml + var data = optionalData || userModelData, length = data.length; + 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; - for (var i = 0; i < data.length; i++) { - if (data[i].sessionId === "") { - myIndex = i; - break; - } - } - if (myIndex !== -1) { + var index = -1; + index = findSessionIndex('', data); + if (index !== -1) { iAmAdmin = Users.canKick; - myData = data[myIndex]; - data.splice(myIndex, 1); + myData = data[index]; + 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(); - var userIndex = 0; - data.forEach(function (datum) { - function init(property) { - if (datum[property] === undefined) { - datum[property] = false; - } + userModelData = data; + for (var ignoredID in ignored) { + index = findSessionIndex(ignoredID); + if (index === -1) { // Add back any missing ignored to the PAL, because they sometimes take a moment to show up. + userModelData.push(ignored[ignoredID]); + } 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 - var userIndex = findSessionIndexInUserModel(userId); + // Get the index in userModel and userModelData associated with the passed UUID + 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++) { - function init(prop) { - if (sortedList[i][prop] === undefined) { - sortedList[i][prop] = false; + + userModel.clear(); + var userIndex = 0; + userModelData.forEach(function (datum) { + function init(property) { + if (datum[property] === undefined) { + datum[property] = false; } } - sortedList[i].userIndex = currentUserIndex++; ['personalMute', 'ignore', 'mute', 'kick'].forEach(init); - userModel.append(sortedList[i]); - } - userModel.remove(0, sortedList.length); + datum.userIndex = userIndex++; + userModel.append(datum); + 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}); } diff --git a/libraries/networking/src/Node.cpp b/libraries/networking/src/Node.cpp index 484ea882b5..8063075e22 100644 --- a/libraries/networking/src/Node.cpp +++ b/libraries/networking/src/Node.cpp @@ -82,11 +82,11 @@ void Node::updateClockSkewUsec(qint64 clockSkewSample) { } void Node::parseIgnoreRequestMessage(QSharedPointer 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); diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 8b4cb41cc9..64f0479a51 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/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 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; }