Redraw only the spinner when nothing else has changed

[AntoineGS]

PR Description

This fixes #4734, where lazygit would use a lot of CPU when the spinner was shown due to redrawing the entire screen every time the spinner needed a redraw.
The change adds the ability to request a UI redraw without invalidating the full UI, thus only redrawing the spinner, re-routing the spinner to this new flow.
CPU usage on a chromebook went from 60% to 5% when the spinner is shown.

Changing the refresh rate of the spinner from 50ms to 200ms brings it down from 5% to 1% if ever we would like to change the default, personally I prefer the slower moving spinner, but that's subjective.

Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[AntoineGS]

Went in a rabbit hole so it'll take some time to wrap up a working solution, my original approach introduced a bug where the full redraw event could be lost while the spinner was shown. I'll let you know when I am done!

[AntoineGS]

Alright so the problem was that some processes that update the layout do not themselves request a redraw and assumed it would be done through the ticker loop, adding a single full redraw after the ticker has completed (aka whatever job it was doing is done) fixed my issue.
Ready for review :)

[stefanhaller]

Great work!

This only fixes it for the bottom line status view; there's also the inline status that is used e.g. for fast-forwarding a branch, would be nice if this could be improved too. The code is in InlineStatusHelper.renderContext; it seems safe to call OnUIThreadContentOnly there, too. When I quickly tested it though, it didn't make as much of a difference as I would have expected, so maybe there's something else going on in that case. (Found it: it ends up calling Gui.render further down the call chain which undoes the optimization.) I'd be ok with solving this in a followup.

One quirk: if a user sets their gui.spinner.frames to a bunch of strings of unequal lengths, this used to push the rest of the status line over as necessary with every frame, and now it no longer does because we skip the layout. We could ignore this since it's unlikely that users do this, but we could also not let them; I pushed a commit (8a7e1fe) that rejects configs that try this.

[stefanhaller]

Also, were you planning on looking into the WithWaitingStatusSync issue? (either as part of this PR, or as a followup)

[AntoineGS]

I have now added it to sync events too, this one takes the views as parameters otherwise some of the UI would update before the task is actually done. For example if moving a commit down, the UI would show the commit moved down at the same time as the spinner starts to spin, but the highlight and control would only be given back to the user when truly done.
(This is how it was prior to this change so no UI change there)
The performance improvement will not be as interesting with this one since it was already targeting specific views.
I also realized after force pushing that I should have used fixup commits instead of doing the actual fixup to facilitate reviewing the changes, sorry!

I added a commit which should probably be another PR, let me know if that's the case and I will happily do so.
I noticed the *Sync call was not honoring the tick rate config so I fixed it.

[stefanhaller]

I have now added it to sync events too,

Ah, good, but that's actually not what I meant. I mistyped in my last comment, I meant WithInlineStatus, not WithWaitingStatusSync, sorry. The WithInlineStatus change needs to happen in InlineStatusHelper.renderContext as I said earlier.

Also, for this change the commit structure doesn't work. You can't make an API-breaking change in one commit and then adapt callers in a later commit. In this case I'd suggest to leave the commits as you had them before (59766d3 and e9a4650), and then add a new commit after that for changing ForceRedrawViews to ForceFlushViewsContentOnly, and adapting the caller in the same commit.

The performance improvement will not be as interesting with this one since it was already targeting specific views.

But it called Layout with every redraw, so it should still make a difference.

I added a commit which should probably be another PR, let me know if that's the case and I will happily do so. I noticed the *Sync call was not honoring the tick rate config so I fixed it.

That's fine, happy to include that here.

[AntoineGS]

Ah that makes more sense, might as well make the changes in this PR for the in-line spinner.
I will review the commit structure too so that each commit is build-able on its own.

[AntoineGS]

I took a conservative approach with the in-line spinner, I did not want to change the Context interface as it has a lot more implementations than just this one, and instead opted for type assertion while leaving a fallback to the original branch. While I believe the original branch is now dead code, it felt wrong to suddenly enforce a type to the generic Context interface.
Let me know what you think.

PS: Thank you for taking the time with the code reviews, it is very much appreciated :)

[stefanhaller]

I did not want to change the Context interface as it has a lot more implementations than just this one,

I don't think it's so bad to extend Context; it has only two implementations really, SimpleContext and ListContextTrait. I pushed dfc3593 that adds HandleContentOnlyRender to the Context interface and implements it in SimpleContext to simply call through to HandleRender (it's never called right now though). I really don't think that a type assertion is an appropriate tool in such a case.

I'm still not super happy with it, because there's an asymmetry: HandleRender takes care of calling Gui.render itself, but HandleContentOnlyRender doesn't, and relies on the caller to put it inside a OnUIThreadContentOnly call. We could have HandleContentOnlyRender do the OnUIThreadContentOnly inside I guess to make it more similar, but then it wouldn't be as flexible any more; a future caller might want to do a OnUIThreadContentOnly and then render the content of several views inside. Or do it the other way round and require callers of HandleRender to do the OnUiThread at the call site, but that would be a much bigger change, as there are so many callers. Thoughts welcome, but maybe we'll just live with the inconsistency.

Apart from that, I pushed two more trivial fixups.

Now the only remaining problem is that the commit "Support to request a content-only UI refresh" still doesn't compile.

[stefanhaller]

Wait, none of this HandleContentOnlyRender stuff makes any sense; I didn't think.

Currently the only difference between HandleRender and HandleContentOnlyRender is that the former calls Gui.render. We should totally move this out to the call sites (many of which don't actually need it even, as far as I can see), then we don't need two different methods on Context. Since this is a bigger change and requires careful examination of each call site, I suggest we do this in a followup and simply drop the inline status commit (and my fixups) from this branch.

[AntoineGS]

Done, I also ensured all commits can be built and tests pass for each.
I will open another PR to discuss the in-line changes.

[stefanhaller]

Looks good, thanks. There's one more little style thing that I pushed a fixup for, but I'm happy with this state now.

I will open another PR to discuss the in-line changes.

I'm not really sure this will save me time. Reviewing such a PR will probably take as much time as making the change myself, so I think I'd prefer to do that in this case.

[stefanhaller]

For some reason Github swallowed my inline comment for why I made that last fixup. I'm too lazy to type that again, let me know if you want to discuss any of that.

[AntoineGS]

I'm not really sure this will save me time. Reviewing such a PR will probably take as much time as making the change myself, so I think I'd prefer to do that in this case.

Sounds good!

For some reason Github swallowed my inline comment for why I made that last fixup. I'm too lazy to type that again, let me know if you want to discuss any of that.

I can see how making the argument explicit have benefits and is less ambiguous to callers, all good.

[stefanhaller]

@AntoineGS Here's the followup-PR for the inline spinner: #5592. Code review would be appreciated, but especially testing; I feel this is a bit of a risky change, there might be redraw glitches where a layout is missing now.