ConnectDialog: use qpAddress for address list in ServerItem, and for qhDNSCache. #3018
Conversation
src/mumble/ConnectDialog.cpp
Outdated
| foreach(const QHostAddress &qha, qlAddresses) | ||
| qsl << Qt::escape(qha.toString()); | ||
| foreach(const qpAddress &addr, qlAddressPorts) | ||
| qsl << Qt::escape(addr.first.toString()); |
There was a problem hiding this comment.
We should include the port here.
src/mumble/ConnectDialog.cpp
Outdated
| foreach(const QHostAddress &qha, info.addresses()) { | ||
| qpAddress addr(HostAddress(qha), si->usPort); | ||
| qpAddress addr(qha, si->usPort); |
There was a problem hiding this comment.
This line should not have changed.
…qhDNSCache. This is prepration for the implementation of the ServerResolver class. (For SRV record resolving.) We now store address/port pairs for the DNS cache and list of resolved addresses in ConnectDialog. This is because SRV records resolve to address/port pairs.
There was a problem hiding this comment.
Man, that's a lot of interwoven logic. Hard to follow/see. I don't feel like I was able to verify everything surrounding this. Especially the lookups, dns cache and ip cache interaction. But the logic already works, so this seems fine/the changes make sense.
Apart from the variable name, looks good/plausible to me.
| @@ -145,7 +145,7 @@ class ServerItem : public QTreeWidgetItem, public PingStats { | |||
| QString qsBonjourHost; | |||
| BonjourRecord brRecord; | |||
|
|
|||
| QList<QHostAddress> qlAddresses; | |||
| QList<qpAddress> qlAddressPorts; | |||
There was a problem hiding this comment.
I don't really like the variable name. It's not address ports, it's address including port. Keeping the name address qlAddresses seems better to me. That also matches the type name, but pluralized, indicating naming consistency. (A port is part of the address.)
| @@ -145,7 +145,7 @@ class ServerItem : public QTreeWidgetItem, public PingStats { | |||
| QString qsBonjourHost; | |||
| BonjourRecord brRecord; | |||
|
|
|||
| QList<QHostAddress> qlAddresses; | |||
| QList<qpAddress> qlAddressPorts; | |||
There was a problem hiding this comment.
This class ServerItem has qsHostname and usPort. So what is qlAddressPorts even? Should have a describing comment.
Contains (DNS-)resolved addresses derived from qsHostname and usPort (IPv4 and IPv6).
| si->qlAddresses.append(qha); | ||
| else | ||
| si->qlAddresses = qhDNSCache.value(host); | ||
| if (! qha.isNull()) { |
There was a problem hiding this comment.
I would like to see a comment here. We check if it's an IP (rather than a hostname/domain) and use that if it is.
Or should I have known QHostAddress is for IP addresses (only)?
| if (! qha.isNull()) { | ||
| si->qlAddressPorts.append(qpAddress(HostAddress(qha), si->usPort)); | ||
| } else { | ||
| si->qlAddressPorts = qhDNSCache.value(host); |
There was a problem hiding this comment.
So we know we have something in the cache?
This is prepration for the implementation of the ServerResolver class.
(For SRV record resolving.)