Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix updatecheck result logging #2424

Merged
merged 1 commit into from Jul 18, 2016

Conversation

@Kissaki
Copy link
Member

commented Jul 14, 2016

Entirely drop the separate autoupdate logic, and no longer use the auto parameter of the update check service. Always log success/failure on any update check.

Fixes unintended backwards logic of when to log result (by always logging as mentioned).

Three commits you may want to check individually. They also contain further info about the individual changes.

Fix missing update check failure message
We always want to log when the updatecheck fails.
As it was, when the info was logged was backwards.
Now it always logs.
@@ -65,121 +65,119 @@ VersionCheck::VersionCheck(QObject *p, bool focus) : QObject(p) {
}

void VersionCheck::fetched(QByteArray a, QUrl url) {
if (! a.isNull()) {
if (! a.isEmpty()) {
if (! a.isNull() && ! a.isEmpty()) {

This comment has been minimized.

Copy link
@mkrautz

mkrautz Jul 14, 2016

Member

I believe isEmpty implies isNull. See examples at http://doc.qt.io/qt-4.8/qbytearray.html#isEmpty

This comment has been minimized.

Copy link
@Kissaki

Kissaki Jul 14, 2016

Author Member

True.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

I don't get the removal of auto.

There's a distinct difference between auto and non-auto:

For auto=1 we shouldn't output anything if we're on the latest version.
We do a version check on startup. If we litter users's logs with "You're using the latest version!" that'd be a behavior change -- for the worse, IMO.

For auto=0, it should output the $latest line.

Can you please elaborate?

Otherwise the two other commits LGTM (with my comments addressed).

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2016

No particular reason apart from making it simpler and always logging. Mostly taking it further, as a suggestion.
I guess the latest version info is pretty useless/annoying in the always visible textlog. So I’ll drop the latter two commits.

@Kissaki Kissaki force-pushed the Kissaki:fix-autoupdatemsg branch from 8b41783 to edba745 Jul 14, 2016

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2016

Adjusted the diff, now its just removing the bSilent

@Kissaki Kissaki changed the title Always log updatecheck result, drop special logic Fix updatecheck logging Jul 14, 2016

@Kissaki Kissaki changed the title Fix updatecheck logging Fix updatecheck result logging Jul 14, 2016

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 14, 2016

Now I'm confused. Wasn't the isEmpty/isNull fix OK?

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 15, 2016

@Kissaki Ping on the question above. :-)

1 similar comment
@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 16, 2016

@Kissaki Ping on the question above. :-)

@Kissaki

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2016

It merges the case isNull case (which I guess is on request errors/issues) which logs an error with the isEmpty case, which (I expect to be) is an empty response from the updatecheck given the auto parameter. As I understood, merging the two is not what we want, as we do not want to log on latest version on autoupdate.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Jul 18, 2016

LGTM

@mkrautz mkrautz merged commit edba745 into mumble-voip:master Jul 18, 2016

mkrautz added a commit that referenced this pull request Jul 18, 2016

@Kissaki Kissaki deleted the Kissaki:fix-autoupdatemsg branch Jul 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.