Skip to content

Conversation

vveljko
Copy link
Contributor

@vveljko vveljko commented Jun 3, 2019

  • Use TTL from DNS response, cache resolved addresses for that
    period
  • Identify Windows vs POSIX and C vs C++ (vs Qt) for reporting
    to the server
  • Detect authenticating proxy that works incorrectly, mostly the
    kind of broken proxy that keeps asking for the credentials
    over and over again, as-if we're not providing them.

vveljko added 5 commits June 1, 2019 14:52
- Use TTL from DNS response, cache resolved addresses for that
  period
- Identify Windows vs POSIX and C vs C++ (vs Qt) for reporting
  to the server
- Detect authenticating proxy that works incorrectly, mostly the
  kind of broken proxy that keeps asking for the credentials
  over and over again, as-if we're not providing them.
Fix a bunch of portability warnings. Update `clang-format` on
changed files.
Also, improve `windows-gcc` makefile (not quite completely, though).
@vveljko vveljko marked this pull request as ready for review June 3, 2019 19:23
@vveljko vveljko requested a review from davidnub June 3, 2019 19:23
Copy link

@davidnub davidnub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cleanup looks good! I like the clean refactor of multiple addresses.

I have a few nitpick comments. Most of the changes I'm thinking is the OS reporting in Qt.

realm currently in use
*/
pbhtdig_EqualConsecutiveRealms,
/** atribute 'realm' is not found yet in 'authentication required' message header
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Typo in comment, should be attribute

el.end = p->http_reply + p->http_buf_len;
jpresult = pbjson_get_object_value(&el, "t", &found);
if (jonmpOK == jpresult) {
struct pbjson_elem titel;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Should this variable be title ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no, it's a short-form of "_ti_me _t_oken _el_ement". It's a kind of an inside joke, also, there's a small town called "Titel" in Serbia. :)

qt/pubnub_qt.cpp Outdated
static QString GetOsName()
{
#if defined(Q_OS_ANDROID)
return QLatin1String("android");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Capitalize -> Android

qt/pubnub_qt.cpp Outdated
#if defined(Q_OS_ANDROID)
return QLatin1String("android");
#elif defined(Q_OS_BLACKBERRY)
return QLatin1String("blackberry");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Capitalize -> Blackberry

qt/pubnub_qt.cpp Outdated
#elif defined(Q_OS_MACOS)
return QLatin1String("MacOS");
#elif defined(Q_OS_TVOS)
return QLatin1String("Tvos");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Casing -> tvOS

Per Apple's developer page: https://developer.apple.com/

qt/pubnub_qt.cpp Outdated
#elif defined(Q_OS_IOS)
return QLatin1String("iOS");
#elif defined(Q_OS_MACOS)
return QLatin1String("MacOS");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Casing -> macOS

Per Apple's developer page: https://developer.apple.com/

qt/pubnub_qt.cpp Outdated
#elif defined(Q_OS_TVOS)
return QLatin1String("Tvos");
#elif defined(Q_OS_WATCHOS)
return QLatin1String("Watchos");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Casing -> watchOS

Per Apple's developer page: https://developer.apple.com/

qt/pubnub_qt.cpp Outdated
#elif defined(Q_OS_WATCHOS)
return QLatin1String("Watchos");
#elif defined(Q_OS_WINCE)
return QLatin1String("Wince");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Perhaps we should call it Windows CE ?

Their docs have stopped shortening it: https://www.microsoft.com/en-us/download/details.aspx?id=14226

Copy link

@davidnub davidnub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! LGTM 👍

@vveljko vveljko merged commit 8e20fea into master Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants