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

Rewrote services to use new message system #29

Merged
merged 5 commits into from Jun 23, 2014

Conversation

Projects
None yet
2 participants
@Adam-Sharpe
Copy link
Contributor

Adam-Sharpe commented Jun 9, 2014

-No more shared objects! Now, just message senders and message receivers.
-Slight changes to the model.
-Some classes that were no longer necessary were removed (new info comes in with JSON strings)

-TO DO: Lock settings need to be done properly (had to "fake" some of them on the connect call for the BigBlueButtonConnection.baseConnection.netConnection.connect(...) call :P )

injector.map(IUsersService).toSingleton(UsersService);
injector.map(IUsersMessageSender).toSingleton(UsersMessageSender);

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

Do the message senders and receivers need to be singletons?

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

After looking at these senders and receivers closer I don't think you need to map them at all. You can just create and set them in their corresponding services.

// Setup all the message receivers for the services that use the BigBlueButtonConnection:
usersService.setupMessageReceiver();
chatService.setupMessageReceiver();
presentationService.setupMessageReceiver();

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

I think it would be better to setup the chat and presentation services after the userSession.success has been received.

*/
public function mouseLocationCallback(x:Number, y:Number):void
{
setMouseCoordinates(x, y);

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

Why is the mouse location callback just in turn calling another public method that sets the coordinates?


// Add the presentations to the presentation list:
trace("PresentMessageReceiver::handleGetPresentationInfoReply() -- adding presentation [" + p.name + "] to the presentation list");
userSession.presentationList.addPresentation(p.name, p.pages.length, p.current);

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

Instead of having the return of addPresentation as void you could have it return a reference to the newly created Presentation object. You then wouldn't have to make the subsequent call to getPresentation.

private function handleSharePresentationCallback(m:Object):void {
var msg:Object = JSON.parse(m.msg);
trace("PresentMessageReceiver::handleSharePresentationCallback() -- now showing presentation [" + msg.presentation.name + "]");
userSession.presentationList.getPresentation(msg.presentation.name).show();

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

If the presentation can't be found for some reason this will throw an exception.

// Add all the slides to the presentation:
for(var i:int = 0; i < length; i++) {
var s:Object = presentation.pages[i];
pres.add(new Slide(s.num, s.swfUri, s.thumbUri, s.txtUri, s.current));

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

This handler does almost exactly the same thing as handleGetPresentationInfoReply(), it would be better to make a separate method and call it from both to reduce duplication

private var presenterViewedRegionX:Number = 0;
private var presenterViewedRegionY:Number = 0;
private var presenterViewedRegionW:Number = 100;
private var presenterViewedRegionH:Number = 100;

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

These variables shouldn't be in the middle of the class either

}
if(user.raiseHand) {
userSession.userList.raiseHandChange(user.userID, user.raiseHand);
}

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

Why not do these three checks inside of the UserList.addUser() method?

userSession.userList.allUsersAddedSignal.dispatch();
}

private function handleParticipantJoined(m:Object):void {

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

The content of the handParticipantJoined is the exact same as each user in the handleGetUsersReply. Breaking it out will reduce code duplication.

/*
var msg:Object = JSON.parse(m.msg);
userSession.userList.getUser(msg.userId).listenOnly = msg.listenOnly;
*/

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

This should be implemented before merging the PR

This comment has been minimized.

@Adam-Sharpe

Adam-Sharpe Jun 19, 2014

Author Contributor

I added the basic logic to the model, but will wait until I merge the new CSS stuff before figuring out how the view should reflect this.

private function handleMeetingHasEnded(m:Object):void {
var msg:Object = JSON.parse(m.msg);
trace("UsersMessageReceiver::handleMeetingHasEnded() -- meeting has ended");
}

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

Is there no content for this handler?

private function handleRecordingStatusChanged(m:Object):void {
var msg:Object = JSON.parse(m.msg);
trace("UsersMessageReceiver::handleRecordingStatusChanged() -- recording status changed");
}

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

We need to think of a way to display to the user what the recording status of the meeting is

This comment has been minimized.

@Adam-Sharpe

Adam-Sharpe Jun 19, 2014

Author Contributor

I added the basic logic to the model, but will wait until I merge the new CSS stuff before figuring out how the view should reflect this.

userSession.mainConnection.sendMessage("participants.getParticipants", defaultSuccessResponse, defaultFailureResponse);
}

public function assignPresenter(userid:String, name:String, assignedBy:Number):void {

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

Why is the assignedBy parameter a Number?


var message:Object = new Object();
message["userId"] = userID;
message["loweredBy"] = userID;

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

userId and loweredBy aren't always the same. I think you need to split the raised and lowered parts into separate methods.

message["mute"] = mute;
message["exceptUsers"] = dontMuteThese;

userSession.mainConnection.sendMessage("voice.muteUnmuteUser", defaultSuccessResponse, defaultFailureResponse, message);

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

I think this is the wrong method on the server. The one that you are calling is the one to mute a single user.

var message:Object = new Object();
message["userId"] = userid;

userSession.mainConnection.sendMessage("voice.kickUSer", defaultSuccessResponse, defaultFailureResponse, message);

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

Is the typo in "voice.kickUSer" the same on the server?

This comment has been minimized.

@Adam-Sharpe

Adam-Sharpe Jun 18, 2014

Author Contributor

Yes, it is.

_fileName = fileName;
_slides = new Vector.<Slide>(numOfSlides + 1);

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

Are you still leaving _slides[0] empty? If so it means the size calculations are off

@@ -77,7 +77,8 @@ package org.bigbluebutton.view.navigation.pages.login

if(Capabilities.isDebugger)
{
url = "bigbluebutton://test-install.blindsidenetworks.com/bigbluebutton/api/join?fullName=Air&meetingID=Demo+Meeting&password=ap&checksum=512620179852dadd6fe0665a48bcb852a3c0afac";
url = "bigbluebutton://192.168.208.142/bigbluebutton/api/join?fullName=Adam-AIR&meetingID=Demo+Meeting&password=ap&checksum=3982b21e0fbb67664753f5165d9803bb1d1d26b4";
//url = "bigbluebutton://test-install.blindsidenetworks.com/bigbluebutton/api/join?fullName=Air&meetingID=Demo+Meeting&password=ap&checksum=512620179852dadd6fe0665a48bcb852a3c0afac";

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

You shouldn't be pushing your changes to the LoginPageViewMediator. Revert it back to before please

@@ -85,7 +88,7 @@ package org.bigbluebutton.view.navigation.pages.presentation
override public function destroy():void
{
userSession.presentationList.presentationChangeSignal.remove(presentationChangeHandler);
userSession.presentationList.slideChangeSignal.remove(slideChangeHandler);
_currentPresentation.slideChangeSignal.remove(slideChangeHandler);

This comment has been minimized.

@capilkey

capilkey Jun 17, 2014

Member

You should put a check for _currentPresentation != null

chatMessageSender.chatMessageService = this;

chatMessageReceiver.userSession = userSession;
chatMessageReceiver.chatMessagesSession = chatMessagesSession;

This comment has been minimized.

@capilkey

capilkey Jun 20, 2014

Member

I think it would be better to pass in the references as parameters to the constructor

public function setupMessageSenderReceiver():void {
chatMessageSender.userSession = userSession;
chatMessageSender.chatMessagesSession = chatMessagesSession;
chatMessageSender.chatMessageService = this;

This comment has been minimized.

@capilkey

capilkey Jun 20, 2014

Member

You should give the sender either a reference to the signal to dispatch or a callback function on the specific call instead of a reference to the full object

private var _sendPublicMessageOnFailureSignal:ISignal = new Signal;

private var _sendPrivateMessageOnSucessSignal:ISignal = new Signal;
private var _sendPrivateMessageOnFailureSignal:ISignal = new Signal;

This comment has been minimized.

@capilkey

capilkey Jun 20, 2014

Member

These four signals could be reduced down to two or even one potentially

This comment has been minimized.

@Adam-Sharpe

Adam-Sharpe Jun 20, 2014

Author Contributor

For the sake of consistency, I think I'll reduce it two signals, but not one for now... Many other signal pairs in the application follow the Onsuccess - OnFailure pattern. All such pairs could be reduced to one signal, but should they?

var presentation:Presentation = userSession.presentationList.getPresentation(msg.presentation.name);

if(presentation != null) {
userSession.presentationList.getPresentation(msg.presentation.name).show();

This comment has been minimized.

@capilkey

capilkey Jun 20, 2014

Member

You already retrieved a reference to the presentation on line 142 so you don't need to call getPresentation() again

capilkey added a commit that referenced this pull request Jun 23, 2014

Merge pull request #29 from SenecaCDOT-BigBlueButton/changing-service…
…s-to-work-with-bbb-version-0.9

Rewrote services to use new message system

@capilkey capilkey merged commit b8512cc into bigbluebutton:master Jun 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.