feat: make analysis tree view more "tree like"

[tom-anders]

Especially for heavily commented PGNs this is now much more readable. We're also more consistent with the website now. I added screenhots for comparison below.

I know this is a rather big refactoring, and the recursive nature of this stuff is quite complex, so let me know if anything is unclear.

I also made a few style adaptions to be a bit more consistent with the the Web UI, but I don't have any strong opinions here, feel free to tweak the styling again after merging.

(Left: Before, Right: This PR)

lichess.org for comparison:

[tom-anders]

(The main trick here that improves the wrapping of comments etc. is to use Text.rich to build the lines, instead of the Wrap widget)

[ijm8710]

This is very cool and agree it looks much better Tom! Thank you! Would you be interested in looking into the two column vertical notation at some point as well? I know it's a different topic but while you're testing notation edge cases, you'll prob be the best man for the job 😀 #866

[veloce]

Well thanks for this! I know it is tricky to deal with.

Quick review for now: I like it a lot, especially the way you handle sidelines and nesting. It it much more clear now.

Making it look like the web UI, is a non goal to me, as I remind often. The app should have is own visual identity. Of course when the design is better in web we should copy it, as you did here. But we can also vary on some point where it makes sense.

One thing I'm not sure of, for instance, are the italics. Looks like the rule in lichess website is to have an inside sideline italic. Why not... But I think I actually prefer having the comments in italic and keep the inline sideline as is (with a smaller font and a slight opacity).

I'll review this carefully after I merge the challenges branch (which should be soon now).

[tom-anders]

Well thanks for this! I know it is tricky to deal with.

Yeah I stopped counting the knots this put into my brain :D

Making it look like the web UI, is a non goal to me, as I remind often. The app should have is own visual identity. Of course when the design is better in web we should copy it, as you did here. But we can also vary on some point where it makes sense. One thing I'm not sure of, for instance, are the italics. Looks like the rule in lichess website is to have an inside sideline italic. Why not... But I think I actually prefer having the comments in italic and keep the inline sideline as is (with a smaller font and a slight opacity).

Yeah I get a that, I can change it. Using the Web UI as a reference, also for font styles etc. was just more convenient while developing/testing, but yeah no strong opinions on that, I'll adjust the style (or maybe it's even easier if you do it after merging the PR)

[tom-anders]

This is very cool and agree it looks much better Tom! Thank you! Would you be interested in looking into the two column vertical notation at some point as well? I know it's a different topic but while you're testing notation edge cases, you'll prob be the best man for the job 😀 #866

Sure why not, but I'll work on the study feature for now, so don't expect anything to happen in the near future

[veloce]

This screen also supports PGN import from random sources. The tree view should look good also in that case.

Could you post a screenshot of the analysis tree when importing this PGN @tom-anders ? https://github.com/lichess-org/dartchess/blob/65fad86b26618785287152e437a7c80efb94b058/data/wcc_2023.pgn#L1-L39

Thanks!

[tom-anders]

This screen also supports PGN import from random sources. The tree view should look good also in that case.

Could you post a screenshot of the analysis tree when importing this PGN @tom-anders ? https://github.com/lichess-org/dartchess/blob/65fad86b26618785287152e437a7c80efb94b058/data/wcc_2023.pgn#L1-L39

Thanks!

Huh, fun fact (or not fun), I tried importing them into a study via the web interface for comparison, but parsing failed:

[tom-anders]

Thanks for that PGN, it uncovered a small bug when building sidelines, it's fixed now. I attached a video where I'm scrolling through the first game of the PGN.

Note that there are some weird looking line breaks in some comments, but that's not a wrapping issue, those line breaks are actually there in the source PGN you posted. I'd assume that usually a PGN comment will only have intentional line breaks...?

treeview.webm

[veloce]

Thanks for the vid! Not sure why there are line breaks in that PGN, let's ignore them.

I find it way more readable in your version, great job!

There's one difference with what I did, the comments are shown in full length, where before only the first 5 lines where shown, and you'd tap on a comment to see the full version in a bottom sheet. I don't know if that change was intentional, but I actually like it, so let's keep it that way!

[veloce]

One question, have you put a limit on nesting?

I suppose we should have one. Or at least prevent further indentation when reaching a nesting threshold.

[tom-anders]

There's one difference with what I did, the comments are shown in full length, where before only the first 5 lines where shown, and you'd tap on a comment to see the full version in a bottom sheet. I don't know if that change was intentional, but I actually like it, so let's keep it that way!

Yeah that was intentional, especially for heavily commented PGNs and studies I thought that it would break your flow if you had to keep tapping comments to read them.

One question, have you put a limit on nesting?

I suppose we should have one. Or at least prevent further indentation when reaching a nesting threshold.

Ah yes good point, haven't implemented that yet - what would be a good threshold after which we stop increasing indentation? Something like 4 maybe?

Also, we could by default hide deeply nested variations and display a + Icon instead like the website does, what do you think about that?

[veloce]

Ah yes good point, haven't implemented that yet - what would be a good threshold after which we stop increasing indentation? Something like 4 maybe?

No more than 4 seems right indeed

Also, we could by default hide deeply nested variations and display a + Icon instead like the website does, what do you think about that?

👍

[tom-anders]

Also, we could by default hide deeply nested variations and display a + Icon instead like the website does, what do you think about that?

👍

