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

Issue #2: wrong naming #5

Closed
wants to merge 2 commits into from
Closed

Issue #2: wrong naming #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e71c7bf
Created new NPM package `peerinstruction`
pbanaszkiewicz Jun 8, 2014
9023312
Removed NPM package, admin~>instructor, some UI tweaks
pbanaszkiewicz Jun 11, 2014
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
27 changes: 0 additions & 27 deletions client/html/admin.html
View file
Open in desktop

This file was deleted.

36 changes: 36 additions & 0 deletions client/html/instructor.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<!DOCTYPE HTML>
<html lang="en">
<head>
<title>Instructor's dashboard</title>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta http-equiv="Content-Language" content="en-us">

<script type="text/javascript" src="/third_party/jquery-2.1.0.min.js"></script>
<script type="text/javascript" src="/third_party/peer.min.js"></script>
<script type="text/javascript" src="/socket.io/socket.io.js"></script>
<script type="text/javascript" src="../app.js"></script>

<link rel="stylesheet" type="text/css" media="screen, print, handheld" href="/styles/styles.css" />

<script>
$(function() {
var is_admin = true,
pittPeer = APP.newPP(is_admin);
});
</script>
</head>

<body>
<h1>Instructor's view</h1>
<h3>Current mode: <span id="current_mode"></span></h3>
<input type="button" id="broadcast" value="Start broadcasting"><br>
or change mode to:
<input type="button" id="bMode" value="Few" />

<div id="main">
You are: <span id="you_are"> </span>
<div id="list_users"></div>
</div>
</body>

</html>
56 changes: 29 additions & 27 deletions client/html/user.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,36 @@
<!DOCTYPE HTML>
<!-- vim: set ts=2 et:-->
<html lang="en">
<head>
<title>peerjs playground</title>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta http-equiv="Content-Language" content="en-us">
<title>Student's dashboard</title>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta http-equiv="Content-Language" content="en-us">

<script type="text/javascript" src="/third_party/jquery-2.1.0.min.js"></script>
<script type="text/javascript" src="/third_party/peer.min.js"></script>
<script type="text/javascript" src="/socket.io/socket.io.js"></script>
<script type="text/javascript" src="/app.js"></script>
<script type="text/javascript" src="/third_party/jquery-2.1.0.min.js"></script>
<script type="text/javascript" src="/third_party/peer.min.js"></script>
<script type="text/javascript" src="/socket.io/socket.io.js"></script>
<script type="text/javascript" src="../app.js"></script>
Copy link
Owner

Choose a reason for hiding this comment

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

Why this (../app.js)?

Copy link
Author

Choose a reason for hiding this comment

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

I honestly don't remember why I had changed that! Reverted.


<link rel="stylesheet" type="text/css" media="screen, print, handheld" href="/styles/styles.css" />
<link rel="stylesheet" type="text/css" media="screen, print, handheld" href="/styles/styles.css" />

<script>
$(function() {
var is_admin = false,
el_my_video = $('#my-video'),
el_their_video = $('#container-their-video'),
pittPeer = APP.newPP(is_admin, el_my_video, el_their_video);
});
</script>

<div id="main">
You are: <span id="you_are"> </span>
<div id="list_users"> </div>
<div id="video_chat_area">
<div id="local_video"> <video id="my-video" muted="true" autoplay></video> </div>
<div id="container-their-video" class=""> </div>
</div>
</div>
<script>
$(function() {
var is_admin = false,
el_my_video = $('#my-video'),
el_their_video = $('#container-their-video'),
pittPeer = APP.newPP(is_admin, el_my_video, el_their_video);
});
</script>
</head>

<body>
<h1>Students's view</h1>
<div id="main">
<p>You are: <span id="you_are"> </span></p>
<div id="list_users"> </div>
<div id="video_chat_area">
<div id="local_video"> <video id="my-video" muted="true" autoplay></video> </div>
<div id="container-their-video" class=""> </div>
</div>
</div>
</body>

</html>
34 changes: 29 additions & 5 deletions client/js/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
// vim: set ts=2 et:

var NOT_WORKING = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

We don't want to pollute the global name space. Notice how I try to put all related logic in its own
name space.

I'd add those in the variable definition within the newPP function in js/events.js

var BROADCAST_MODE = 2;
var GROUP_MODE = 3;

APP.newPP = function(isAdmin, el_my_video, el_their_video) {
var my_id, iface = {},
socket = io.connect('http://localhost:8111'),
mode = NOT_WORKING,
v_chat; // All the video chat logic

function set_click_mode_change() {
Expand All @@ -11,27 +16,45 @@ APP.newPP = function(isAdmin, el_my_video, el_their_video) {
currVal = b.val(),
newVal = (currVal === "All") ? "Few" : "All";

b.attr('value', newVal);
socket.emit('mode_change', currVal);
b.attr('value', newVal);
socket.emit('mode_change', currVal);
mode = (currVal === "All") ? BROADCAST_MODE : GROUP_MODE;
show_mode("#current_mode");
Copy link
Owner

Choose a reason for hiding this comment

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

Why not just adding this line in this function: show_mode(currVal) and let show_mode
do all the heavy lifting? After all, that's what that function is all about, displaying the mode.

Also, set a variable within the module to point to the id of the element where we will drop
the mode status.

});
}

