Fix #3241 UX/UI - Introduce PrivateSend tab which allows to spend fully mixed coins only #3442

10xcryptodev · 2020-04-19T05:58:04Z

Apply the changes suggested on #3241

UdjinM6 · 2020-04-19T10:44:57Z

Looks good code-wise but I don't like the new layout tbh.

10xcryptodev · 2020-04-19T13:36:12Z

@UdjinM6 agree with you. Another suggestion that i thought is use the old layout, remove the PrivateSend checkbox and add the option in confirmation dialog Yes (PrivateSend). It will be cleaner and the user will always see the option before send

UdjinM6 · 2020-04-20T12:04:03Z

Simply removing PS checkbox won't work because:

it also switches "Balance" from "All" to "PS only" numbers, you'd have to show both balances if you remove it;
it affects numbers in CoinControl labels (PS txes can't have change, "fee"/"after fee" are adjusted accordingly), duplicating these would be a mess.

IMO the GUI is already pretty clear + there is another label about funds you are going to use (mixed/no-mixed) in confirmation box. The only possible tweak I can think about is that we could add another button "PrivateSend" right next to "Send" which would internally call the same on_sendButton_clicked() but would be disabled if PS checkbox is off (and "Send" would be disabled if PS checkbox is on). Not sure if that would it make things clearer or the other way around though.

10xcryptodev · 2020-04-21T00:49:09Z

@UdjinM6 PrivateSend changes the available balance and inputs, so it should be the first step the user should choose. I thought 2 possibilities:

Create a new PrivateSend tab between Send and Receive that will use the same UI (sendcoinsdialog.ui) and behind will act "as the checkbox is activated"
Ask the user when enter the Send tab if want to Send or PrivateSend using a confirmation dialog, the checkbox could stay at the same place, but the user will always be asked the option

Personally i prefer the first option, it will be clear and simple and give us more possibilities if we want to add more PrivateSend features in the future

UdjinM6 · 2020-04-21T20:39:09Z

Hmm.... yeah, I would probably go with (1) assuming it's going to reuse the same code/ui.

10xcryptodev · 2020-04-22T03:17:55Z

@UdjinM6 implemented the number 1 solution, reusing the same UI

UdjinM6

I have one question/change request and still kind of on the fence if new UI looks better or not but otherwise code changes look good IMO 👍

PS. and it works as expected as far as I can tell.

src/qt/coincontroldialog.cpp

src/qt/forms/sendcoinsdialog.ui

UdjinM6

ACK

PastaPastaPasta · 2020-04-25T23:25:52Z

Could we get some screen-shots of the new format etc?

PastaPastaPasta

some formatting nits

src/qt/coincontroldialog.cpp

src/qt/walletframe.cpp

src/qt/walletview.cpp

10xcryptodev · 2020-04-27T00:40:12Z

@PastaPastaPasta code formating updated

Screenshots:

New PrivateSend tab, shows only the mixed balance and mixed outputs in coin control

This way will be clear for the user that is using PrivateSend in his first action selecting the type of sending. Also before this change, the user could select PrivateSend checkbox and select a not mixed output in coin control, that could cause confusion too.

PS.: Didn't finished the mixing on my test wallet, it's a good test to do. I will finish the mixing process and see

UdjinM6 · 2020-04-27T12:37:28Z

Also before this change, the user could select PrivateSend checkbox and select a not mixed output in coin control, that could cause confusion too.

We have a warning for this case https://github.com/dashpay/dash/blob/develop/src/qt/coincontroldialog.cpp#L430-L436 (which I think can be dropped now btw since this should be no longer possible) but it's much better to hide non-mixed inputs completely, I like that.

UdjinM6 · 2020-04-27T12:44:43Z

Hmm... Interesting. So switching from Send to PrivateSend tab with non-mixed inputs selected via CoinControl on the former results in some new confusing situation when it says N inputs selected but you can't see them cause they are not in the list. 🤔 PS would still fail as it should saying "not enough mixed funds" so it's not that bad but maybe we could also clear non-mixed inputs from selection when CoinControl is opened via PS tab?

10xcryptodev · 2020-04-27T22:16:01Z

@UdjinM6 great! I will check to clear the selections when changing the tab and remove the unnecessary warning

10xcryptodev · 2020-04-29T02:01:13Z

@UdjinM6 reviewed the points about clear selected outputs and warning message

UdjinM6 · 2020-04-29T12:10:52Z

Hmm... clear() is pretty disruptive (imagine filling 10 recipients and then switching tabs accidentally). I was thinking about smth like 803f029a8c (and maybe smth like a00c2e98 on top cause silently dropping coins from selection feels kind of weird).

10xcryptodev · 2020-05-01T11:58:48Z

@UdjinM6 thanks for the contribution, tested and agreed with the better UX. Added the suggested commits

PastaPastaPasta · 2020-05-01T21:42:51Z

test failure looks unrelated, can you force-push and hopefully CI will be happy

10xcryptodev · 2020-05-02T02:24:19Z

@PastaPastaPasta passed CI check now

UdjinM6 · 2020-05-02T15:16:12Z

Found another issue - when running with --enableprivatesend=0 the PS tab shows "Balance: 0" yet it allows to send PS txes. I think it makes sense to disable the tab (and the corresponding icon/tray menu item) in this case i.e. ef3f107344.

10xcryptodev · 2020-05-03T01:10:45Z

@UdjinM6 good catch! also tested with -litemode and it's ok. Added the suggestion

UdjinM6

ACK

PastaPastaPasta · 2020-05-11T16:34:59Z

Will review in the next few days

PastaPastaPasta · 2020-05-13T18:20:56Z

Started a review, when clicking on "PrivateSend" it doesn't become bold and unbold the prior.

PastaPastaPasta · 2020-05-13T18:55:24Z

Also, "Use available balance" button doesn't work properly

Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

10xcryptodev · 2020-05-14T19:47:05Z

@PastaPastaPasta rebase done 👍

PastaPastaPasta · 2020-05-14T21:04:52Z

Man you must be really unlucky, cause there are still test failures that seem unrelated

PastaPastaPasta

reACK: 791908e
light code review utACK: b373b33

UdjinM6

👍

Slightly tested ACK

PastaPastaPasta · 2020-05-14T22:11:43Z

test passed this time 👍

10xcryptodev · 2020-05-14T22:16:30Z

Lucky now 😆

…end fully mixed coins only (dashpay#3442) * dashpay#3241 change PrivateSend and balance location * include resource location back * change back balance and privatesend checkbox position * add new PrivateSend tab on main window and PrivateSend tray option * adjust code formating * revert QT TODO * Update src/qt/forms/sendcoinsdialog.ui code formating Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com> * code formating update * clear send dialog and remove unnecessary warning * change clear location for better UX and performance * remove variable used in warning * Do not clear everything, simply unselect non-fully-mixes coins instead Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> * Warn about unselected coins Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> * Disable PrivateSend tab/menu item when PrivateSend is disabled Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> * fix tab selection bold font * fix use available balance for PS * change back line Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

UdjinM6 reviewed Apr 22, 2020

View reviewed changes

src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved

UdjinM6 requested changes Apr 23, 2020

View reviewed changes

src/qt/forms/sendcoinsdialog.ui Outdated Show resolved Hide resolved

UdjinM6 previously approved these changes Apr 24, 2020

View reviewed changes

PastaPastaPasta requested changes Apr 25, 2020

View reviewed changes

10xcryptodev dismissed UdjinM6’s stale review via 5d33312 April 27, 2020 00:35

10xcryptodev force-pushed the pr_fix_privatesend_location branch from e512189 to 37b7445 Compare May 2, 2020 01:48

UdjinM6 previously approved these changes May 10, 2020

View reviewed changes

UdjinM6 requested a review from PastaPastaPasta May 11, 2020 12:29

PastaPastaPasta linked an issue May 11, 2020 that may be closed by this pull request

0.15.0.0-rc1 UI/UX Send tab move Privatesend checkbox #3241

Closed

10xcryptodev and others added 16 commits May 14, 2020 16:39

include resource location back

9fbe850

change back balance and privatesend checkbox position

3d6f869

add new PrivateSend tab on main window and PrivateSend tray option

2cba4e9

adjust code formating

8d4db11

revert QT TODO

7005db2

Update src/qt/forms/sendcoinsdialog.ui code formating

dc142cf

Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>

code formating update

971ea84

clear send dialog and remove unnecessary warning

e6ad15e

change clear location for better UX and performance

04f947e

remove variable used in warning

2f10f71

Do not clear everything, simply unselect non-fully-mixes coins instead

9030eb2

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

Warn about unselected coins

be5a977

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

Disable PrivateSend tab/menu item when PrivateSend is disabled

23e7681

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

fix tab selection bold font

6c884ea

fix use available balance for PS

51abf66

change back line

b373b33

10xcryptodev dismissed PastaPastaPasta’s stale review via b373b33 May 14, 2020 19:46

10xcryptodev force-pushed the pr_fix_privatesend_location branch from 791908e to b373b33 Compare May 14, 2020 19:46

UdjinM6 added this to the 16 milestone May 14, 2020

PastaPastaPasta approved these changes May 14, 2020

View reviewed changes

UdjinM6 approved these changes May 14, 2020

View reviewed changes

UdjinM6 changed the title ~~#3241 UX/UI - change PrivateSend and balance location~~ #3241 UX/UI - Introduce PrivateSend tab which allows to spend fully mixed coins only May 14, 2020

UdjinM6 changed the title ~~#3241 UX/UI - Introduce PrivateSend tab which allows to spend fully mixed coins only~~ Fix #3241 UX/UI - Introduce PrivateSend tab which allows to spend fully mixed coins only May 15, 2020

UdjinM6 merged commit 4c1f65b into dashpay:develop May 15, 2020

PastaPastaPasta mentioned this pull request Jun 28, 2020

0.15.0.0-rc1 UI/UX Send tab move Privatesend checkbox #3241

Closed

Fix #3241 UX/UI - Introduce PrivateSend tab which allows to spend fully mixed coins only #3442

Fix #3241 UX/UI - Introduce PrivateSend tab which allows to spend fully mixed coins only #3442

Uh oh!

Conversation

10xcryptodev commented Apr 19, 2020

Uh oh!

UdjinM6 commented Apr 19, 2020

Uh oh!

10xcryptodev commented Apr 19, 2020

Uh oh!

UdjinM6 commented Apr 20, 2020

Uh oh!

10xcryptodev commented Apr 21, 2020

Uh oh!

UdjinM6 commented Apr 21, 2020

Uh oh!

10xcryptodev commented Apr 22, 2020

Uh oh!

UdjinM6 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented Apr 25, 2020

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

10xcryptodev commented Apr 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Apr 27, 2020

Uh oh!

UdjinM6 commented Apr 27, 2020

Uh oh!

10xcryptodev commented Apr 27, 2020

Uh oh!

10xcryptodev commented Apr 29, 2020

Uh oh!

UdjinM6 commented Apr 29, 2020

Uh oh!

10xcryptodev commented May 1, 2020

Uh oh!

PastaPastaPasta commented May 1, 2020

Uh oh!

10xcryptodev commented May 2, 2020

Uh oh!

UdjinM6 commented May 2, 2020

Uh oh!

10xcryptodev commented May 3, 2020

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta commented May 11, 2020

Uh oh!

PastaPastaPasta commented May 13, 2020

Uh oh!

PastaPastaPasta commented May 13, 2020

Uh oh!

10xcryptodev commented May 14, 2020

Uh oh!

PastaPastaPasta commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment •

edited

Loading

10xcryptodev commented Apr 27, 2020 •

edited

Loading

PastaPastaPasta commented May 14, 2020 •

edited

Loading