TEXT_VIEW_MODEL

[diana]

TEXT_VIEW_MODEL

[meddle]

I think it passes as an analysis, for such a task, you should describe this processor, what will it do, in the implementation idea.

[meddle]

Super review, because of the implementor.

[meddle]

I see you added some new requirements, and short description what the processors do, so, it's ok for me.

[pap]

• No design related code. So the maximun is 3 points.
• In the TextProcessor interface you can choose better names for generic parameters. For example - O for Options and E for Effect.
• HotTextInterval is inconsistent as name with the interface name ImmText. Please rename it to fix that.
• Please use proper distribution of new lines in JavaDoc and between methods - TextChange
• "damaged" sounds strange. From the JavaDoc is not clear what 'damaged' really means. You should try to explain it properly and think of another term, 'damaged' sounds ill!
• We don't understand why do you need the DUMMY_ATTRIBUTE. Can't we check if we have a DOC_BREAK. Also its name is quite general and not production-like.
• You list methods you create and where you create the and that's ok. But you don't give any clue why do you need these methods (unite, concat, .....).
• Should the TextModel class be a ProObject and why? Now it has only two properties and lots of normal methods. Maybe some of these methods should be made properties.
• It is a bit strange that you have such a deep hierarchy of TextModels.
  • Last HeadTextModel would better be a top level class as it is quite huge. It can be constructed by a SceneTextView or HeadTextSceneView.
  • Also it is better to have properties in the TextModel class as this would ensure trackablility to the users of the library.
  • TextModel and BaseTextModel is not a nice pair of names for an abstract class and its abstract implementation. Either make TextModel an interface and {{{BaseTextModel}} an abstract class or merge these two.
  • The processedText property of BaseTextModel is frightening. It is so huge. Simplify it by extracting some logig in methods inner to the AutoProperty class.
  • TextResourceModel would better be named ResourceTextModel. Its existence would make sense if the logic of its changeModel method gets more complicated.
• Shouldn't the SET_TEXT event be renamed to CHANGE_TEXT instead of SET_CHANGE ?
• Please make yourself the author of classes that you change completely. Don't blame "vlado" for them :)
• Whether the carret is visible and whether the text is editable is not the same thing. Also if the carret should not be visible you may just ommit putting the CARET_ATTR in the processed text. Why should the layout bother with such logic????
• In TextLinkProcessorTest line 82 use assertEquals instead of assert. After all you're writing a test :)
• The same is valid for some asserts several lines below that.

[diana]

• The DUMMY_ATTRIBUTE is needed to guarantee the separation of the last unit of the text (the DOC_BREAK unit) as a single text run from the other text runs. This makes it easy to get the outline shapes of the runs without bothering if the last unit should be in the shape or not.

[meddle]

Merged to the text-trunk at [9101] and [9110].

• TextFrameLogic - ON_MOUSE_MOVED. No need for "if (hitUnit != null) {" check.
• Same place. Fires too much MOUSE_LEAVE events.
• ON_MOUSE_EXITED_FRAME too fires too much.
• text.unitAt(pos) returning null. This is strange, and is not mentioned in the JavaDoc.
• HotAttr -> bad equals without braces. Generate this method through Eclipse's Source menu and don't forget to check the "Use blocks in if statements" checkbox
• Same for DoublePoint, TextRun, TextUnit, TextLinkProcessor
• TextView - interactions for copy/cut/paste MacOS compatible
• ImmTreeCollection hashCode - braces
• You didn't took seriously our design code related notes... May be the reason we write them at the design review is to take them in mind? Or may be we waste our time, because we have a lot of free time...
• When writing TODOs that will pass between tasks be a bit more descriptive. Not like in TextStyleChange - "TODO: Fix". Also there is an idea about how TODO/FIXME/XXX should be written with --id@date in the end.
• Be sure to fix the TODOs before the merge with the trunk.
• In HotSegmentLayout.draw there are two carretCounts. Why is this needed? Can't there be only one counter and an assertion that it is <=1?
• What is the idea of having HotStylDef.EMPTY and HotStyleDef.DEFAULT?
• There is lots of commented out code. When integrating in the real trunk it will be deleted. Please, have this in mind.
• In CommentEntryView the getTextModel method returns null. Isn't that strange?
• In the selection options, I didn't understand withCaret before I read the JavaDoc, bad naming, renamed it to caretVisible.
• What about asserting that the text classes are equal in the TextChange methods.

[meddle]

Closing all the tickets before M Y1