TEXT_MODEL_REDESIGN

[kyli]

TEXT_MODEL_REDESIGN

[kyli]

This analysis is merge of #2370 (HOT_TEXT_INTERNAL) and #2344 (TEXT_LAYOUT_COMMONS). Since it is a new ticket, I guess it needs a new an-re.

[pap]

• Ok, although some parts sound a bit vague and at the same time quite harsh.

[kyli]

Thanks.

[pap]

TEXT MODEL PART

• Please rename the "smart" package to something smarter.
• TextUnit related
  • There is no need of the word "unit" in the methods.
  • In the description in the wiki page we don't understand the meaning of "only reference to a text style".
  • Please put some empty lines between methods. This increases readability.
  • Malformed JavaDoc - an empty line between description and @param/@return/@author should be left.
• ImmText interface
  • Write a better JavaDoc of the interface itself. Usually interfaces have lots of it so that implementors can understand how to implement them. Don't mention the name of an implementation of the interface in its JavaDoc.
  • Replace the getBegin and getEnd methods with getSize. ImmText represents a list so let it behave as such. This similarity should be pointed in the JavaDoc.
  • Malformed JavaDoc.
  • There is no need of the getCharAt method in the interface. This is considered a redundancy.
  • It is better to rename getAt to unitAt.
  • The replace method needs a better explanation of the styles behavior. The current one is not sufficient to understand the desired behaviour.
  • We think that the getStyleValue method should not return the default value of the attribute but null instead. It should be left to the client code of the text to decide how to handle such a case.
• Please explain why should HotStyleDef be made Hashable.
• We don't understand why do you need separate LINK_ATTRIBUTE and ATTACHMENT_ATTRIBUTE. This should be explained in the design.
• Probably the idea of handling attachments as styles should be pointed out.
• The Attachment interface needs better JavaDoc (more explanatory).
• The static function getAllAttributes should be moved to CommonAttribute, because it returns all the common attributes. You should use reflection in the implementation of this method, because if you add new attribute it will return it as well. Cache the values returned by this function once they are retrieved.
• It would be good to mention the arguments and return type of new methods you introduce in the text of the design.
• ImmTextUtils
  • empty should better be named createEmptyText
  • getNextAttrInterval is not described correctly in the wiki page and has a bit strange name.
  • checkBounds is a bad name. isIndexInText is better.

TEXT LAYOUT PART

• Why does the ImmTextUtils.getRun method return ImmText and not a TextRun ?
• Why is it in this util class.
• Why is TextRun.layoutGlyphVector named this way?
• We think that the design is good, but is too abstract, you should mention some classes and methods, algorithms.
• It is nice if you use the following format for comments "TODO: text_of_the_comment --kyli@2010-03-25"

Tests

• Look at ImmTextTest, you should be able to see that it is an example of BAD test:
  • First we have some rules for writing tests in the GoodCodeBLA page.
  • Second there are some redundant test methods and meaningless ones...
  • You should rewrite it, or just delete it, you decide.
• You should improve the old tests, not only fix them. You can delete meaningless one too, because it is good to have only usefull tests.

3p

[pap]

• As text tasks tend to break lots of things first and then fix them in other tasks please in the implementation mention what to expect in order to ease the implementation review mainly regarding user experience. (i.e. chaining, persistence etc. :) )

[diana]

TEXT MODEL PART

"Replace the getBegin and getEnd methods with getSize. ImmText represents a list so let it behave as such. This similarity should be pointed in the JavaDoc." - NO!!!!!This is completely wrong and I won't replace it :)))Now the text behaves as a list but can you guarantee that after our second and third task we won't refactor it again?!!!And even if not so...is it better to have the magic number 0 in the code than simply have getBegin?!What if we decide to have -1 element of the text(in order to help the layout draw the caret after the character????):)
"We think that the getStyleValue method should not return the default value of the attribute but null instead. It should be left to the client code of the text to decide how to handle such a case." - I do not understand this...it is logically wrong for me...the value of a given attribute is NEVER null according to our design(the HotStyleDef of a given char always has all the attributes even if some of them are with default values)...that's why it's strange to return null:)please give a better explanation why to do such a thing.
"The static function getAllAttributes should be moved to CommonAttribute, because it returns all the common attributes. You should use reflection in the implementation of this method, because if you add new attribute it will return it as well." - I'm not sure that all the attributes will be contained. In the next task I may separate the class CommonAttr because there will be attributes that the model of the text cannot hold. So I won't refactor it now but in the next task.
"The Attachment interface needs better JavaDoc (more explanatory)." - It's not in my task...this is in the trunk since some of the link tasks and it's not mine but deni's...:)
And about the tests...I will remove the "meaningless" tests but those tests REALLY helped me for the redesigning. And I will need them while the project is over. So you make me copy all the "meaningless" tests for every one of my tasks...(cool)

[pap]

• Fake JavaDoc on methods in DoublePoint.
• LayoutUtils.getParaOffses had bad JavaDoc for the return statement.
• The JavaDoc of the return statement should be on a new line - HotAreaLayout.getHitIndex.
• someText.getEnd() == 0 doesn't seem to be always a correct measure of the emptiness of someText. Please find a solution to such a problem and make sure it is well documented in the JavaDoc of the ImmText interface.
• It is good to use CommonChar.TAB in comparisons instead of '\t'.
• run.getAttrValue(CommonAttr.STRIKE_THROUGH).equals(Boolean.TRUE) - this is wierd as a condition in an if statement.
• When logging have the following in mind: TRACE is used to look at the progress of methods, DEBUG is used to look at the progress between methods.
• I suppose that the HotSegmentLayout.getHitIndex can be made a bit faster because the distance should first be decreasing and then increasing.
• It is nice if you use the following format for comments "TODO: text_of_the_comment --kyli@2010-03-25"
• Put new lines between methods
• Put a space between the "if" and the "(".
• Like the improved JavaDoc of ImmText methods :).
• And as was expected not all of the design remarks were taken in mind. But we are happy with the positive changes.

[meddle]

Closing all the tickets before M Y1