When server list received, only start pings if timer not running

[softins]

Short description of changes

In CConnectDlg::SetServerList(), test whether TimerPing is already running before starting it and calling OnTimerPing()

This avoids a double ping on startup due to receiving the reduced server list and the normal server list in quick succession. It also appears to fix the longstanding behaviour of the first ping values displayed being wildly inaccurate.

CHANGELOG: Client: remove double ping from Connect Dialog, improving accuracy of initial ping times displayed.

Context: Fixes an issue?

I noticed while working on the TCP support, that when opening the connect dialog or changing directory, it would initially ping every server twice in succession. This turned out to be because of receiving the reduced and full server lists from the directory. They would both result in a call to OnTimerPing() and starting of TImerPing. This PR checks whether TimerPing is already running before doing so, thereby avoiding the double ping.

Does this change need documentation? What needs to be documented and how?

No, bugfix only.

Status of this Pull Request

Ready. Would also be worth backporting to 3.12.4

What is missing until this pull request can be merged?

Review

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want
• My code follows the style guide
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[mcfnord]

MY Opus 5 LLM WROTE:

Traced this through the call sites and the logic holds up. showEvent() and OnDirectoryChanged() both funnel into RequestServerList(), and CClient::CLServerListReceived drives SetServerList() twice — once with bIsReducedServerList set, once without — so the unconditional OnTimerPing() + TimerPing.start() at the end of SetServerList() did indeed fire two ping bursts a few ms apart. Since the ping display measures round-trip time without matching responses to specific requests, the interference between the two in-flight bursts is a plausible mechanism for the inaccurate initial values you describe.

The TimerPing.stop() in RequestServerList() looks necessary rather than incidental: without it, a timer still active from a previous dialog session would make the new isActive() guard suppress the first legitimate ping burst after a reopen or directory change.

The one thing worth checking rather than assuming is whether the isActive() guard can leave a server waiting a full PING_UPDATE_TIME_SERVER_LIST_MS (2.5 s) for its first ping — that would need the full list to carry servers the reduced list didn't, so the guard suppresses their immediate ping and they wait for the next timer tick. I measured it instead of reasoning about it: built this branch and mainline, ran a local directory plus 5 servers on loopback with the client under Xvfb, and captured the real CLM_PING_MS packets on the wire, 5 repetitions per build.

That subset turns out to be empty. CServerListManager builds one vecServerInfo and sends it through both CreateCLRedServerListMes() and CreateCLServerListMes(), so the reduced message omits fields, not servers — and the capture agrees: both bursts hit the identical set of server ports, on both builds. Measuring the delay directly anyway, full-list arrival to first ping is 0.014–0.084 s on every server in every repetition, on both builds. No 2.5 s stragglers.

The same captures show the fix doing what it claims: on mainline the first burst carries 22 ping packets, on this branch 11. No blocker from me.