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

New wizards + modes #1909

Merged
merged 2 commits into from Feb 22, 2019
Merged

New wizards + modes #1909

merged 2 commits into from Feb 22, 2019

Conversation

sanderfoobar
Copy link
Contributor

@sanderfoobar sanderfoobar commented Jan 18, 2019

This PR is the result of rewriting and redesigning the wizards. Summary:

  • All screens have been built up from scratch using knueffelbunds designs as reference.
  • Language sidebar makes it possible to switch language at any time. Introduces a flag icon in the titlebar.
  • Splash/Welcome shows upon first launch of the GUI.
  • Access wallets from a 'most recently opened' list.
  • Images are high DPI (Should look nice on retina).
  • Wallet name/location inputs check for duplicate wallet files on-the-fly (as you can't overwrite existing wallets).
  • Some input fields have regexp filters (restore height, subaddress lookahead).

Edit:

  • Now also includes mode selection wizards and functionality.

Technical notes:

  • Javascript logic related to opening/closing of wallets has been refactored and moved into wizardController.
  • An effort has been made to use explicit variable names, i.e: wizardController.variablename instead of merely variable (which is inherited via component hierarchy). This should in theory make the code more verbose and reveal where variables are actually coming from.
  • Layout. used wherever possible. Still some 'mixing anchors/layouts' warnings though. Could not get rid of them.
  • "recent wallets" functionality is implemented completely inside QML via FolderListModel, sorted on time modified.

Heads-ups for testers:

  • Most files under wizard/ have had their name changed which renders any existing translation mappings broken. I plan to write a Python script to (partially) migrate some translation strings to ease the pain for the translation workgroup.
  • Test creating a wallet
    • Using different languages
    • Test restore height functionality
  • Test if creating a wallet using Ledger works
    • Test restore height functionality
    • Test subaddress lookahead
  • Test if you can break "recent wallets"
  • Test performance of rotating globe on the welcome screen
  • Test if dynamically updating translations work (except for the wizards)

screenshots

@selsta
Copy link
Collaborator

selsta commented Jan 18, 2019

I’ve noticed so far:

@sanderfoobar
Copy link
Contributor Author

Thanks @selsta. I made some changes.

The wizard page is a little bit smaller than the normal GUI.

Window size between the wizards and wallet view stays the same now.

Language sidebar isn’t usable if custom decorations are off, the icon still seems to get slightly displayed.

Without custom decorations, the language can now be changed by going to Settings->Layout. In addition, there will be a button visible on the 'wizard home' screen to change the language.

Open a wallet from file -> Browse filesystem opens the GUI without a wallet open. If you press cancel inside the file dialog, the GUI stays open with no wallet file loaded.

During opening of a wallet, the GUI will now stay in the wizard view.

Language sidebar is empty for me (GUI and inside wizard).

Possibly related to your local setup, qmake should include these translations (via the .pro file).

In addition, I also rebased to latest master.

}

