Remove delay from BackgroundUpdater #10680
Conversation
As the BackgroundUpdater waits for the current _operation() to complete before executing another task anyway, the delay serves no purpose. This makes the UI feel more responsive by removing a noticeable delay between scrolling the revision grid and the subsequent update of the revision graph.
|
The BackgroundUpdater executes only one operation: |
My point has not been answered. I am not saying this part of #8809 should not be reconsidered. CC: @wischi-chr
|
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 14 days. It will be closed if no further activity occurs. |
|
Do the graph updates of release 4.1.0 work well enough for you, @MaxKoll? |
|
Hi, sorry for not getting back to you sooner.
The glitch that made it unusable for me is gone, which I'm happy about. I still notice the delay between grid updates (i.e. after scrolling, the grid is stuck for a tiny moment, then updates). Also, when scrolling many rows at a time, there are visual glitches while the grid is in motion, as this screenshot shows (v4.1.0.16698): These quirks are not obstructive, though. They just make the grid feel slightly sluggish. Before 02d5fdb it felt snappy and scrolling was instant (disregarding the bug that was fixed by this commit).
Compared to the previous updating method which used CancellationTokens (before 02d5fdb), the BackgroundUpdater method seems kind of naive: When the BackgroundUpdater is triggered, it will work on some specific rows of the grid, no matter if they are still in the view when the update finishes. The previous method seemed more convoluted, but also more fine-tuned, aborting grid updates as soon as possible when they became obsolete. I understand that using exception handling as a means of controlling the flow is generally frowned upon, which might have been one of the incentives to get rid of the previous updating method. But looking into "official" code examples regarding CancellationTokens and related topics, this seems to be the intended way of implementing it. |
|
@MaxKoll is this something you are still working on? |
|
No. And should #11046 get merged, the delay is probably not going to matter in practice (in the context of scrolling the grid). I'm fine with this being closed, if that's what you're asking. (I still believe there should be a better way than a forced delay between updates, but then again I'm unable to provide it.) |
|
@MaxKoll is you still have plans for this - feel free to re-open. |
I noticed this while investigating #10672.
As I understand, the BackgroundUpdater waits for the current _operation() to complete before executing another task. Also, execution happens asynchronously. Therefore I cannot see a reason for the delay before executing the next task.
This makes the UI feel more responsive by removing a noticeable delay between scrolling the revision grid and the subsequent update of the revision graph.
Proposed changes
Remove the delay from BackgroundUpdater.
Test methodology
I compared the scrolling behaviour against v3.5.4.12724 and the current master using this repo, and had an eye on visual appearance and performance.
Test environment(s)
Windows 10.0.19045.0 (134dpi, 140% scaling)
VS2022 Community
Git 2.38.1
Merge strategy
I agree that the maintainer squash merge this PR (if the commit message is clear).
✒️ I contribute this code under The Developer Certificate of Origin.