[sip-comm-dev] whiteboard patch


#1

Hi,
I made a new patch release for the whiteboard in SIP-Communicator.
Now we can :
- draw line/polygon/polyline/rectangle/text/free path/circle
- delete a shape
- move a shape
- modify points in a shape (dynamic modification control points, wysiwyg
editor)
- modify color/tickness
- save your whiteboard in jpeg or png

The GUI use now a "resources.properties" system for all the icons, and soon
for the texts

For the moment, it's limited for a "picto-chat" between only 2 persons,
and all the functionalities aren't activated
(and be careful it's a beta version)

And soon :
- a dialog properties for each shape
- Bezier curve
- svg import
- zoom
.. and many other things :wink:

patch is here :
http://caotic.free.fr/sip/whiteboard2.patch

Regards,
Julien


#2

Hello Julien,

I've finished reviewing your patch yesterday night. You've done a very
good work and you've done a lot of it! :slight_smile: Your white board works very
well and I can't wait to have it all in the source tree and use it!
Thanks for all you hard work!

Now, let's get to the details. I have decided that it would be best for
us to start from the service. I've made a number of modifications in
there so you'd probably need to modify both your implementation and UI.
Following is a list of these modifications (as well as a list of generic
comments). Please review them and let me know if you have trouble
understanding the reasoning behind them or if you simply disagree.

Generic comments on the code:

* Javadocs. I see that you've been making efforts to have javadoc
comments but we need to be more rigorous than this. I would really like
_every_ method and field to have javadocs. This is absolutely mandatory
for public, protected and package fields and methods and is strongly
recommended even for private ones. The reason why this is important is
so that people that would like to debug, modify, or improve your code be
able to do so as easily as possible. I know that it could often be hard
to find inspiration but what you need to do in javadocs is simply say
what the method is supposed to do. This way it is a lot easier to
understand it (I am saying this from experience: there's nothing harder
than to figure what a method is doing and why it is doing it when you
don't know what it is supposed to be doing :slight_smile: ).

This is even more important for service classes and interfaces. I've
added (and improved where necessary) javadoc comments in all service
interfaces but if you decide to add stuff there later, please make sure
that you also add good javadocs. There are many reasons why you need to
do so: 1) There's no code to look at in interfaces so if you don't
understand tfrom the javadocs what exactly it is that the method is
supposed to do, things would become very difficult. 2) Your comments
would not only be used by users of the service, but also by future
implementors, so they need to be 100% clear as to what is the behavior
that they should be implementing.

One last thing on javadocs. I've seen that in some cases you have
default IDE generated comments:

/**
* method

···

*
* @param param1 param1
*/
public void method() ...

Well, this is definitely not a piece of javadoc that we could accept :slight_smile:
. IDE's generate such comments in order to make it easier for you to
fill in the rest, and not because you are supposed to leave them as they
are.

* All javadocs MUST start with a capital letter.

* Make sure you leave at least one empty line after each method/field
definition/declaration. I mean I've been often seen definitions like these:

/**
  * javadocs
  */
public void method1()
{
     ...
}
/**
  * javadocs
  */
public void method2()
{
     ...
}

In cases like the one above, there should be a new line after the end of
method1 and after the end of method2.

* Opening braces must be on a line of their own (just as they are in
method1 and method2 in the example above). You can have a look at our
code convention here:

http://www.sip-communicator.org/index.php/Documentation/CodeConvention

* I've changed your author name from
@author julien
to
@author Julien Waechter

in order to avoid potential conflicts with future Julien contributors :slight_smile:

* I've seen that you've been using System.out.println() for logging
purposes in some cases. You'd have to replace this with usage of our
Logger. You can see more on logging here:

http://www.sip-communicator.org/index.php/Documentation/LogLevels

* We've discussed this off-list already but I'll add it here for the
sake of completeness: We try to keep the code IDE independent, so
there's no need to add your .form Netbeans forms in the patch. I'd
actually even discourage you from using the Netbeans editor in this
case. The whiteboard UI is fairly simple and could easily be written by
hand so the unreadability of automatically generated code is not really
justified. This is not a priority though so you don't need to re-write
it immediately.

