Differential D14527

Show hostname in krfb connection info
ClosedPublic
Actions

Authored by alexeymin on Jul 31 2018, 11:18 PM.

Details

Reviewers
ngraham
aacid
Group Reviewers
KDE Applications
Commits
R437:718b0ac000ea: Show hostname in krfb connection info
Summary

Using krfb as desktop support tool for ~250 desktops on LAN - 95% of the users aren't equipped to deal with reciting IP addresses accurately, so I wanted to add the hostname into the connection details as well, since it's easier for non-tech people to read that back.

Attached patch does this, and works internally for us - thoughts on doing it better:

  • hostnames as reported by the host are not necessarily accurate for network access, so
  • consider actually doing a nameserver lookup to verify
  • only show the hostname if you can verify

Diff Detail

Repository
R437 Desktop Sharing
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bgray requested review of this revision.Jul 31 2018, 11:18 PM
bgray created this revision.
alexeymin added a subscriber: alexeymin.May 18 2019, 9:35 AM

Maybe we should land this? 😉

alexeymin added a comment.May 18 2019, 10:20 AM

This patch does not apply on master now, needs some changes like that:

diff --git a/krfb/mainwindow.cpp b/krfb/mainwindow.cpp
index 8de7822..ad2b04c 100644
--- a/krfb/mainwindow.cpp
+++ b/krfb/mainwindow.cpp
@@ -34,6 +34,7 @@
 #include <QVector>
 #include <QSet>
 #include <QNetworkInterface>
+#include <QHostInfo>
 
 
 class TCP: public QWidget, public Ui::TCP
@@ -137,14 +138,17 @@ MainWindow::MainWindow(QWidget *parent)
     int port = KrfbConfig::port();
     const QList<QNetworkInterface> interfaceList = QNetworkInterface::allInterfaces();
     for (const QNetworkInterface& interface : interfaceList) {
-        if(interface.flags() & QNetworkInterface::IsLoopBack)
+        if (interface.flags() & QNetworkInterface::IsLoopBack) {
             continue;
+        }
 
-        if(interface.flags() & QNetworkInterface::IsRunning &&
-                !interface.addressEntries().isEmpty())
-            m_ui.addressDisplayLabel->setText(QStringLiteral("%1 : %2")
+        if (interface.flags() & QNetworkInterface::IsRunning &&
+                !interface.addressEntries().isEmpty()) {
+            m_ui.addressDisplayLabel->setText(QStringLiteral("%1 (%2) : %3")
+                    .arg(QHostInfo::localHostName())
                     .arg(interface.addressEntries().first().ip().toString())
                     .arg(port));
+        }
     }
 
     //Figure out the password

After that, it works and looks OK, which I think is already an improvement:

pino added a subscriber: pino.May 18 2019, 11:00 AM
In D14527#466635, @alexeymin wrote:

This patch does not apply on master now, needs some changes like that:
[...]

Too many unrelated changes (coding style) -- the only change needed is s/QString/QStringLiteral/ for the m_ui.addressDisplayLabel text.

alexeymin added a comment.May 18 2019, 11:39 AM

I could take over this revision and update the diff as required (even without adding braces around if statements), but I guess we will need to know @bgray 's email to be able to preserve authorship.

ngraham added a comment.May 20 2019, 7:50 PM

The diff is so trivial, I say just submit a new one yourself with your own authorship information, if that's the sticking point.

aacid added a subscriber: aacid.Jun 9 2019, 10:09 PM
In D14527#467671, @ngraham wrote:

The diff is so trivial, I say just submit a new one yourself with your own authorship information, if that's the sticking point.

Let's be nice to people and give attribution where it's due :)

About the patch, note that localHostName *can* return empty according to the documentation, so it'd look a little weird in that case.

Maybe switch localhostname and the ip address?

alexeymin commandeered this revision.Jun 16 2019, 11:09 PM
alexeymin added a reviewer: bgray.
alexeymin edited the summary of this revision. (Show Details)
alexeymin edited reviewers, added: aacid; removed: bgray.
alexeymin added a project: KDE Applications.
alexeymin updated this revision to Diff 59965.Jun 16 2019, 11:12 PM

How about more complex logic then? Display hostname only if it is not empty string.

alexeymin updated this revision to Diff 59966.Jun 16 2019, 11:26 PM
alexeymin edited the summary of this revision. (Show Details)

Simplify code a bit

aacid accepted this revision.Jun 17 2019, 10:46 PM
This revision is now accepted and ready to land.Jun 17 2019, 10:46 PM
akulichalexandr added a subscriber: akulichalexandr.Jun 19 2019, 4:05 PM
akulichalexandr added inline comments.
krfb/mainwindow.cpp
150

Reduce memory allocations by using .arg(hostName, ipAddress)?

Not a big deal though. Looks good as it is :).

alexeymin updated this revision to Diff 60088.Jun 19 2019, 8:07 PM

Reduce memory allocations

alexeymin marked an inline comment as done.Jun 19 2019, 8:09 PM
Closed by commit R437:718b0ac000ea: Show hostname in krfb connection info (authored by alexeymin). · Explain WhyJun 19 2019, 8:09 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mkrfb/mainwindow.cpp (13 lines)

Diff 60089

View Options

krfb/mainwindow.cpp

Loading...