[In design terms, Yoshiki's solution is very simple] I'm very glad to hear that.
[acknowledgement and fixes to small issues raised] Now you've moved it a little bit closer to being the simplest thing that could possibly work. Considering the review you gave generally, it's quite probably close enough, too. Of course, we'll respect Yoshikis wish to wait for his new version.
[generally - does code need to be perfect before we'll even look at it?] Depends. If the code is two isSymbol methods, yes, it needs to be perfect. OTOH, for something that simple, I don't mind if the implementor is the reviewer is the approver.
For the other extreme case, something that touches lots of innards, and affects everything, like Anthony's closure changes, it's obvious to me that there's no way to make it perfect. However, to minimize the pain in changes to more publically released code, such as images, I would definitely want it to be reviewed by at least one other expert in the area, and preferably at least one other, and that the issues they raise be dealt with. It still won't be perfect, but it'll be doing reasonable justice to future users of the code. And since I know I'm not qualified to do justice to his changes, I currently see no way my even looking at it can help, really (though in this specific case, I tried anyway).
Yoshiki's code is somewhere between the two extremes, and should be treated accordingly. With a review by you and a review by Goran, and a response to those issues when Yoshiki can get to it, I'm sure we'll be in good shape.
[What are the measures?] Note that "the simplest thing that could possibly work" is to be interpreted very widely. It means it should have comments, but only comments that actually help. All the good things you mentioned about Yoshiki's solutions to various problems fall under this too. As long as one can easily point out something that is definitely an improvement, however small, it isn't the simplest thing that could possibly work. Of course to avoid infinite effort, we need to be practical, so see the paragraphs above about limiting the effort.
A very good starting point to see what this includes is http://c2.com/cgi/wiki?DoTheSimplestThingThatCouldPossiblyWork and http://c2.com/cgi/wiki?WhatIsSimplest
Now does it work for you both abstractly and concretely? Do you have some suggestions for improvement? (I'll summarize and post these as a draft for review rules).
[critique about implicitly returning self method] If this method was in use, it would be very nearly semantically equivalent to not existing, since it does nothing except a super send. Very nearly, but not quite, because it doesn't return the super sends result. Which could, imaginably, be justified, but is much more likely to simply a source of endless confusion if someone modifies the super implementations return value, and then, sometimes, "his change doesn't work".
Daniel
Andreas Raab andreas.raab@gmx.de wrote:
This is a multi-part message in MIME format.
--Boundary_(ID_JoGVnwLKXwZRd95pmxwCeg) Content-type: text/plain; charset=us-ascii Content-transfer-encoding: 7BIT
Andreas, does this help?
Abstractly yes, concretely I'm not sure. I'll give you my take on it in the TTF discussion:
- Is it the simplest thing that could possibly work?
I would claim so. When Yoshiki first did it I was more than just a little surprised to see not only that he'd done it but also how simple it was. I had been looking into this myself and there were a number of problems where I simply had no idea how to do them (most importantly how to deal with color changes in the text). Given that Yoshiki's code does not require any kind of low-level surgery (such as new primitives, BitBlt combination rules etc), yes I claim it is absolutely the simplest thing that could possibly work.
- Does it have dead code?
The one method you mentioned (and where I still don't understand the critique about sending to super and implicitly returning self ;-) seems unused. I've attached a fix which simply removes it.
- Does it have extensions to classes which are not strictly required?
All of the extensions are required.
- Does it break or undo other peoples work?
There is one conflict with an MCP change to which a fix is attached.
Summary: If I take your measures then to me everything's fine with the two attached minor changes.
Cheers,
- Andreas
-----Original Message----- From: squeakfoundation-bounces@lists.squeakfoundation.org [mailto:squeakfoundation-bounces@lists.squeakfoundation.org] On Behalf Of Daniel Vainsencher Sent: Sunday, June 22, 2003 3:33 AM To: Discussing the Squeak Foundation Subject: RE: [Squeakfoundation]re: TrueType font support and 3.6
I'll take that as a reminder that we haven't yet created the official submission/review policy we talked about last week. Here's a draft -
Code submitted should be the simplest thing that could possibly work (*), where "work" is defined as providing the functionality promised (ENH) or correcting the bug (FIX) without creating others.
(*) note that this precludes, among other forms of complexity -
- Dead code that does nothing.
- Extensions to existing classes that are not strictly
required for the code to work. 3. Breaking or undoing other peoples changes.
Andreas, does this help?
Daniel
Andreas Raab andreas.raab@gmx.de wrote:
Hi Daniel,
If alpha is closed without the TTF stuff it means you've delayed the most important improvement to Squeak's user interface in the last three years.
Have you ever failed a test because you didn't put in the effort and had to do it over? did you blame the teacher for writing the test?
Yes, but in these situations the rules were clear to me and
I knew what
exactly I should have done differently.
Again: Please explain to me what your desired code quality
is. What measures
do you use? What are the rules?
You may see things so that you think that I'm delaying
this code, but
this code could easily be cleaner, and you know it, and you can do something about it just as easily as me.
Only if I know the rules. Otherwise I can only wait until
there is some
critique here or there and depending on your workload this
may take days,
weeks, or months.
[Conflicts are hard to find] Well, yes. Do you think that the fact that conflict
detection is hard
means that we should ignore it?
You lost me here. Finding potential conflicts with other
packages is quite
simple once you've got the stuff loaded. Just go into a
dual change sorter
and choose "conflicts with other change sets" and it'll
list you all of
them. Then you turn on the diffs and voila! all your
conflicts get nicely
highlighted. Fixing them can be a different matter though ;-)
[DecPools and KCP] The fact that conflicts with packages are hard to solve is a sad consequence of not having package releases. It is made more difficult by having more packages, but the solution is not in going back to a monolithic image, but in improving our infrastructure (SM1.1).
You lost me again. I don't see how the above is related to
my question about
how to handle a concrete case in a concrete situation.
Please explain.
Wish I could give you a release date for that, but I can't.
I sure hope it's soon. I already started to manually mirror
important
packages in order to be able to rely on the right versions.
It sucks like
hell but there's nothing else I can do.
When we're talking about a package, like RB, what I did
was fork it into
two packages. When we're talking about a patch like
DecPools, I would
track
the current alpha, because the version for an old 3.6 won't be
particularly
useful to anyone.
I think it's quite valuable to have it available for older
Squeak versions.
The point is that someone may want to load a package which
contains a
declarative pool in something like 3.4/3.5. Having the
package at SqueakMap
means that they have at least a chance to try it.
Cheers,
- Andreas
Squeakfoundation mailing list Squeakfoundation@lists.squeakfoundation.org http://lists.squeakfoundation.org/listinfo/squeakfoundation
Squeakfoundation mailing list Squeakfoundation@lists.squeakfoundation.org http://lists.squeakfoundation.org/listinfo/squeakfoundation
--Boundary_(ID_JoGVnwLKXwZRd95pmxwCeg) Content-type: application/octet-stream; name=RemoveGrafportMethod.1.cs Content-transfer-encoding: quoted-printable Content-disposition: attachment; filename=RemoveGrafportMethod.1.cs
'From Squeak3.6alpha of ''17 March 2003'' [latest update: #5278] on 22 = June 2003 at 2:15:28 pm'!=0DGrafPort removeSelector: = #installTTCFont:foregroundColor:backgroundColor:!=0D=
--Boundary_(ID_JoGVnwLKXwZRd95pmxwCeg) Content-type: application/octet-stream; name=FixConflict.1.cs Content-transfer-encoding: quoted-printable Content-disposition: attachment; filename=FixConflict.1.cs
'From Squeak3.6alpha of ''17 March 2003'' [latest update: #5278] on 22 = June 2003 at 2:16:22 pm'!=0D=0D!CanvasDecoder methodsFor: 'decoding' = stamp: 'ar 6/22/2003 14:16'!=0DprocessCommand: command onForceDo: = forceBlock=0D | verb verbCode |=0D command isEmpty ifTrue: [ ^self = ].=0D=0D verb _ command first.=0D verbCode :=3D verb first.=0D=0D = verbCode =3D CanvasEncoder codeClip ifTrue: [ ^self setClip: command = ].=0D verbCode =3D CanvasEncoder codeTransform ifTrue: [ ^self = setTransform: command ].=0D verbCode =3D CanvasEncoder codeText ifTrue: = [ ^self drawText: command ].=0D verbCode =3D CanvasEncoder codeLine = ifTrue: [ ^self drawLine: command ].=0D verbCode =3D CanvasEncoder = codeRect ifTrue: [ ^self drawRect: command ].=0D verbCode =3D = CanvasEncoder codeBalloonRect ifTrue: [ ^self drawBalloonRect: command = ].=0D verbCode =3D CanvasEncoder codeBalloonOval ifTrue: [ ^self = drawBalloonOval: command ].=0D verbCode =3D CanvasEncoder = codeInfiniteFill ifTrue: [ ^self drawInfiniteFill: command ].=0D=0D = verbCode =3D CanvasEncoder codeOval ifTrue: [ ^self drawOval: command = ].=0D verbCode =3D CanvasEncoder codeImage ifTrue: [ ^self drawImage: = command ].=0D verbCode =3D CanvasEncoder codeReleaseCache ifTrue: [ = ^self releaseImage: command ].=0D verbCode =3D CanvasEncoder codePoly = ifTrue: [ ^self drawPoly: command ].=0D verbCode =3D CanvasEncoder = codeStencil ifTrue: [ ^self drawStencil: command ].=0D verbCode =3D = CanvasEncoder codeForce ifTrue: [ ^self forceToScreen: command = withBlock: forceBlock ].=0D verbCode =3D CanvasEncoder codeFont ifTrue: = [ ^self addFontToCache: command ].=0D verbCode =3D CanvasEncoder = codeTTCFont ifTrue: [ ^self addTTCFontToCache: command ].=0D verbCode = =3D CanvasEncoder codeExtentDepth ifTrue: [ ^self extentDepth: command = ].=0D verbCode =3D CanvasEncoder codeShadowColor ifTrue: [ ^self = shadowColor: command ].=0D=0Dself error: 'unknown command: ', command = first.! !=0D=0D=
--Boundary_(ID_JoGVnwLKXwZRd95pmxwCeg) MIME-version: 1.0 Content-type: text/plain; charset=us-ascii Content-transfer-encoding: 7BIT
Squeakfoundation mailing list Squeakfoundation@lists.squeakfoundation.org http://lists.squeakfoundation.org/listinfo/squeakfoundation
--Boundary_(ID_JoGVnwLKXwZRd95pmxwCeg)--
squeakfoundation@lists.squeakfoundation.org