Hi Bert, thanks for reviewing my code!<div><div><br></div><div><div><div class="gmail_quote">On Mon, Aug 9, 2010 at 6:20 PM, Bert Freudenberg <span dir="ltr">&lt;<a href="mailto:bert@freudenbergs.de">bert@freudenbergs.de</a>&gt;</span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">We should not add a package named &quot;GSoC&quot;. Rather, move the classes/extension methods to the appropriate Etoys or Morphic package. Also, give them an appropriate name (the prefix &quot;GSoC&quot; will not make sense next year).</div>

</blockquote><div><br></div><div>Yes, it doesn&#39;t make sense. I&#39;ll move it to the Etoys package.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

GSoCTextMorph, GSoCScrollPane: do we really need new classes for this? Can&#39;t the behavior incorporated in the existing classes?<br></blockquote><div><br></div><div>I knew this classes wouldn&#39;t stay long but at the time I didn&#39;t want to modify existing classes so I hacked my way out :)</div>

<div><br></div><div>GSoCScrollPane simply defers halo to its interior. I think this could be added to ScrollPane without problems.</div><div><br></div><div>GSoCTextMorph adds the following behavior:</div><div>* It asks the compiler to execute its string in order to get its numeric value, then it checks if the returning value is a number. I know this is totally insecure but it&#39;s nice to be able to write &quot;1/3&quot; and get the correct result :). This feature forced me to create the SyntaxErrorOmission class, which is another hack. I could live without this if you think it&#39;s not worth it.</div>

<div>* It passes the focus to the next morph when the enter key is pressed. I could add this to TextMorph with a boolean property to avoid imposing this behavior to regular TextMorphs.</div><div>* It changes its border color to red when it gets the focus. I could also add this to TextMorph with a boolean property.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Also, all the new Player subclasses are a bit problematic. The canonical way is to just add the methods to Player itself (see the chat log).<br></blockquote><div><br></div><div>I don&#39;t really like adding methods to Player, I thought it wasn&#39;t necesary since &quot;look like&quot; was changed but I didn&#39;t take into account replacing receivers. I&#39;ll remove all my Player subclasses.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">We don&#39;t want a new class Vector, nor a method to: in Point.  It does not really seem useful enough to occupy that name. Also, VectorMorph is just a PolygonMorph - why subclass that at all?<br>

</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><br>
PointMorph seems particularly hacky. Is the whole point of this class to have an update method? Why can&#39;t any morph be used as a point in a diagram?<br></blockquote><div><br></div><div>The #to: method and the Vector class are not needed at all, but VectorMorph (as well as PointMorph and BarMorph) adds a specific viewer category to handle its properties inside a graph. I don&#39;t know how to implement this without making specific subclasses.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
So much for now. The educators really like your additions, now we just need to get the code into shape for inclusion :)<br>
<br>
Thank you for working on this!<br></blockquote><div><br></div><div>Thank you very much for taking the time to review my code. I&#39;ll fix the things you mention ASAP so it could be included in the next release :)</div><div>

<br></div><div>Cheers</div><div>Richo</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
- Bert -<br>
<br>
_______________________________________________<br>
etoys-dev mailing list<br>
<a href="mailto:etoys-dev@squeakland.org">etoys-dev@squeakland.org</a><br>
<a href="http://lists.squeakland.org/mailman/listinfo/etoys-dev" target="_blank">http://lists.squeakland.org/mailman/listinfo/etoys-dev</a><br>
</blockquote></div><br></div></div></div>