width: 240 * scaleRatio
height: parent.height - 50
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
height: parent.height - 50
height: parent.height - (persistentSettings.customDecorations ? 50 : 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks


onStatusChanged: {
if(status === XmlListModel.Ready){
console.log("languages availible: ",count);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed :) Was from old code.

text: "Change language"

onClicked: {
wizardController.wizardState = 'wizardLanguage';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wizardController.wizardState = 'wizardLanguage';
wizardController.wizardState = 'wizardLanguage';
languageSidebar.open();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one :)

console.log("languages availible: ",count);
if(count === 1){
console.log("Skipping language page until more languages are availible")
wizard.switchPage(true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wizard.switchPage(true);
if (wizardStateView.state = "wizardLanguage") {
wizard.switchPage(true);
}

I think this is more correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@selsta
Copy link
Collaborator

selsta commented Jan 28, 2019

I tried to debug the issue with languages not showing up without resizing but I’ve had no luck.

The only other improvement suggestion I have is some kind of scrollbar or something. Currently, if the window height isn’t high enough, there’s no way to move forward.

@sanderfoobar sanderfoobar changed the title New wizards [WIP] New wizards + modes Jan 29, 2019
@sanderfoobar
Copy link
Contributor Author

Note to self: integrate #1892

Copy link
Contributor

@mmbyday mmbyday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these two changes, Ledger Nano S hardware wallet will work again.


var written = wizardController.createWalletFromDevice();
if(written){
wizardStateView.state = "wizardHome";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wizardStateView.state = "wizardHome";
wizardStateView.state = "wizardCreateWallet2";

Otherwise, wizard will not advance to next page.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, you tested Ledger. I have yet to. Ill make these changes for the time being. Thanks.

import "../js/Wizard.js" as Wizard
import "../components"
import "../components" as MoneroComponents

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import moneroComponents.Wallet 1.0

Fixes
qrc:/wizard/WizardController.qml:342: ReferenceError: Wallet is not defined

@mmbyday
Copy link
Contributor

mmbyday commented Feb 14, 2019

Language sidebar is empty for me (GUI and inside wizard).

Possibly related to your local setup, qmake should include these translations (via the .pro file).

Also empty language sidebar for me, even with a clean build.

@mmbyday
Copy link
Contributor

mmbyday commented Feb 14, 2019

Seems like there's a regression with #1878

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my comment about a scrollbar is still valid, also the language bar still requires changing the window height to show up.

Will review more later.

strength = 100;

// privacyLevel component uses 1..100 scale
strength = Wizard.mapScope(1, 100, 1, 100, strength);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Collaborator

@selsta selsta Feb 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function does nothing if the input and output scale are both 1, 100.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

12:13 dsc_: If the password strength is fine, remove the mapscope function IMO.


delegate: StackViewDelegate {
pushTransition: StackViewTransition {
// PropertyAnimation {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to stay in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't see why not.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it commented out in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the comment. On second thought the animations look pretty sweet.

if (success) {
wizardController.m_wallet = wallet;
wizardController.walletOptionsIsRecoveringFromDevice = true;
wizardController.tmpWalletFilename = tmp_wallet_filename;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like @mmbyday pointed out, this should fix the regression (untested):

Suggested change
wizardController.tmpWalletFilename = tmp_wallet_filename;
wizardController.tmpWalletFilename = tmp_wallet_filename;
wizardController.walletOptionsRestoreHeight = wizardController.m_wallet.walletCreationHeight;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

if(walletLocation === "") return false;

var exists = Wizard.walletPathExists(walletLocation.text, walletName.text, isIOS, walletManager);
if(!exists && walletLocation.error === false) return true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return !exists && walletLocation.error === false;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please elaborate

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (x) return true;
else return false;

is the same as

return x;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping on this? It’s not a bug but it simplifies the code. Writing the return value explicit as you did isn’t usual.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

anchors.margins: 8 * scaleRatio
anchors.leftMargin: 10 * scaleRatio
font.family: MoneroComponents.Style.fontRegular.name
text: qsTr("Enter your 25 (or 24) word mnemonic seed") + translationManager.emptyString
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 24? 25 word seed without the checksum?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently:

text: qsTr("Enter your 25 (or 24) word mnemonic seed") + translationManager.emptyString

Probably checksum.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think we should remove mentions of 24 word seeds but not in this PR.

// Set `wizardProgress` width based on amount of progress dots
wizardProgress.width = 30 * menuNav.progressSteps;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline missing :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) fixed

@mmbyday
Copy link
Contributor

mmbyday commented Feb 15, 2019

This seems broken.

    • Test creating a wallet > Test restore height functionality

Also, on the final summary page of the wizard, when creating a new wallet from seed restore, the restore height is no longer displayed. The old wizard would show restore height. Working as intended? Or just missing.

@@ -1891,6 +1954,14 @@ ApplicationWindow {
passwordDialog.open();
}

function changeWalletMode(mode){
Copy link
Contributor

@mmbyday mmbyday Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Perhaps in the Layout/UI settings tab, allow users to change wallet modes. Or is there already a way to change modes in an existing wallet?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm. I see the mode selection button on the welcome to monero screen. Nice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a good suggestion. I think we can think about this at a later point. Would be nice to be able to more easily switch modes (whilst you have a wallet opened).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, this should be added later.

@mmbyday
Copy link
Contributor

mmbyday commented Feb 15, 2019

I think my comment about a scrollbar is still valid, also the language bar still requires changing the window height to show up.

Will review more later.

Confirmed. While language bar is open but empty, resizing the window will cause the languages to appear.

Layout.fillWidth: true

Text {
text: qsTr("This mode is ideal for managing small amounts of money. You have access to basic features for making and managing transactions. It will automatically connect to the Monero network so you can start using Monero immediately.") + translationManager.emptyString
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
text: qsTr("This mode is ideal for managing small amounts of money. You have access to basic features for making and managing transactions. It will automatically connect to the Monero network so you can start using Monero immediately.") + translationManager.emptyString
text: qsTr("This mode is ideal for managing small amounts of monero. You have access to basic features for making and managing transactions. It will automatically connect to the Monero network so you can start using Monero immediately.") + translationManager.emptyString

better IMO :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good catch.

btnNext.visible: false
progressSteps: 0

onPrevClicked: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: This Mode Selection screen could use a "previous" button, along with the change language button.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would previous bring you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess back to the main menu?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would add some complexity to determine if an user has already chosen a mode, in which case you can show the previous button but not on the first run. I would prefer to leave it like it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more vote for a Back to menu button on the Mode selection screen: If you arrive here from the menu it's impossible to back out without making a choice, with the added difficulty that right now this screen does not indicate which mode is the currently active one.


MoneroComponents.StandardButton {
small: true
text: qsTr("Mode selection") + translationManager.emptyString
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
text: qsTr("Mode selection") + translationManager.emptyString
text: qsTr("Change wallet mode") + translationManager.emptyString

For better button text consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
}

RowLayout {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps these two buttons would look better centered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think it will look OK since it is a change from the 'far-left' & 'far-right' button positions we have currently. In addition, there is no guarantee that there are 2 buttons displayed since they depend on custom decorations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to PR for it after this is merged though if you think it's important.

Layout.minimumWidth: 150 * scaleRatio
text: "Continue"

onClicked: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this extra click on the continue button required. If I click a language, that should be good to move on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is pretty confusing in terms of UX. Clicking language in the sidebar should not do anything more than (dynamically) switching the language it is currently displaying.

In addition, I think the complexity of code needed does not justify the goal.

opacity: 0.75
visible: true

MouseArea {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A mouseover effect, like that on the accounts page's listview, would be nice.
i.e.

                                onEntered: {
                                    delegateTable.color = "#26FFFFFF"
                                }
                                onExited: {
                                    delegateTable.color = "transparent"
                                }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The background of those items is actually an image asset. However, I was able to simulate a hover effect by playing around with the opacity. Looks better indeed.

font.pixelSize: 12 * scaleRatio
font.family: MoneroComponents.Style.fontRegular.name
color: MoneroComponents.Style.defaultFontColor
text: Version.GUI_VERSION + " (Qt " + qtRuntimeVersion + ")"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps some pre-text. Otherwise this text looks odd without context.

Suggested change
text: Version.GUI_VERSION + " (Qt " + qtRuntimeVersion + ")"
text: "Wallet version: " + Version.GUI_VERSION + " (Qt " + qtRuntimeVersion + ")"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not odd. That's just how software usually display their versions in splashes and/or intros.


wizardStateView.state = "wizardCreateWallet3";
}
onNextClicked: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scenario: restore wallet, open wallet, logout of wallet, restore wallet, then previous mnemonic still visible. Perhaps clear sensitive fields at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sanderfoobar
Copy link
Contributor Author

sanderfoobar commented Feb 16, 2019

Thanks for review... Some fixes/changes:

  • Fixed empty language sidebar with Qt >5.7
  • Hover effect language sidebar similiar to accounts/receive table
  • Fixed creating wallet with Ledger (famous last words)
  • Revamp WizardSummary.qml, made more modular.
  • Clear fields on reset for 'restore from device'

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ledger related.


var wallet = walletManager.createWalletFromDevice(tmp_wallet_filename, "", nettype, deviceName, restoreHeight, subaddressLookahead);

var success = wallet.status === Wallet.Status_Ok;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requires import moneroComponents.Wallet 1.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

wizardController.walletOptionsRestoreHeight = wizardController.m_wallet.walletCreationHeight;
} else {
console.log(wallet.errorString)
walletErrorDialog.text = wallet.errorString;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

walletErrorDialog is undefined as far as I can see.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@selsta
Copy link
Collaborator

selsta commented Feb 18, 2019

Bugs I’ve found:

  • Simple mode, remote node: Creating a new wallet skips the node page, but pressing Previous on the You're all set up page sends you to the node page.
  • Advanced mode: It will show a random remote node address on the You're all set up page even if you select local node only.
  • If you create a wallet from device and press the Previous button, you will get to wrong page.
  • Creating wallet from device is missing the dots.

value: walletOptionsName
}

WizardSummaryItem {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a testnet and stagenet wallet doesn’t seem to work currently.

}

WizardSummaryItem {
visible: persistentSettings.remoteNodeAddress !== ""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

persistentSettings.remoteNodeAddress should get set to "" if you use a local node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did visible: persistentSettings.remoteNodeAddress !== "" && appWindow.walletMode == 0 instead and same approach for subsequent bootstrap item.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this seems fixed now.

}

WizardSummaryItem {
visible: persistentSettings.bootstrapNodeAddress !== ""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a copy paste mistake.

Suggested change
visible: persistentSettings.bootstrapNodeAddress !== ""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as intended.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said on IRC, this doesn’t look right. Nettype has nothing to do with bootstrapNodeAddress. I’d say it should be always visible or only visible if the nettype is testnet/stagenet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, you're right. Fixed.

js/Utils.js Outdated

function netTypeToString(){
var nettype = appWindow.persistentSettings.nettype;
return nettype == NetworkType.TESTNET ? qsTr("Testnet") : nettype == NetworkType.STAGENET ? qsTr("Stagenet") : qsTr("Mainnet");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qrc:/js/Utils.js:94: ReferenceError: NetworkType is not defined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@sanderfoobar
Copy link
Contributor Author

sanderfoobar commented Feb 19, 2019

  • import moneroComponents.Wallet 1.0 where necessary
  • replaced non-existent walletErrorDialog with appWindow.showMessage()
  • Fixed debug leftover @ WizardSummary
  • Hide advanced options on the wizards main menu when using simple mode
  • Enabled Stackview transition animations for all wizard views

Creating a new wallet skips the node page, but pressing Previous on the You're all set up page sends you to the node page.

Fixed

Creating a testnet and stagenet wallet doesn’t seem to work currently.

Fixed via import moneroComponents.NetworkType 1.0

NetworkType is not defined @ qrc:/js/Utils.js:94

Fixed.

Creating wallet from device is missing the dots.

Added

@xiphon
Copy link
Collaborator

xiphon commented Feb 19, 2019

The PR is too big, I can't review it.
Have couple general remarks:

  1. For obvious reason, i don't like the idea to hardcode https://autonode.xmr.pm/ URL to fetch remote nodes list from.
    Given that most Monero GUI users will use default hardcoded url and Simple mode, this change makes them to blindly trust to some person controlling https://autonode.xmr.pm/.
    Such a kind of centralization introduces single point of failure, heavily affects most Monero GUI users' privacy.
  2. I don't get the idea behind the rotating globe welcoming screen. Why do we need it? What are the pros in terms of UX?

@selsta
Copy link
Collaborator

selsta commented Feb 19, 2019

We are getting close to merging this :)

I think this should still get fixed though:

If you create a wallet from device and press the Previous button, you will get to wrong page.

https://user-images.githubusercontent.com/7697454/53013705-b75a1500-3446-11e9-835b-d580665d7543.gif

@sanderfoobar
Copy link
Contributor Author

sanderfoobar commented Feb 20, 2019

Thanks @selsta

  • Removed function mapScore (password strength)
  • Fixed 'Previous' button while restoring from device
  • Fixed network type visibility on the 'All finished page'

@xiphon regarding rotating globe; perhaps talk with kneuffelbund on IRC and see what he thinks. Maybe he can come up with something. Removing it all together is also fine by me but would rather do that in a separate PR where we can discuss the starting screen more in depth - as we are currently limited to make release in time.

As for autonode.xmr.pm; it was proposed in issue 1846 and discussed multiple times on IRC. TL;DR: temp. solution until node-o-matic in C++ is implemented. autonode.xmr.pm is like a seed node and core team has full access.

Another fix via @dEBRUYNE-1:

If a user already has a wallet, I presume the UI for him will be 'Advanced' mode

Wallet defaults to Advanced mode when no prior mode configuration is found.

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work @xmrdsc, must have been a lot of work! I’ve approved it but left some small comments.

else{
return 0;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newline :)

property alias viewState: rootItem.state

property string remoteNodeService: {
// support user-defined remote node aggregators
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a TODO to finish support for user-defined remote node aggregators. I think there’s currently no way to set one. Or is it intended to be set in code only?

@@ -81,7 +82,21 @@ ApplicationWindow {
readonly property string localDaemonAddress : "localhost:" + getDefaultDaemonRpcPort(persistentSettings.nettype)
property string currentDaemonAddress;
property bool startLocalNodeCancelled: false
property int estimatedBlockchainSize: 50 // GB
property int disconnectedEpoch: 0
property int estimatedBlockchainSize: 75 // GB
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a function that estimates the blockchain size. Just an idea, not for this PR.

appWindow.disconnectedEpoch = 0;
return;
}, function(){
appWindow.showStatusMessage(qsTr("Failed to fetch remote nodes from third-party server."), simpleModeConnectionTimer.interval / 1000);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens to simple mode users if your server goes down?

@@ -1891,6 +1954,14 @@ ApplicationWindow {
passwordDialog.open();
}

function changeWalletMode(mode){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, this should be added later.

<file>images/create-wallet.png</file>
<file>images/remote-node.png</file>
<file>images/local-node.png</file>
<file>images/local-node-full.png</file>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • <file>images/local-node-full@2x.png</file>
  • <file>images/local-node@2x.png</file>
  • <file>images/remote-node@2x.png </file>

id: bootstrapNodeEdit
Layout.minimumWidth: 300 * scaleRatio

//labelText: qsTr("Bootstrap node (leave blank if not wanted)") + translationManager.emptyString
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn’t supposed to stay in?

// Copyright (c) 2014-2018, The Monero Project
//
// Copyright (c) 2014-2019, The Monero Project
//
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray spaces in the following lines look unintended.

@luigi1111 luigi1111 merged commit c6107d8 into monero-project:master Feb 22, 2019
luigi1111 added a commit that referenced this pull request Feb 22, 2019
f329a71 Wizard redesign (xmrdsc)
c6107d8 wizard: allow calendar date for restoration height (mmbyday)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants