Ticket #1674 (closed unplanned_task: obsolete)

Opened 10 years ago

Last modified 9 years ago

GROUP_FILLING_R0

Reported by: pav Owned by: pav
Priority: major Milestone: M07_ALPHA2
Component: uncategorized Version: 2.0
Keywords: Cc: pav
Category: MAIN Effort:
Importance: Ticket_group:
Estimated Number of Hours: Add Hours to Ticket:
Billable?: Total Hours:
Analysis_owners: pav Design_owners: pav
Imp._owners: pav, pav Test_owners:
Analysis_reviewers: kyli Changelog:
Design_reviewers: pap, meddle Imp._reviewers: pap,pap
Test_reviewers: Analysis_score: 3.5
Design_score: 3 Imp._score: 3.5
Test_score: 0

Change History

comment:1 Changed 10 years ago by pav

  • Status changed from new to s1a_analysis_started

comment:2 Changed 10 years ago by pav

  • Status changed from s1a_analysis_started to s1b_analysis_finished

2h

comment:3 Changed 10 years ago by kyli

  • Status changed from s1b_analysis_finished to s1c_analysis_ok
  • Analysis_reviewers set to kyli
  • Analysis_score changed from 0 to 3.5

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.

comment:4 Changed 10 years ago by pav

  • Design_owners set to pav
  • Status changed from s1c_analysis_ok to s2a_design_started

comment:5 Changed 10 years ago by pav

  • Status changed from s2a_design_started to s2b_design_finished

comment:6 Changed 10 years ago by pap

  • Description modified (diff)

comment:7 Changed 10 years ago by pap

  • Design_score changed from 0 to 2
  • Design_reviewers set to 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.


comment:8 Changed 10 years ago by pap

  • Status changed from s2b_design_finished to s1c_analysis_ok

Forgot to change the status in teh previous change

comment:9 Changed 10 years ago by pap

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

comment:10 Changed 10 years ago by pav

  • Status changed from s1c_analysis_ok to s2a_design_started

comment:11 Changed 10 years ago by pav

  • Status changed from s2a_design_started to s2b_design_finished

1.5d

comment:12 Changed 10 years ago by meddle

  • Status changed from s2b_design_finished to s2c_design_ok
  • Design_score changed from 2 to 3
  • Design_reviewers changed from pap to pap, 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)

comment:13 Changed 10 years ago by pav

  • Status changed from s2c_design_ok to s3a_implementation_started

comment:14 Changed 10 years ago by pav

  • Status changed from s3a_implementation_started to s3b_implementation_finished
  • Imp._owners set to pav

comment:15 Changed 10 years ago by pap

  • Status changed from s3b_implementation_finished to s2c_design_ok
  • Imp._score changed from 0 to 2
  • Imp._reviewers set to 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.

comment:16 Changed 10 years ago by pap

  • Cc pav added

adding CC

comment:17 Changed 10 years ago by pav

  • Status changed from s2c_design_ok to s3a_implementation_started

comment:18 Changed 10 years ago by pav

  • Status changed from s3a_implementation_started to s3b_implementation_finished

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 :)

comment:19 Changed 10 years ago by pap

  • Status changed from s3b_implementation_finished to s3c_implementation_ok
  • Imp._score changed from 2 to 3.5
  • Imp._reviewers changed from pap to pap,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.

comment:20 Changed 10 years ago by pav

  • Imp._owners changed from pav to pav, pav

comment:21 Changed 9 years ago by deyan

  • Status changed from s3c_implementation_ok to closed
  • Resolution set to obsolete

Batch update from file query-obsoleted.csv

Note: See TracTickets for help on using tickets.