whiteboard patch

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!  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  ).

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 
. 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 

* 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/Docum
entation/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  . 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! 

-- 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 ;)
> 
> 
> patch is here : http://ca
otic.free.fr/sip/whiteboard2.patch 
> <http:
//caotic.free.fr/sip/whiteboard2.patch>
> 
> Regards, Julien

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
For additional commands, e-mail: dev-helpsip-communicator.dev.java.net

Hi Julien,

I completely agree with Emil, you have done a very good job
! 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!  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  ).
> 
> 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 
> . 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 
> 
> * 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/Docum
entation/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  . 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! 
> 
> -- 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 ;)
>>
>>
>> patch is here : http://ca
otic.free.fr/sip/whiteboard2.patch 
>> <http:
//caotic.free.fr/sip/whiteboard2.patch>
>>
>> Regards, Julien
> 
>
------------------------------------------------------------
---------
> To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
> For additional commands, e-mail: dev-helpsip-communicator.dev.java.net
> 
> 

------------------------------------------------------------
---------
To unsubscribe, e-mail: dev-unsubscribesip-communicator.dev.java.net
For additional commands, e-mail: dev-helpsip-communicator.dev.java.net