TEXT_FONTS_INTERFACE_R0

[Astea]

wiki page: TEXT_FONTS_INTERFACE_R0 - effort: 1d

[boyan]

• The module name might not be appropriate. Probably a better suggestion is org.sophie2.main.func.text.gui - see the GROUP_PLUGIN_STRUCTURE_R0 task and discuss with pav the most appropriate name.
• It is a good idea to link existing classes that are mentioned for easier reviewing.

Other than that the design seems ok. (30m)

[meddle]

The implementation works, but there are terrible problems with the memory. When you fix that the task will pass.

2.5p (20m)

[meddle]

The implementation will fail because of the following reasons:

• There are still problems when you change the styles rapidly, the frames freeze.
• There was files without JavaDoc at all.
• There were files for the chaining task with the files for this task, see changeset : 2958.
• The code will be re-reviewed detailed when the above things are OK.

I advice you to create separate clean branches for all your text tasks.

2p (40m)

[nenko]

• new branch is added
• the problems when you change styles rapidly are not solved
• classes wuthout JavaDoc are removed
• the changeset should cotaint only thigs related to this task

[meddle]

The first exception that is thrown when you rapidly change styles.

[meddle]

The second exception that is thrown when you rapidly change styles.

[meddle]

The branch is in good state now. We took a deeper look at your implementation and saw a few more problems:

• Problems with the code :
  • You have very strange conditions, for example : "if (preStyle.equals(HotStyleDef.EMPTY_STYLE) == false)"... Why didn't you write "if (!preStyle.equals(HotStyleDef.EMPTY_STYLE))"?
  • Fix the name of your test, now it's called "TextStlyerTest".
  • Use assertEquals instead of "assertTrue(something.equals(otherthing));".
  • Use assertEquals instead of "assertTrue(somePrimitivevalue == otherPrimitiveValue);".
  • Test NaiveStyler#replaceStyle even if you don't use it now.
  • The sort key of TextFontHaloButton's VisualElementDef ("text-font-halo-button") is not a good sort key, usualy these sort keys start with three lower letters_dash_something. Example "aaa-text-font-halo-button".
  • The same as above applyes for TextFontHaloMenu, TextFontFamily, TextFontSize, TextItalicStyle, TextBoldStyle, TextUnderlineStyle, TextStrikethroughStyle and TextFontHud.
• Problems with the behavior:
  • The controls in the hud should be initialized depending on the underlying model text and its style. This includes the selected font/font_size in the combo boxes.
  • The later should change when the hud is attached to a new frame.
• Problems with exceptions:
  • The exceptions when you rapidly change styles are attached to the ticked this time.
    • firstStyleException.txt
    • secondStyleException.txt

1p (80m)

[nenko]

well, another try...

[meddle]

• Problems With the behavior and exceptions:
  • When trying to change the font size, exception is thrown; See the attachment "fontSizeException.txt". I wrote "anime" and tryed to change it's size. (That is if the cursor is not visible...)
  • The same as above applies to the use-cases when you try to change bold, italic, etc.. all of the styles, so practically the styles don't work.
  • When there is no text in the Frame you can change the styles, but the font size returns to "12" every time when you click in some of the checkboxes for the other styles or change the font name. The same applyes and for the font name itself.
  • The row of the hud that contains the "font name: " and the BoundComboBox is to long and its ends are out of the hud. (That's only under windows...)
  • Now when you navigate through the text with the keyboard arrows, an exception is thrown. See the attachment "navigationException.txt".
• Code notes:
  • You fixed the strange condition checks in you test, and the test itself is super. In the code you left them like in the NaiveHotText. But that thing is not mandatory...

This implementation like code is good, but has many bug, the bug with the memory is not present anymore, and there is some strange one with the layout (the one from the previous review) but it is triggered hard and rarely, so if you fix the new bugs from the above section the implementation will pass.

2p (70м)

[meddle]

• There is the assertation error when changing to some of the fonts and they are with size over 13.
• You left a system out :)

3p (2h)

[deyan]

Batch update from file query-obsoleted.csv