I think these are all my generic comments. I know Yana has also been
looking at your code so she may have something else to add.

Now, here's the list of modifications that I've been making on the
service classes and interfaces:

* WhiteboardObject.java

-- There were no javadocs for the static fields. I've tried to add some
but you may want to have a look. For example, it would have been worth
mentioning the difference between polyline and path.

-- I've remove the WB_ prefix for all types and replaced it with a TYPE_
prefix

-- I've rewritten most of the javadocs.

-- I've removed the setID() method. ID's should be generated by the
service implementation upon creation of an object and there should be no
possibility to change an ID after creating an object.

-- I've removed the setType() method. I was having doubts here, though.
Can the type of an object really change during a session? If you think
so then you could bring it back. (In the mean time I have added the
corresponding param in the WhiteboardSession.createWhiteboardObject()
method)

-- I've remove the action static fields as well as the action get and
set methods. Actions do make sense in the XML syntax of the protocol but
there's not much point in having them in our WhiteboardObject interface.
The way we have it now objects are supposed to be more or less passive
things and it is up to the WhiteboardSession to do actions with them.
(I've added a delete and move method in WhiteboardSession)

-- Attributes. If we decide to keep using attributes then we should
define the set of allowed attribute keys as static fields in the
WhiteboardObject interface so that they are part of the "contract"
between WB plugins and WB implementations. I am not so sure we should
keep using attributes though. Actually I am fairly certain that we
shouldn't :slight_smile: . There are many things that may go wrong with them.
Attributes for example, don't allow you to specify that "r" is mandatory
for circles and not allowed for lines.

I guess I'd be more in favor of extending WhiteboardObject with
interfaces for every existing type of figure and adding the
corresponding create methods in the WhiteboardSession. (I think you are
already doing it this way in your implementation).

-- The javadoc of getPoints() was not specifying the type of objects
that it was returning. It is very important to explicitly state this in
documentation for methods that have a generic return type like List,
Vector, Map and so on, without using 1.5 Generics.

I saw that in your implementation you were inserting 2-element arrays in
the list. I thought that it would instead be better to have a
WhiteboardPoint interface in our service, so I've added one and adjusted
javadocs accordingly.

-- I've renamed the [get|set]Background() methods to
[get|set]BackgroundImage() to avoid confusion with getting and setting
the background color.

-- Added [get|set]BackgroundColor() methods

-- Can you please confirm that WhiteboardObjects are passive and changes
made on them won't be seen by other participants until you call the
WhiteboardSession.sendWhiteboardObject() method? If this is the case
then it would probably be a good idea to say so in the WhiteboardObject
class javadocs.

-- I am not sure I am comfortable with the setPoints() method, as it
gives the user the possibility to potentially provoke inconsistencies
and, for example, set for a circle points that actually define a
triangle. I guess that if we decide to go for the one-class-per-figure
approach that I've mentioned above, we should only have setPoints() for
interfaces where it would make sense to have arbitrary points - like for
example path and polyline.

* WhiteboardObjectModifiedEvent

-- Modified the class javadocs

-- Added comments for the private WhiteboardObject obj field

* WhiteboardObjectDeliveryFailedEvent

-- Added getSourceWhiteboardObject() method

* WhiteboardObjectDelivered

-- Renamed getSourceWbObject() -> getSourceWhiteboardObject() for
consistency

* WhiteboardObjectEvent

-- Did not commit this class as it doesn't seem to be used.

* WhiteboardParticipantEvent

-- There were no class javadocs.
-- The source must always be the object that you are adding the listener
to, so I've changed this to be the whiteboard session.

* WhiteboardParticipantListener

-- Removed the methods that were indicating changes in transport
address, and address, as they don't seem relevant in the context of
whiteboarding.

* WhiteboardParticipant

-- Removed unused import statements (java.util and java.net)

* WhiteboardSession

-- Removed unnecessary imports of util and awt.Shape
-- Modified javadocs

There's no reason to specify the fire methods inside the service since
no one would be calling them from the outside so I've:

-- Removed fireWhiteboardChangeEvent()
-- Removed fireWhiteboardParticipantEvent()

-- Renamded [set|get]WhiteboardState() to [set|get]State() (I've also
renamed the state class)

-- Removed ID param for createWbObject() as the ID has to be generated
by the implementation, and Added a type param.

-- Added a deleteWbObject() and moveWhiteboardObject() methods as I've
removed the action fields from WhiteboardObject

* WhiteboardState

-- Renamed WhiteboardState to WhiteboardSessionState

* OperationSetBasicWhiteboarding

-- Renamed it to OperationSetWhiteboarding. I know that we decided the
basic part of the name together, but you've created a very complete
service so it's not that basic any more! :slight_smile:

-- Added a session param on the endSession() method so that we know
which session we're ending.
-- Resolved javadoc conflicts in createWhiteboardSession()

OK, these are all the changes that I've noted. There might be others so
please watch our for modifications that I haven't described here.

One last thing: I know that the above is a long list of modifications
but don't worry about it. This is normal and it doesn't mean that you've
been doing things badly. You've done a great job, Julien, all that's
left is polish it and get it ready for integration!

Cheers and good luck!
Emil

Julien wrote:

Hi, I made a new patch release for the whiteboard in
SIP-Communicator. Now we can : - draw
line/polygon/polyline/rectangle/text/free path/circle - delete a
shape - move a shape - modify points in a shape (dynamic modification
control points, wysiwyg editor) - modify color/tickness - save your
whiteboard in jpeg or png

The GUI use now a "resources.properties" system for all the icons,
and soon for the texts

For the moment, it's limited for a "picto-chat" between only 2
persons, and all the functionalities aren't activated (and be careful
it's a beta version)

And soon : - a dialog properties for each shape - Bezier curve - svg
import - zoom .. and many other things :wink:

patch is here : http://caotic.free.fr/sip/whiteboard2.patch
<http://caotic.free.fr/sip/whiteboard2.patch>

Regards, Julien

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#3

Hi Julien,

I completely agree with Emil, you have done a very good job !:slight_smile: I'm so impatient to use the whiteboard.

As Emil said, I was also looking at your code and I have also two three little notes to add to Emil's list.

* I have noticed some variable names and some internal code comments in French:) For example in WhiteboardObjectJabberImpl you have two fields named: epaisseur and listePTS. You have to replace them by thickness and pointsList for example. Also in the WhiteboardPluginSession I found the following comment: // personne non encore presente. Although these looks accidental for me could you please go through the code and ensure you have all in English.

* The second thing is that I saw some commented lines in the code (like for example in WhiteBoardFrame line 2104 : //selectedShape.setSelected(false);), which is something absolutely normal for a work in progress. I just wanted to remind you to remove the code that is useless and to add some more comments for the commented code that you prefer to keep. The comments should only explain why this code is there and why you prefer to keep it commented.

That's all from me.

Again very good work!

Yana

Emil Ivov wrote:

···

Hello Julien,

I've finished reviewing your patch yesterday night. You've done a very
good work and you've done a lot of it! :slight_smile: Your white board works very
well and I can't wait to have it all in the source tree and use it!
Thanks for all you hard work!

Now, let's get to the details. I have decided that it would be best for
us to start from the service. I've made a number of modifications in
there so you'd probably need to modify both your implementation and UI.
Following is a list of these modifications (as well as a list of generic
comments). Please review them and let me know if you have trouble
understanding the reasoning behind them or if you simply disagree.

Generic comments on the code:

* Javadocs. I see that you've been making efforts to have javadoc
comments but we need to be more rigorous than this. I would really like
_every_ method and field to have javadocs. This is absolutely mandatory
for public, protected and package fields and methods and is strongly
recommended even for private ones. The reason why this is important is
so that people that would like to debug, modify, or improve your code be
able to do so as easily as possible. I know that it could often be hard
to find inspiration but what you need to do in javadocs is simply say
what the method is supposed to do. This way it is a lot easier to
understand it (I am saying this from experience: there's nothing harder
than to figure what a method is doing and why it is doing it when you
don't know what it is supposed to be doing :slight_smile: ).

This is even more important for service classes and interfaces. I've
added (and improved where necessary) javadoc comments in all service
interfaces but if you decide to add stuff there later, please make sure
that you also add good javadocs. There are many reasons why you need to
do so: 1) There's no code to look at in interfaces so if you don't
understand tfrom the javadocs what exactly it is that the method is
supposed to do, things would become very difficult. 2) Your comments
would not only be used by users of the service, but also by future
implementors, so they need to be 100% clear as to what is the behavior
that they should be implementing.

One last thing on javadocs. I've seen that in some cases you have
default IDE generated comments:

/**
* method
*
* @param param1 param1
*/
public void method() ...

Well, this is definitely not a piece of javadoc that we could accept :slight_smile:
. IDE's generate such comments in order to make it easier for you to
fill in the rest, and not because you are supposed to leave them as they
are.

* All javadocs MUST start with a capital letter.

* Make sure you leave at least one empty line after each method/field
definition/declaration. I mean I've been often seen definitions like these:

/**
  * javadocs
  */
public void method1()
{
     ...
}
/**
  * javadocs
  */
public void method2()
{
     ...
}

In cases like the one above, there should be a new line after the end of
method1 and after the end of method2.

* Opening braces must be on a line of their own (just as they are in
method1 and method2 in the example above). You can have a look at our
code convention here:

http://www.sip-communicator.org/index.php/Documentation/CodeConvention

* I've changed your author name from
@author julien
to
@author Julien Waechter

in order to avoid potential conflicts with future Julien contributors :slight_smile:

* I've seen that you've been using System.out.println() for logging
purposes in some cases. You'd have to replace this with usage of our
Logger. You can see more on logging here:

http://www.sip-communicator.org/index.php/Documentation/LogLevels

* We've discussed this off-list already but I'll add it here for the
sake of completeness: We try to keep the code IDE independent, so
there's no need to add your .form Netbeans forms in the patch. I'd
actually even discourage you from using the Netbeans editor in this
case. The whiteboard UI is fairly simple and could easily be written by
hand so the unreadability of automatically generated code is not really
justified. This is not a priority though so you don't need to re-write
it immediately.

I think these are all my generic comments. I know Yana has also been
looking at your code so she may have something else to add.

Now, here's the list of modifications that I've been making on the
service classes and interfaces:

* WhiteboardObject.java

-- There were no javadocs for the static fields. I've tried to add some
but you may want to have a look. For example, it would have been worth
mentioning the difference between polyline and path.

-- I've remove the WB_ prefix for all types and replaced it with a TYPE_
prefix

-- I've rewritten most of the javadocs.

-- I've removed the setID() method. ID's should be generated by the
service implementation upon creation of an object and there should be no
possibility to change an ID after creating an object.

-- I've removed the setType() method. I was having doubts here, though.
Can the type of an object really change during a session? If you think
so then you could bring it back. (In the mean time I have added the
corresponding param in the WhiteboardSession.createWhiteboardObject()
method)

-- I've remove the action static fields as well as the action get and
set methods. Actions do make sense in the XML syntax of the protocol but
there's not much point in having them in our WhiteboardObject interface.
The way we have it now objects are supposed to be more or less passive
things and it is up to the WhiteboardSession to do actions with them.
(I've added a delete and move method in WhiteboardSession)

-- Attributes. If we decide to keep using attributes then we should
define the set of allowed attribute keys as static fields in the
WhiteboardObject interface so that they are part of the "contract"
between WB plugins and WB implementations. I am not so sure we should
keep using attributes though. Actually I am fairly certain that we
shouldn't :slight_smile: . There are many things that may go wrong with them.
Attributes for example, don't allow you to specify that "r" is mandatory
for circles and not allowed for lines.

I guess I'd be more in favor of extending WhiteboardObject with
interfaces for every existing type of figure and adding the
corresponding create methods in the WhiteboardSession. (I think you are
already doing it this way in your implementation).

-- The javadoc of getPoints() was not specifying the type of objects
that it was returning. It is very important to explicitly state this in
documentation for methods that have a generic return type like List,
Vector, Map and so on, without using 1.5 Generics.

I saw that in your implementation you were inserting 2-element arrays in
the list. I thought that it would instead be better to have a
WhiteboardPoint interface in our service, so I've added one and adjusted
javadocs accordingly.

-- I've renamed the [get|set]Background() methods to
[get|set]BackgroundImage() to avoid confusion with getting and setting
the background color.

-- Added [get|set]BackgroundColor() methods

-- Can you please confirm that WhiteboardObjects are passive and changes
made on them won't be seen by other participants until you call the
WhiteboardSession.sendWhiteboardObject() method? If this is the case
then it would probably be a good idea to say so in the WhiteboardObject
class javadocs.

-- I am not sure I am comfortable with the setPoints() method, as it
gives the user the possibility to potentially provoke inconsistencies
and, for example, set for a circle points that actually define a
triangle. I guess that if we decide to go for the one-class-per-figure
approach that I've mentioned above, we should only have setPoints() for
interfaces where it would make sense to have arbitrary points - like for
example path and polyline.

* WhiteboardObjectModifiedEvent

-- Modified the class javadocs

-- Added comments for the private WhiteboardObject obj field

* WhiteboardObjectDeliveryFailedEvent

-- Added getSourceWhiteboardObject() method

* WhiteboardObjectDelivered

-- Renamed getSourceWbObject() -> getSourceWhiteboardObject() for
consistency

* WhiteboardObjectEvent

-- Did not commit this class as it doesn't seem to be used.

* WhiteboardParticipantEvent

-- There were no class javadocs.
-- The source must always be the object that you are adding the listener
to, so I've changed this to be the whiteboard session.

* WhiteboardParticipantListener

-- Removed the methods that were indicating changes in transport
address, and address, as they don't seem relevant in the context of
whiteboarding.

* WhiteboardParticipant

-- Removed unused import statements (java.util and java.net)

* WhiteboardSession

-- Removed unnecessary imports of util and awt.Shape
-- Modified javadocs

There's no reason to specify the fire methods inside the service since
no one would be calling them from the outside so I've:

-- Removed fireWhiteboardChangeEvent()
-- Removed fireWhiteboardParticipantEvent()

-- Renamded [set|get]WhiteboardState() to [set|get]State() (I've also
renamed the state class)

-- Removed ID param for createWbObject() as the ID has to be generated
by the implementation, and Added a type param.

-- Added a deleteWbObject() and moveWhiteboardObject() methods as I've
removed the action fields from WhiteboardObject

* WhiteboardState

-- Renamed WhiteboardState to WhiteboardSessionState

* OperationSetBasicWhiteboarding

-- Renamed it to OperationSetWhiteboarding. I know that we decided the
basic part of the name together, but you've created a very complete
service so it's not that basic any more! :slight_smile:

-- Added a session param on the endSession() method so that we know
which session we're ending.
-- Resolved javadoc conflicts in createWhiteboardSession()

OK, these are all the changes that I've noted. There might be others so
please watch our for modifications that I haven't described here.

One last thing: I know that the above is a long list of modifications
but don't worry about it. This is normal and it doesn't mean that you've
been doing things badly. You've done a great job, Julien, all that's
left is polish it and get it ready for integration!

Cheers and good luck!
Emil

Julien wrote:

Hi, I made a new patch release for the whiteboard in
SIP-Communicator. Now we can : - draw
line/polygon/polyline/rectangle/text/free path/circle - delete a
shape - move a shape - modify points in a shape (dynamic modification
control points, wysiwyg editor) - modify color/tickness - save your
whiteboard in jpeg or png

The GUI use now a "resources.properties" system for all the icons,
and soon for the texts

For the moment, it's limited for a "picto-chat" between only 2
persons, and all the functionalities aren't activated (and be careful
it's a beta version)

And soon : - a dialog properties for each shape - Bezier curve - svg
import - zoom .. and many other things :wink:

patch is here : http://caotic.free.fr/sip/whiteboard2.patch

Regards, Julien

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net