dev release channel #2557
dev release channel #2557
Conversation
cockatrice/src/releasechannel.cpp
Outdated
| @@ -49,7 +51,7 @@ bool ReleaseChannel::downloadMatchesCurrentOS(const QString &fileName) | |||
| #include <QSysInfo> | |||
|
|
|||
| bool ReleaseChannel::downloadMatchesCurrentOS(const QString &fileName) | |||
| { | |||
| {VERSION_STRING_NUMS | |||
| * This can be either a pre-release or a published release | ||
| * depending on timing. Both are acceptable. | ||
| */ | ||
| QVariantMap resultMap = array.at(0).toObject().toVariantMap(); |
There was a problem hiding this comment.
array.at(0) can fail if we ever deleted the releases or if the github api for some reason returned bad json.
Does the app crash in that case?
There was a problem hiding this comment.
Did not account for this will re-review
There was a problem hiding this comment.
If array size is 0, will return as a bad input.
If bad json is found, that is checked for below
There was a problem hiding this comment.
array.at(0) will return a QJsonValue of type Undefined; the following call to toObject() will return an empty QJsonObject, and toVariantMap() will then return an empty QVariantMap.
Each one of these calls could fail for some reason, but it should be safe anyway.
cockatrice/src/releasechannel.cpp
Outdated
| continue; | ||
| if(!map["version"].toString().endsWith(shortHash)) | ||
|
|
||
| // File name _MUST_ contain the build hash |
There was a problem hiding this comment.
Quite awkward. Do we have to have it this way? Can't we just go off the tag name + file?
Dev channel updater should release stable releases if that's the latest upload
There was a problem hiding this comment.
I agree it's awk, will look at a change
There was a problem hiding this comment.
I just noticed you're also using QJsonDocument instead of QtJson. That's not wrong, but we probably want to stick to one only inside the file for now - if we're setting a baseline Qt version that assumes QJsonDocument is present that's ok, but I thought it was only present in very new versions?
|
@Daenyth QJsonDocument was added in QT5.0.0, so it's fine to use. |
|
Works on Mac as expected, will need Windows check done to verify. |
cockatrice/src/releasechannel.cpp
Outdated
| << "name=" << lastRelease->getName() | ||
| << "desc=" << lastRelease->getDescriptionUrl() | ||
| << "commit=" << lastRelease->getCommitHash() | ||
| << "date=" << lastRelease->getPublishDate(); | ||
|
|
||
| qDebug() << "Searching for a corresponding file on the dev channel: " << QString(DEVFILES_URL); | ||
| response = netMan->get(QNetworkRequest(QString(DEVFILES_URL))); | ||
| QString devbuilddownload_url = resultMap["assets_url"].toString(); |
There was a problem hiding this comment.
minor: please don't mix different variable name styles. Everything else is camelCased, this should be devBuildDownloadUrl or similar.
| auto exeDebugName = devSnapshotEnd + ".exe"; | ||
| return (fileName.endsWith(exeName) || fileName.endsWith(exeDebugName)); | ||
| auto exeDevName = devSnapshotEnd + ".exe"; | ||
| return (fileName.endsWith(exeName) || fileName.endsWith(exeDevName)); |
| qWarning() << "Invalid received from the release update server:" << tmp; | ||
| // Make sure resultMap has all elements we'll need | ||
| if (!resultMap.contains("assets") || | ||
| !resultMap.contains("author") || |
There was a problem hiding this comment.
We don't use author anymore?
Can't find assets either.
|
Since this works as expected, on mac/windows, will merge when passes |
This fixes the dev build update cycle. The dev builds will now come from GitHub and take the latest release posted. If the latest release is a pre-release, for example, it will take that. If it's a full release, it will download that.
The following images will be seen if you're on the dev build only:
If a full release is available, this should be seen:
If a dev release is available, this should be seen:
