OSInfo improvements

[Zuko]

More information about OS in Server Information and User Information.

Screens:

[mkrautz]

We should be careful of changing the OSInfo getOS and getOSVersion methods. They're used for our usage stats (src/mumble/Usage.cpp), but also in other things where long strings might look stupid, such as the User-Agent for Mumble's HTTP requests (src/mumble/Network.cpp; Network::prepareRequest).

Now, for Unix-like systems, I believe getOS and getOSVersion can return some pretty long strings already, so take that with a grain of salt. I'm actually more wary of breaking the Windows stats.

In this situation, I think it would be preferable to have a new set of methods that return "full" or "user displayable" version strings instead. Perhaps something like getOSDisplayableName, getOSDisplayableVersion. Suggestions are welcome.

[Zuko]

You are right, working on it.

[Zuko]

Done

[Kissaki]

I don’t think there is a need to transmit displayable strings!?
Instead, the identifying strings are already provided - which the client can translate to human readable form.
The server does not need to know about human readable strings.

[Zuko]

I cant get all informations from version string.
17 paź 2013 00:51 "Jan Klass" notifications@github.com napisał(a):

In src/Mumble.proto:

@@ -7,6 +7,7 @@ message Version {
optional string release = 2;
optional string os = 3;
optional string os_version = 4;

• optional string os_displayable_version = 5;

I don’t think there is a need to transmit displayable strings!?
Instead, the identifying strings are already provided - which the client
can translate to human readable form.
The server does not need to know about human readable strings.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/178/files#r7018948
.

[Kissaki]

Is Version used to send server and user version info?

[Zuko]

There is no other message, so yes.

[mkrautz]

Drop the caching in getOSDisplayableVersion.

Move the ifdef Q_OS_WIN for getOSDisplayableVersion inside the method.
Make getOSDisplayableVersion return an empty/null QString (return QString(), or whatever fits the code) for non Q_OS_WIN.

If getOSDisplayableVersion is empty, don't include it when transmitting the Version protobufs.

Remove the Q_OS_WIN ifdef in UserInformation.cpp.
Also in UserInformation.cpp, use mpv.has_os_displayable_version() instead of checking for emptiness.

A question: in your screnshots, your Polish version has Polish strings for the displayable version. I don't see any code in the current pull request that does that - and I think we should keep the strings transmitted over the wire in English. (I think the code as-is is fine in this regard.)

My own comments:

I think this change makes sense. As a server admin (or just power user), you will sometimes help
other people on the server with Mumble or other computer-related issues. In this case, it can
sometimes be useful to know the exact version of the OS they're running.

I'm fine with the "displayable version" being the only string shown in UserInfo if it is present in the Version protobuf, but I also think it'd be nice if we could show the OS and OSVersion values somewhow. A tooltip would be my preferred choice, but I guess that's a bit tough to do with the current structure, so let's wait with that.

I'd like to know what other devs think about the change before we land it, though.

For example, on Unix-like systems, I believe the OSVersion field is already pretty free-form. Maybe we should drop os_displayable_version from the protobuf, and simply use the same mechanism for Windows? Or alternatively, transition the Unix code over to os_displayable_version.

[Zuko]

• all mkrautz suggested changes are done.

[mkrautz]

Please just use a single #else clause (drop Q_OS_MAC) and return QString();

[Zuko]

two return's? one from indef and one without? I don't get it ;/
EDIT. nvm I'm stupid ;D

[mkrautz]

    QString OSInfo::getOSDisplayableVersion() {
#ifdef Q_OS_WIN
        // your current code
        return osdispver;
#else
        return QString();
#endif
    }

[mkrautz]

Seems good to me now, with the few nits I just added.

I will try to ping the others to see what they think.

[hacst]

formating

[Zuko]

bump? 👍 🎱

[mkrautz]

Landed as 8f0ae69.

Sorry for the long wait.

Slightly modified to add some additional error handling and remove the new field of the Version protobuf, which was deemed overkill in the end.