| ngraham | |
| broulik |
| Frameworks |
Not entirely sure about this approach (and the stuff to get the test working), but I use kunitconversion in mangonel and the blocking behavior annoyed me a lot.
A simpler approach would be to just try to check and download the cache immediately, but still force wait in the foreground, but the code (at least behavior) gets a lot more complex, especially with threading.
The tests pass, it gets the currencies fairly quickly but doesn't block the UI thread.
| Lint Skipped |
| Unit Tests Skipped |
I don't fully understand the need for a thread here.
It merely blocks because the API is synchronous and thus we create a nested eventloop.
I don't think the writing and parsing of the cache file is a real bottleneck here.
| src/currency.cpp | ||
|---|---|---|
| 57 | Can probably just use QNetworkConfigurationManager::isOnline()? | |
| 123 | Use prefix s_ since it's static | |
| 153 | Whenever you do networking in KDE, ensure it follows redirects: | |
I don't use any threading (unless I'm missing something)? I just rewrote it to remove the nested event loop and instead use the normal one, and switched from a mutex to a lockfile (I feel that is more appropriate for this usecase, QSaveFile is more for data integrity and not racing between applications in my head at least).
Fixed review comments
| src/currency.cpp | ||
|---|---|---|
| 57 | IIRC that's less reliable (e. g. if you don't use networkmanager or connman). the whole bearer thing is a bit messy, I guess that's why it's going away. but this is also not very good, it just checks for IPs while it should at the very least wait for a route to a gateway (I spent way to much time on this problem at work...). tldr: we could probably just remove the whole thing, that's why I added the retry-timer. | |
| 153 | I don't think that makes sense, but sure. | |
| 887 | Because:  | |