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 checking for gui updates #2100

Merged
merged 1 commit into from May 3, 2019

Conversation

Projects
None yet
5 participants
@mmbyday
Copy link
Contributor

commented Apr 21, 2019

@xmrdsc

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

function onWalletCheckUpdatesComplete(update) {

update is always empty for me. This is the bug I meant in #2089

@erciccione

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2019

I'm not sure the direct link is the best option.Maybe would be better something more mininmalistic? like: "a new version is available, check out getmonero.org".

@xmrdsc

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

To verify, how were you able to test the update mechanism? Can you confirm the GUI:

  1. Makes a DNS request on startup
  2. WalletManager::checkUpdatesAsync emits the checkUpdatesComplete with a non-empty update parameter when it is done checking.
  3. So that onWalletCheckUpdatesComplete(update) in main.qml is not empty
  4. So that it shows the dialog

^ cannot reproduce

@mmbyday

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@xmrdsc Good questions. I tested this PR against the v0.13.0.4 branch, as you may see from clues in the GUI (missing accounts, openalias placeholdertext)

Yes, your understanding sounds almost right. In step 2, checkUpdatesComplete can be empty when there is no update.

You can look under the hood of WalletManager::checkUpdates and see that the tuple of update data comes from Monero::WalletManager::checkUpdates here:
https://github.com/monero-project/monero/blob/29073f65e8816d4c32b6ffef514943a5650b8d3b/src/wallet/api/wallet_manager.cpp#L334

When there is no new update, this is why update parameter is empty.

return std::make_tuple(false, "", "", "", "");

Hope that helps you.

@mmbyday

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@erciccione That's a good option. It's simple and minimalistic as you say. Ultimately however, I think the typical user wants a one-click update though. That's what most user friendly wallets do for updates, like ledger-live. That would be a separate PR however. This one is just to get it working again :-)

@mmbyday mmbyday force-pushed the mmbyday:update-notifications branch from e4a57f2 to a23abc3 Apr 29, 2019

@mmbyday

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Rebased.

@xmrdsc

xmrdsc approved these changes Apr 30, 2019

main.qml Outdated
var msg = qsTr("New version of monero-wallet-gui is available: %1<br>%2").arg(version).arg(user_url) + translationManager.emptyString
//var user_url = parts[2]
//var auto_url = parts[3]
var osBuildTag = isMac ? "mac-x64" : isWindows ? "win-x64" : isLinux ? "linux-x64" : "unknownBuildTag"

This comment has been minimized.

Copy link
@xiphon

xiphon Apr 30, 2019

Contributor

Return or throw a error in the unknown/unrecognized OS case.

This comment has been minimized.

Copy link
@mmbyday

mmbyday May 1, 2019

Author Contributor

Excellent idea. Thanks.

@mmbyday mmbyday force-pushed the mmbyday:update-notifications branch from a23abc3 to 171e1fc May 1, 2019

@mmbyday

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Updated to handle unknown/unrecognized OS case with generic message.

@xiphon

xiphon approved these changes May 1, 2019

@luigi1111 luigi1111 merged commit 171e1fc into monero-project:master May 3, 2019

4 of 6 checks passed

buildbot/monero-gui-osx-10.13 Build done.
Details
buildbot/monero-gui-ubuntu-i686 Build done.
Details
buildbot/monero-gui-osx-10.11 Build done.
Details
buildbot/monero-gui-ubuntu-amd64 Build done.
Details
buildbot/monero-gui-win32 Build done.
Details
buildbot/monero-gui-win64 Build done.
Details

luigi1111 added a commit that referenced this pull request May 3, 2019

Merge pull request #2100
171e1fc Fix checking for gui updates (mmbyday)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.