Ok I checked the website, it's actually pretty aggressive when it comes to hiding variations: Only two levels of nesting are displayed by default, anything that's three or more levels is hidden by default.

I guess unless we have a good reason not to, let's do the same for now..?

[tom-anders]

Okay, added an indentation limit, collapse nesting>2 by default and added the plus-icon to expand variations. IMO this makes the "expand variations" action in the move context menu redundant, so I removed it.

"Hide variations" works a little bit differently now, it used to only hide the variation you selected the move from, but now it hides all other sibling variations as well. I think it's more useful and intuitive this way, but curious to know what others think

treeview.webm

[veloce]

Great work! Yes, I think it should hide all variations.

I wonder if the "plus" button is easy to tap? Seems a bit small.

Also can you post again the screen recording with the WCC PGN with this new version? thanks!

[tom-anders]

Yeah you're right, I made the icon bigger now. New video:

treeview.webm

[veloce]

Looks really great!

[tom-anders]

Thanks for the review!

There is a lot of changes and this is complex code. I know this part is not much tested yet, but I think it would be nice to add some tests to analysis_screen_test relating to what you changed.

Tests don't have to be extra complex first. Using simple PGN with variations, add some tests that check a few basic things:

* when the variation is indented

* when it is inline

* when it is collapsed

* ... etc

Sure, I'll add some more tests :+1:

One last thing: this is a performance sensitive block. Have you tested the performance using a profile build on a real device? Using large trees to compare with before?

Will do some before/after tests today, the WCC PGN is probably a good test here again

[tom-anders]

I plan to reuse the new _PgnTreeView for the study feature, so maybe it's better to make it public, move it to a new file and test it in isolation? what do you think?

[tom-anders]

Okay, so I did some profiling, by loading the WCC PGN, selecting the first move (e4) and then pressing the "Next" Button, which should rebuild the entire tree view. In general, for both the main branch and this PR I observed slight lag, but note that my device has a pretty slow CPU, so I'm seeing lag in a lot of other apps as well.

Note that I did not enable stockfish for these tests (as that would lead to pretty much constant lag anyway)

On the main branch, the frame that corresponds to drawing the tree widget takes about 50ms on the UI thread.

With this PR, it instead takes 100-120ms, followed by the frame that draws the indent lines, taking 50-60ms

So yeah, it's a significant performance hit for very large trees like this.

However, I also tested with https://github.com/lichess-org/dartchess/blob/65fad86b26618785287152e437a7c80efb94b058/data/lichess-bullet-game.pgn, which has mostly inline sidelines (but also two indented sidelines due to their length).

With that PGN, main takes 15-16ms to render the tree view, while this PR takes 20-22ms, with the next frame taking 6ms to render the indent guides.

[tom-anders]

Performance update: I've implemented an optimization that only rebuilds the subtree of the mainline that actually changed. With this optimization in place rendering the tree of the WCC PGN now only takes 15ms when switching a move within the same mainline subtree, and 30ms when switching the current move from one mainline subtree to another one (since then we need to redraw both the old and the new subtree in that case). Rendering the indents becomes pretty much negligible now. :tada:

Pretty impressive that with this optimization performance is even better than with the old code

[veloce]

I plan to reuse the new _PgnTreeView for the study feature, so maybe it's better to make it public, move it to a new file and test it in isolation? what do you think?

Ideally we'd include the debouncing and auto-scroll logic, but the debouncing relies on analysis controller. We'd need to refactor to have a common interface for analysis and study (and broadcast). I think it's a lot more work to do this refactoring. Also I see that _PgnTreeView hasAnalysisController in its parameter, so it shouldn't be compatible with study yet?

Perhaps the easiest for now is to add the tests in the analysis_screen_test (so depending on the analysis controller), and we'll move the tests later when the common interface is done.

[veloce]

Performance update: I've implemented an optimization that only rebuilds the subtree of the mainline that actually changed. With this optimization in place rendering the tree of the WCC PGN now only takes 15ms when switching a move within the same mainline subtree, and 30ms when switching the current move from one mainline subtree to another one (since then we need to redraw both the old and the new subtree in that case). Rendering the indents becomes pretty much negligible now. 🎉

Pretty impressive that with this optimization performance is even better than with the old code

Amazing! Great idea, I hadn't thought about it.

[tom-anders]

I plan to reuse the new _PgnTreeView for the study feature, so maybe it's better to make it public, move it to a new file and test it in isolation? what do you think?

Ideally we'd include the debouncing and auto-scroll logic, but the debouncing relies on analysis controller. We'd need to refactor to have a common interface for analysis and study (and broadcast). I think it's a lot more work to do this refactoring. Also I see that _PgnTreeView hasAnalysisController in its parameter, so it shouldn't be compatible with study yet?

Oh I forgot that I've already written an interface to make the _PgnTreeView independent from AnalysisController on my study branch, but not on this branch. For _PgnTreeView it's easy, because it only calls notifier methods, but does not have any ref.watch in it. But also factoring out the debouncing logic is trickier.

So yeah, let's put it into the analysis_screen_test.dart for now and refactor in a follow-up PR.

[tom-anders]

Sorry for the general lack of comments, with complex pieces of code like this that had many iterations until it ended up in its final state, I often find it hard to determine which parts need comments and which don't, especially for non-public classes (since at that point in your head everything seems obvious).

Added some comments and also renamed stuff, let me know if anything is unclear still.