Automatic Spoiler Season #2991
Automatic Spoiler Season #2991
Conversation
|
As said in gitter, Cockatrice crashes immediately after/during updating on my Windows 10. I need to retest with a clean install and fresh settings now.
|
| @@ -3,6 +3,7 @@ | |||
| #include <QIcon> | |||
| #include <QTranslator> | |||
| #include <QLibraryInfo> | |||
| #include <QCommandLineParser> | |||
There was a problem hiding this comment.
I'm leaving the changes in Oracle as an Easter egg that can be used in the future.
There was a problem hiding this comment.
the first attempt of this PR used Oracle to download the spoilers. Since I find that code to be useful, I have decided to leave it in the PR as an Easter Egg so it can be used in the future by another process (who knows, maybe we'll want to have Oracle do new things, and this is a great example of what it is capable of!)
If I understand it correctly you added unused code, shouldn't you comment that accordingly in the code at least?
The reference in this pr will be forgotten and the next coder will wonder. :)
There was a problem hiding this comment.
There's nothing to comment really. It's fairly obvious this code isn't used (as there are no calls to it).
… elsewhere to prevent the reload if spoiler check happens
|
I really like that we have something working like this now 👍 Here is my list of thoughts:
If you add a hint and explain the data a bit it's more helpful to everybody. Original message:
Design: (I'm sorry, but we never spoke about it before)
I retested with the latest commit btw and the reload problem is now also covered. |
|
|
||
| setLayout(mainLayout); | ||
| auto *lpGeneralGrid = new QGridLayout; | ||
| auto *lpSpoilerGrid = new QGridLayout; |
There was a problem hiding this comment.
All GeneralGrid could be removed maybe?
It has been empty for a long time now, and most likely will for some more... I don't know of any feature in the planning that could end up here!
And it looks not really nice 😄
There was a problem hiding this comment.
I don't see harm in leaving it - let's not increase the amount of stuff being changed since this is already large
|
I will respond to your bullets with my own bullets.
Design Bullets 1 & 2 have been address. For 3 & 4, I'll wait to see what others think |
|
The description in the main pr comment sounds like an excellent workflow. I might offer a tip that on error we could pop up a notification that instructs the user to check the debug log, but that's minor - overall the workflow sounds good now. |
cockatrice/src/carddatabase.cpp
Outdated
| #include <QMessageBox> | ||
|
|
||
| const int CardDatabase::versionNeeded = 3; | ||
| const char* CardDatabase::TOKENS_SETNAME = "TK"; | ||
|
|
||
| static QXmlStreamWriter &operator<<(QXmlStreamWriter &xml, const CardSet *set) | ||
| { | ||
| if (! set) |
There was a problem hiding this comment.
Can you explicitly check nullptr here rather than the implicit style please? It helps with clarity and intent
There was a problem hiding this comment.
And further down in the file also
cockatrice/src/carddatabase.cpp
Outdated
| } | ||
| } | ||
|
|
||
| static QXmlStreamWriter &operator<<(QXmlStreamWriter &xml, const CardInfo *info) | ||
| { | ||
| if (! info) | ||
| { | ||
| qDebug() << "operator<< (~376) info is nullptr"; |
There was a problem hiding this comment.
Was referring to the line number, i'll be more explicit
|
|
||
| const QList<CardRelation *> related = info->getRelatedCards(); | ||
| for (int i = 0; i < related.size(); i++) { | ||
|
|
||
| for (auto i : related) |
| @@ -124,12 +123,12 @@ DlgConnect::DlgConnect(QWidget *parent) | |||
| QGroupBox *btnGroupBox = new QGroupBox(tr("")); | |||
| btnGroupBox->setLayout(buttons); | |||
|
|
|||
| QGridLayout *grid = new QGridLayout; | |||
| auto *grid = new QGridLayout; | |||
There was a problem hiding this comment.
I don't really see the point in auto-fying these but I guess it doesn't hurt
There was a problem hiding this comment.
CLion says auto is better and more "modern" so I'm listening to it and fixing all the "warnings" i come across
|
|
||
| setLayout(mainLayout); | ||
| auto *lpGeneralGrid = new QGridLayout; | ||
| auto *lpSpoilerGrid = new QGridLayout; |
There was a problem hiding this comment.
I don't see harm in leaving it - let's not increase the amount of stuff being changed since this is already large
| // Delete the spoiler.xml file as we're not in spoiler season | ||
| if (file.exists() && file.remove()) | ||
| { | ||
| qDebug() << "Spoiler Season Offline, Deleting spoiler.xml"; |
There was a problem hiding this comment.
Can you also pop a desktop notification telling the user the spoiler file is being removed and they need to run oracle? Otherwise checking for updates will remove cards from their db and leave them confused
There was a problem hiding this comment.
Makes sense, yeah I can do that
|
|
||
| /* | ||
| * ALERT: Ensure two reloads of the card database do not happen | ||
| * at the same time or a racetime condition can/will happen! |
There was a problem hiding this comment.
We should probably have a mutex around db reloading to avoid that, but maybe in another pr unless you feel up to adding that. Might need refactoring
There was a problem hiding this comment.
Agreed. I'm going to hold off on that so we can get this PR in and i'll start working on it later.
| } | ||
|
|
||
| // Check if the data matches. If it does, then spoilers are up to date. | ||
| if (getHash(fileName) == getHash(data)) |
There was a problem hiding this comment.
Are you hashing the file name or the file contents? Maybe include the individual hash values in a qDebug?
There was a problem hiding this comment.
nvm I see it's overloaded. That works
| if (file.write(data) == -1) | ||
| { | ||
| qDebug() << "Spoiler Service Error: File write (w) failed for" << fileName; | ||
| return false; |
There was a problem hiding this comment.
The file handle won't be closed in this branch I think?
There was a problem hiding this comment.
Never has been, but I can make it close
| if (file.open(QFile::ReadOnly)) | ||
| { | ||
| // Only read the first 512 bytes (enough to get the "created" tag) | ||
| const QByteArray bytes = file.read(512); |
There was a problem hiding this comment.
Might be worth qDebugging this value
There was a problem hiding this comment.
An empty spoiler.xml file looks like this: https://github.com/Cockatrice/Magic-Spoiler/blob/77ec169ec5ec36352d02db8ab1137a780ba7133e/spoiler.xml
It has 272 bytes, so only checking first 256bytes instead 512 is sufficient?
Adding a do-not-edit hint as requested by you, won't change too much since it could simply be placed behind the timestamp.
There was a problem hiding this comment.
@tooomm I read the files and put it through systems to get byte counts. Most services tell me i'll need ~400 bytes to be certain to capture the date. As such, I put a power of 2 in place. 512 bytes isn't that much 😄
|
Finished testing the second last iteration (c70dfc3) Further thoughts:
The updated pr description is very helpful and detailed btw 👍 |
|
That last error is kind of concerning. Can you reproduce it reliably? |
|
@tooomm I've resolved all bullets and have made the reload thread safe. The last error is no longer an issue based on the refactoring I just completed. |
|
Nice, thank you!
Sadly I can still reproduce the error the same as before. And yes @Daenyth, it can easily and 100% reliable be reproduced: |
This PR contains a lot, so lets try and break it down a bit.
The goal of this PR is to allow users to automatically download and manage their spoilers through our spoiler service, Magic-Spoiler. In order to accomplish this, I've added components to the settings dialog.
If the user ticks the "Download Spoilers Automatically" button, the following gears internally will start turning:
On Cockatrice launch, check if the file "SpoilerSeasonEnabled" exists on the Magic-Spoiler repo. If it does not, then we know we are not in spoiler season and will continue with normal launch.
If the file does exist, then we will continue and now download spoiler.xml from the Magic-Spoiler repo. The system will then check if the downloaded copy matches the current local copy. If there is no local copy or the file hashes do not match, then we will replace the local copy with the downloaded copy and alert the user.
If the user presses the "Update Spoilers" button in the system settings, it will trigger bullet point 1 above with the SpoilerSeasonEnabled check and continue down the line.
If any errors occur, they will be properly logged in the debugger.
In addition to the changes above, there is some general cleanup in the code base (as dictated by CLion's strict IDE policy of ensuring top quality code). Some examples include adding
explicitto functions oroverrides. In addition, there are some bracket realignments and header realignments to meet the new coding policy (which will be updated in the near future).Continuing on, the first attempt of this PR used Oracle to download the spoilers. Since I find that code to be useful, I have decided to leave it in the PR as an Easter Egg so it can be used in the future by another process (who knows, maybe we'll want to have Oracle do new things, and this is a great example of what it is capable of!)
Finally, I detected a race-time condition that can come about if the system attempts to reload the card database while a reload is currently in progress. I have added a Mutex to make the card database reload thread safe from now on.