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

Deskshare module #28

Merged
merged 5 commits into from Jun 12, 2014

Conversation

Projects
None yet
2 participants
@syeshchenko
Copy link
Contributor

syeshchenko commented Jun 9, 2014

No description provided.

@@ -154,6 +156,7 @@
stage.addEventListener(Event.DEACTIVATE, fl_Deactivate);
stage.addEventListener(StageOrientationEvent.ORIENTATION_CHANGING, stageOrientationChangingHandler);
stage.scaleMode = flash.display.StageScaleMode.NO_SCALE;

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

I don't think you need this

return _isStreamingSignal;
}

public function get mouseLocationChangedSignal():ISignal

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

The method name should be the same as the variable

_video.height = screenHeight;
_video.width = ((_originalVideoWidth * _video.height) / _originalVideoHeight);
_video.x = screenWidth/2 + _video.width/2;
_video.y = screenHeight+50;

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

Shouldn't have magic numbers in calculations

public override function execute():void
{
trace("Checking if Desk Share stream is streaming.");
deskshareService.checkIfStreamIsPublishing(conferenceParameters.room);

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

Is it really necessary to have a signal for one line?

{
trace("DeskShare-deskshareStreamStopped.");
userSession.deskshareConnection.isStreaming = false;
userSession.deskshareConnection.isStreamingSignal.dispatch(userSession.deskshareConnection.isStreaming);

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

You shouldn't dispatch the signal of a different class. You can do the dispatching inside of the setter for isStreaming

*/
public function mouseLocationCallback(x:Number, y:Number):void
{
userSession.deskshareConnection.mouseLocationChangedSignal.dispatch(x, y);

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

Same thing here, create a method to set the x and y and then dispatch from within the connection class

if (!userSession.deskshareConnection.streamCheckedOnStartup)
{
checkDeskshareSignal.dispatch();
userSession.deskshareConnection.streamCheckedOnStartup = true;

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

What is this for?


private var userName:String;

public function get aspectRatio():Number

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

There's no reason for the private variables to be exposed with public getters and setters.

_ns.play(streamName);
}

public function resizeVideoRotatedUpsideDown(screenHeight:Number, screenWidth:Number):void

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

The only difference between UpsideDown and rightside up is the positioning calculation. You can move the resize code to two methods to reduce duplication. One method for portrait calculations and one for landscape calculations.


public override function startStream(connection:NetConnection, name:String, streamName:String, userID:String, width:Number, height:Number, screenHeight:Number=0, screenWidth:Number=0):void
{
super.streamName = streamName;

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

You don't need to use super to before all of these variables

}
}

public override function rotateVideo(rotation:Number, screenHeight:Number, screenWidth:Number):void

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

A lot of this is a duplicate of the super version. You can just call super and do whatever else extra you need to do after.

}
}

private function onNetStatus(e:NetStatusEvent):void{

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

Shouldn't need this method, just let super handle it

deskshareVideoView.moveMouse(x, y);
}

public function addMouseToStage():void

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

Why not add the mouse on stream start? Is there a reason to keep them separate?

{
import org.bigbluebutton.view.navigation.pages.common.VideoView;

public class VideoChatVideoView extends VideoView

This comment has been minimized.

@capilkey

capilkey Jun 9, 2014

Member

Does this class actually have a purpose?

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

@capilkey capilkey merged commit 6d6056e into bigbluebutton:master Jun 12, 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.