GROUP_FILLING_R0

[pav]

GROUP_FILLING_R0

Ticket	Summary	Effort	Status
#678	PAGE_FILLING_R0	1	closed
#765	FRAME_FILLING_IMAGE_R0	0.5	closed
#768	FRAME_FILLING_SOLID_R0	0.5	closed
#775	FRAME_FILLING_GRADIENT_R0	1	closed

[pav]

2h

[kyli]

3.5p

• Not mentioned what happens if the STRONG filling is not chosen.
• If the current "Background color" property is to be replaced by the filing, what should happen to the hud?
• Something like a diagram of the visible end result should be provided.
• Gradient fillings probably need some other settings - where should they be placed?
• Obsolete information about FRAME_FILLING_PATTERN_R0 was removed.

[pap]

• When creating design it is good to describe if classes should be made final, immutable, etc.
• You should mention if some annotations should be used on methods/classes
• You could rename Filling to ImmFilling and make it @Immutable
• Add a transform(ImmColor) method to Filling. It should take care of color transforms and transparency (there are already such methods for ImmColor and BufferedImage).
• Make sure that all implementations of the above method are really immutable.
• If there are no tests you could mention what tests should be implemented in the implementation phase.

• Besides that the pattern is not in the requirements of the task and also I don't think that the FRAME_APPEARANCE_HALO_AND_HUD_R0 really belongs to this group.

[pap]

Forgot to change the status in teh previous change

[pap]

Note: Make sure JXGradientChooser doesn't break the Felix execution of Sophie

[pav]

1.5d

[meddle]

The design is OK, but:

• The test is small and I think there are more things for testing, for the implementation phase there will be no place for "TODO" tests.
• You haven't removed FRAME_APPEARANCE_HALO_AND_HUD_R0 from the group as pap wanted.
• You had to say that ImmFilling is immutable despite of it's name.
• UML diagram would make the review shorter.

3p (30m)

[pap]

• It is obvious that the implementation is not according to the design so please correct the wiki page.
• You do not fulfill requirements of the task stated in the analysis section. This also needs to be mentioned and explained.
• ImmColor's equals method is bad -floats compared with ==
• It looks strange that ImmImage's fill method cals Graphics2D.dispose()
• ImmColor's transform method could just "return multiply(immColor);" It seems to me that the way it is now does nothing.
• All conditional and loop operators should have their bodies in curly braces even if they have only a single statement - your equals implementations
• Point2D is not immutable and its usage in ImmGradientPaint is not immutable.
• In ImmGradientTest you use assertTrue(blabla.equals(albAlb)
• I do not think that you really test the immutability of the ImmGradient in all cases(and that test fails). Also you obviously copied the method from ImmColor(see the JavaDoc).
• The methods of ImmGradient are not well ordered (see ​http://java.sun.com/docs/codeconv/html/CodeConventions.doc2.html#11684)
• You have a constructor which Javadoc states taht it makes a MultipleGradientPaint
• The TODO: toString method seems a bit strange to me
• The Javadoc at the top of the ImmGradient class is awful
• A little note is that you mentioned changeset 2608 in the design and it was merged int othe trunk so it is incorrect to mention it in the implementation too.
• Also I suppose that you may make ImmGradient a final class.(You can consult on this with someone else, too.)
• You may correct the Javadoc of ImmColor. It states it is an interface. Also the javadoc of methods is not fully correct.
• You may write a bit more explanatory Java doc of the transform method of ImmFilling
• I think that you may move the static method from transformImageColors ElementHelper into ImmImage (I suggest that you synchronize this with milo first).
• Also you may create some static method that converts the image to ARGB format because now we have lots of lots of code duplication which seems bad to me.
• The tranform method of ImmImage should take care of this conversion.
• The drawContent method of ShapeElementHelper is awful. First it should not do this check for ImmImage(see the previous bullet) and second it should call paint on the transformed filling
• You should not use dialogs directly. they should be wrapped into sophie's dialogs. Otherwise they are untestable.
• The BackgroundImageField has javadoc that is not nice and what is more important it doesn't function under linux. Also it has unnecessary commented out code.
• I don't like the "bounds" for dialogs, but this is out of the scope of this task.

[pap]

adding CC

[pav]

The review steps are followed, except some of them ;) I'm very happy to receive such a detailed review of my task and this helps a lot Sophie and me and thank you for that. I just want to note that words like "awful" or sentences like "You obviously copy/paste this" seem to me a little personal and I don't think that they help to make the task quicker or better, on the contrary they don't motivate me at all.. I think that we just have to focus on the positive even in our critics :)

[pap]

• This time I mainly like what you have done.
• If you are a bit more concentrated you can write much better code
• You should improve yourself on the unit tests. If you had written more detailed tests before you started impelmenting you would've had less errors.
• The test you submit about ImmGradient is not good enough especially about immutability.
• You didn't fulfil the requirement about the NONE filling. - fixed
• You didn't make radial gradients correctly. That took me additional 30 minutes to find and fix.
• Good luck in the next tasks you pick.

[deyan]

Batch update from file query-obsoleted.csv