next: Enhance the variation read and display #412
Conversation
zsalch
commented
Oct 31, 2018
•
zsalch
commented
- Support more sgf variation format and enhance the variation display.
- New move number in the branch, the option is 'new-move-number-in-branch', default on.
|
Even though there has been no review I am tempted to merge this. Thoughts? @OlivierBlanvillain |
|
The diff is +347-76, I think it deserves a review ^^' |
| public Optional<int[]> lastMove; | ||
| public int[] moveNumberList; | ||
| public boolean blackToPlay; | ||
| public boolean dummy; |
There was a problem hiding this comment.
What does dummy mean here?
There was a problem hiding this comment.
There are some sgf file contains the AB/AW/AE attributes. For these attributes, it is better to create a dummy BoardData.
There was a problem hiding this comment.
Still, I don't understand. Why do we need dummy BoardData?
| @@ -6,9 +6,11 @@ | |||
|
|
|||
| public class BoardData { | |||
| public int moveNumber; | |||
| public int moveMNNumber; | |||
There was a problem hiding this comment.
Why do we need to store 2 move numbers? Can't we merge these two into one field?
There was a problem hiding this comment.
In current code, the moveNumber actual keeps the BoardData sequence number.
Many logic depend on the moveNumber.
The moveMNNumber for display move number, such as 'MN' attribute, move number in the branch.
There was a problem hiding this comment.
I'm not really happy with keeping all this state around, its quite hard to follow to be honest. Do you think we could we instead recompute moveNumber on the fly? I also think the naming doesn't help here. Maybe we should rename these things as follows:
- moveNumber → depth
- moveMNNumber → numberlabel
WDYT?
| @@ -72,6 +76,11 @@ public static BoardData empty(int size) { | |||
| */ | |||
| public void addProperty(String key, String value) { | |||
| SGFParser.addProperty(properties, key, value); | |||
| if ("N".equals(key) && comment.isEmpty()) { | |||
There was a problem hiding this comment.
I don't really like to have SGF parsing spreading all over the code base, it seams that this code doesn't belong to the BoardData class but should be moved to SGFParser.
There was a problem hiding this comment.
Yes, I think these code needs to be refactored and optimized in the next version.
| next = next.next().get(); | ||
| } | ||
| BoardHistoryNode end = isMain ? null : start; | ||
| while (next.previous().isPresent() && next != end) { |
There was a problem hiding this comment.
I think it would be clearer if you inline that condition as follows:
&& (isMain || next != start)
|
Sorry for not enough time to review the changes further, I have to hurry and release 0.6. I agree about no SGF parsing outside of the SGF parser classes. |