function show_mode(element) {
console.log("changing mode!")
$(element).text(
(mode === BROADCAST_MODE) ? "Broadcasting" : "Working in groups"
)
}

function set_click_start_broadcasting() {
Copy link
Owner

Choose a reason for hiding this comment

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

In general, do not send dead code in a PR.
Only send code that implements / does something.

$("#broadcast").click(function() {

})
}

function for_admin() {
console.log("Admin mode.");
console.log("Admin mode.");
socket.on('list_rooms', function(listRooms) {
console.log(listRooms);
// console.log(listRooms);
var r = $('#list_users');
r.empty();
Object.keys(listRooms).forEach(function(name, i, a) {
r.append("<ul>" + name);
listRooms[name].forEach(function(user, _i, _a) {
r.append("<li>" + user);
r.append("<li>" + user + "</li>");
Copy link
Owner

Choose a reason for hiding this comment

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

In html5, tag closing for some tags is optional.

Copy link
Author

Choose a reason for hiding this comment

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

Wow, I had no idea!
I think for that kind of templating I'll use something else in the future. One idea is to leverage jQuery or DOM manipulation.

});
r.append("</ul>");
});
});

set_click_mode_change();
socket.emit('mode_change', "All");
Copy link
Owner

Choose a reason for hiding this comment

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

Why are we doing a socket.emit here ? set_click_mode_change will do that already.

Copy link
Author

Choose a reason for hiding this comment

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

Student's code prepares their browsers for streaming when they receive mode_change event from the server.

So basically all I want to do is to set to single room mode as soon as teacher appears online.

mode = BROADCAST_MODE;
show_mode("#current_mode");
Copy link
Owner

Choose a reason for hiding this comment

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

Same thing as before, it is more decoupled if we do: show_mode(BROADCAST_MODE)

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. Except that I think having current mode set in a variable in a numeric format is a good idea.

}

function for_peer() {
Expand Down Expand Up @@ -69,6 +92,7 @@ APP.newPP = function(isAdmin, el_my_video, el_their_video) {
});
});

// XXX: this never happens?
socket.on('mode_change', function(data) {
console.log('mode_change: ' + data);
});
Expand Down
3 changes: 2 additions & 1 deletion client/styles/styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
}

#you_are {
padding: 10px;
padding: 5px;
background-color: #EEE;
}

#list_users {
Expand Down
2 changes: 2 additions & 0 deletions server/load.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,5 @@ l.setSocketEvents(io);

app.use('/', express.static(__dirname + '/../client'));
http.listen(8111);

console.log("Server listening on http://localhost:8111/")
22 changes: 18 additions & 4 deletions server/logic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ module.exports = function() {
var iface = {}, // intarface too all this logic
def_room = "room1",
admins = {},
peers = {}, rooms = {}, ppr = 2; // # People per room
peers = {},
rooms = {},
ppr = 2; // # People per room

// bunch of getters and setters for .peers, .rooms and .ppr
iface.peers = function(_) {
if (!arguments.length) return peers;
peers = _;
Expand Down Expand Up @@ -65,20 +68,27 @@ module.exports = function() {
io.sockets.on('connection', function (socket) {

socket.on('newadmin', function (id) {
console.log("newadmin1", rooms)
socket.peer_id = id;
socket.is_admin = true;
admins[id] = socket;
//io.sockets.emit('list_rooms', rooms);
io.sockets.emit('list_rooms', rooms);
console.log("newadmin2", rooms)
});

socket.on('newpeer', function (id) {
console.log("newpeer1", rooms)
socket.peer_id = id;
socket.is_admin = false;
peers[id] = socket;
rooms[def_room].push(id);
io.sockets.emit('update_list', Object.keys(peers), 'new');
io.sockets.emit('list_rooms', rooms);
console.log("newpeer2", rooms)
});

socket.on('mode_change', function(newMode) {
console.log("New mode:", newMode)
if (newMode === 'Few') {
multiRooms();
updateClientsLists('few');
Expand All @@ -91,14 +101,18 @@ module.exports = function() {
});

socket.on('disconnect', function() {
console.log("disconnect1", rooms)
delete peers[socket.peer_id];
delete admins[socket.peer_id];
update_rooms(socket.peer_id);
if (!socket.is_admin) {
Copy link
Author

Choose a reason for hiding this comment

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

Hopefully introduction of a new attribute is an acceptable fix for the bug.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeap, good catch!

update_rooms(socket.peer_id);
}

io.sockets.emit('update_list', Object.keys(peers), 'gone');
io.sockets.emit('list_rooms', rooms);
console.log("disconnect2", rooms)
console.log("Peer gone: " + socket.peer_id);
console.log(peers);
// console.log(peers);
});
});
}
Copy link
Owner

Choose a reason for hiding this comment

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

Ok, after this first review, I'd like to highlight what I think are the important things we have
to agree on to facilitate future PRs. Let me know what you think:

  1. Each PR has to be focused to a very specific feature or bug fix.
  2. The PR should not introduce regressions. Tests will be run when available and
    manual user interface testing has to be done.

Expand Down