qt: Finetune CoinControlDialog + bitcoin#14828 #3701

xdustinface · 2020-09-11T16:36:57Z

The rows resize without it if they get locked and the lock icon appears besides the checkbox. Looks weird.. and especially if you press the lock all button its just not nice.

Update

The scope has been expanded during review, see #3701 (comment) for the final result.

The rows resize without it if they get locked and the lock icon appears besides the checkbox. Looks weird.. and especially if you press the lock all button its just not nice.

UdjinM6 · 2020-09-12T02:24:27Z

I can't reproduce the issue...

xdustinface · 2020-09-12T07:25:09Z

Hmm can you send screens? Does it make anything look worse over there?

Here is how it looks on my mac (macOS 10.15.6)

Unlocked	Locked

And here besides the "lock resizes the items" thing linux needs min-height anyway otherwise it looks squished imo.

Linux without min-height	Linux with min-height

Also on windows it doesn't hurt..

Windows without min-height	Windows with min-height and hover transparency

Not sure how this stuff looks on other systems/resolutions but here in my VMs this improves things..

UdjinM6 · 2020-09-12T14:31:40Z

I don't know, for me it just works, nothing jumps or shifts anywhere. With this patch it looks ok-ish but there is too much space between rows now imo. Lets maybe add some lines there e.g. aa48d26 (this also unifies the look with other tables as a side effect :) )?

UdjinM6 · 2020-09-13T11:02:33Z

Also, how about applying 9f02792 while at it? This one is from btc 0.18 and had a couple of (trivial-ish) merge conflicts, so (if @PastaPastaPasta would complain :P) we could simply reduce this https://github.com/dashpay/dash/blob/develop/src/qt/forms/coincontroldialog.ui#L379 by 2 instead for now. And then apply smth like 8cf9965 maybe.

Mode	This PR currently	With 3 more patches on top
List
Tree

(Note: it's not obvious from screenshots but with this new column count/sizes there will be no useless horizontal scrolling anymore)

xdustinface · 2020-09-13T12:26:54Z

@UdjinM6 I won't be able to test it until late evening here my time, but in the meantime, from looking at the screenshots i see some things we should probably still tweak

The lines are not fully drawn over the full width.. looks a bit strange to me right now but im also not sure how good it will look like in the tree mode with fully drawn lines. I think that was even one reason why i initially removed the lines in the CoinControl :D In case you want to try it i think you can extend them with ::branch.
The last column in your patched screenshots are not stretched so there is a gap between the column end and the border of the widget. I think it should still be possible to stretch them over the full width without getting a horizontal scroll, no?

I would be fine with adding the bitcoin backport here btw 👍 lets see what pasta has to say.

1c28feb qt: Remove hidden columns in coin control dialog (João Barbosa) Pull request description: Instead of having hidden columns, store the data in specific roles. Overlaps with bitcoin#14817, fixes bitcoin#11811. Tree-SHA512: e86e9ca426b9146ac28997ca1920dbae6cc4e2e494ff94fe131d605cd6c013183fc5de10036c886a4d6dcae497ac4067de3791be0ef9c88f7ce9f57f7bd97422

…llow to show them.

xdustinface · 2020-09-16T22:22:57Z

@UdjinM6 i added some more commits with the following impacts:

69bed8d adds the border like you proposed.. didn't add the branch borders i talked about, even though it somehow looks "unfinal" without them it still looks better compared to when the lines are over the full width :D
55d6d64 streches the address column to have the tree stretched over the full width, solves the issue with the last column i pointed out in your last suggestions
7327ed1
- Hides Denominated/Collateral utxos for normal Send tab's CoinControl by default (Still possible to show them with a new button)
- Hides not fully mixed utxos in PrivateSend tabs's CoinControl by default (Still possible to show them with a new button, helps to see whats going on with the mixing process)
f0fb60f Removes the mixing rounds from the normal Send tab's CoinControl as it doesn't make a lot sense there imo
fcdf46b Removes the selection between list/tree mode for PrivateSend CoinControl as we anyway will always have only 1 utxo per address it makes no sense to offer tree view, right? Correct me if there are cases where it would make sense to have a tree view!
9272da7 Hides the label column in the PrivateSend CoinControl, didn't see the point in leaving it there, complain if there are reasons..
631d08b I guess it makes sense now to rename it as we won some space and we can have a longer string for it.
f2e2d70 looks like something the guys from the bitcoin PR missed?

Preview

Mode	Before	With this PR
Send Tree
Send List
PrivateSend

UdjinM6 · 2020-09-17T11:34:51Z

That's an interesting idea 👍 How about 2bf641f on top of it?

… ones with rounds>=1 Also tweak button text

UdjinM6

ACK

And make it default to show all coins in that case..

UdjinM6

re-ACK

PastaPastaPasta · 2020-09-18T02:30:36Z

Initial thoughts (might get edited as I find more stuff I don't like :D ) :
PrivateSend tab inputs should default to show all privatesend coins, not default to only fully mixed coins
Also, please edit the initial post that gives the high level of all the changes this PR includes

xdustinface · 2020-09-18T02:51:12Z

PrivateSend tab inputs should default to show all privatesend coins, not default to only fully mixed coins

To be honest, i don't think that this makes a lot sense. Why do you think that? The purpose of this dialog is to pick coins for sending, means there is actually no need to show unrelated (non-spendable) entries by default it's just a nice to have IMO.

Also, please edit the initial post that gives the high level of all the changes this PR includes

Oh will do 👍 didn't realize that the scope of this PR has become that big compared to what the initial post says 😄

PastaPastaPasta · 2020-09-18T02:53:10Z

PrivateSend tab inputs should default to show all privatesend coins, not default to only fully mixed coins

To be honest, i don't think that this makes a lot sense. Why do you think that? The purpose of this dialog is to pick coins for sending, means there is actually no need to show unrelated (non-spendable) entries by default it's just a nice to have IMO.

If we don't do that, I think people are going to "lose" coins after they get denominated but not fully mixed, since they won't be able to see those coins in either the Send tab or PrivateSend tab

xdustinface · 2020-09-18T02:58:14Z

PrivateSend tab inputs should default to show all privatesend coins, not default to only fully mixed coins

To be honest, i don't think that this makes a lot sense. Why do you think that? The purpose of this dialog is to pick coins for sending, means there is actually no need to show unrelated (non-spendable) entries by default it's just a nice to have IMO.

If we don't do that, I think people are going to "lose" coins after they get denominated but not fully mixed, since they won't be able to see those coins in either the Send tab or PrivateSend tab

Well.. the button says Show all PrivateSend coins so i guess the moment of "lose" coins will be very short, no? :D

PastaPastaPasta · 2020-09-18T03:01:12Z

PrivateSend tab inputs should default to show all privatesend coins, not default to only fully mixed coins

To be honest, i don't think that this makes a lot sense. Why do you think that? The purpose of this dialog is to pick coins for sending, means there is actually no need to show unrelated (non-spendable) entries by default it's just a nice to have IMO.

If we don't do that, I think people are going to "lose" coins after they get denominated but not fully mixed, since they won't be able to see those coins in either the Send tab or PrivateSend tab

Well.. the button says Show all PrivateSend coins so i guess the moment of "lose" coins will be very short, no? :D

IMO, the default between the two tabs, needs to show all coins. If we want the "Send" tab to show all coins that are not fully mixed, then that's fine, but I'd prefer just changing the default on privatesend tab to be showing all, if someone wants to see just mixed coins, it's just a click away

xdustinface · 2020-09-18T03:05:06Z

if someone wants to see just mixed coins, it's just a click away

Same applies vice versa, not?

And still, the main purpose of the dialog is "Pick coins to Send" not "Look what your coins are doing" so i guess it will happen more often that you need to do that one click if they are shown by default because the not fully mixed just distract when choosing coins.

PastaPastaPasta · 2020-09-18T03:15:48Z

if someone wants to see just mixed coins, it's just a click away

Same applies vice versa, not?

And still, the main purpose of the dialog is "Pick coins to Send" not "Look what your coins are doing" so i guess it will happen more often that you need to do that one click if they are shown by default because the not fully mixed just distract when choosing coins.

Maybe it's just my usage, but I probably use it more often as a way to see the status of my outputs then choosing the outputs to spend

UdjinM6 · 2020-09-18T09:56:19Z

Agree with @xdustinface on this, the purpose of this dialog is to list coins that are safe to spend and not coins that are (potentially) yours. When IS is disabled you won't see 0-conf coins received from someone else for example (same as in btc). With this in mind it makes perfect sense imo to make it harder for advanced users to mess things up (spending "fully mixed" coins in non-PS tx creates new change output which will become a link between this tx and some another tx spending it) and makes it easier to pick coins that are actually ready to spend (by hiding/disabling coins that are PS elated but aren't spendable via PS yet/at all).

PastaPastaPasta · 2020-09-19T03:29:06Z

Agree with @xdustinface on this, the purpose of this dialog is to list coins that are safe to spend and not coins that are (potentially) yours. When IS is disabled you won't see 0-conf coins received from someone else for example (same as in btc). With this in mind it makes perfect sense imo to make it harder for advanced users to mess things up (spending "fully mixed" coins in non-PS tx creates new change output which will become a link between this tx and some another tx spending it) and makes it easier to pick coins that are actually ready to spend (by hiding/disabling coins that are PS elated but aren't spendable via PS yet/at all).

Okay, I'd prefer at least in the long term to make it even harder spending mixed funds in a not PrivateSend tx.

@xdustinface please update pr description and I'll try to give it a full review tomorrow

xdustinface · 2020-09-19T16:48:12Z

@xdustinface please update pr description

Done

PastaPastaPasta

ACK, all working as expected. mild code review

* qt: Add min-height for CoinControlTreeWidget#treeWidget::item The rows resize without it if they get locked and the lock icon appears besides the checkbox. Looks weird.. and especially if you press the lock all button its just not nice. * qt: Set background transparency for CoinControl item::hover * Merge bitcoin#14828: qt: Remove hidden columns in coin control dialog 1c28feb qt: Remove hidden columns in coin control dialog (João Barbosa) Pull request description: Instead of having hidden columns, store the data in specific roles. Overlaps with bitcoin#14817, fixes bitcoin#11811. Tree-SHA512: e86e9ca426b9146ac28997ca1920dbae6cc4e2e494ff94fe131d605cd6c013183fc5de10036c886a4d6dcae497ac4067de3791be0ef9c88f7ce9f57f7bd97422 * qt: Add border-bottom for tree items in CoinControl * qt: Stretch address column in CoinControlDialog * Adjust column width for a couple of columns * qt: Hide PrivateSend rounds column for normal Send tab's CoinControl * qt: Hide unrelated coins in CoinControl based on active mode. Still allow to show them. * qt: Hide empty top level items in CoinControlDialog's tree mode * qt: Hide tree/list radio buttons and default to list for PrivateSend * qt: Hide address/label column in CoinControl for PrivateSend * qt: Remove obsolete empty columns * qt: Rename column "PS Rounds" to "Mixing Rounds" * qt: Move border-bottom in already existing css selector * Reveal all PS related coins in coincontrol while in PS mode, not only ones with rounds>=1 Also tweak button text * qt: Only moving a statement a bit * qt: Hide the "hideButton" in CoinControlDialog if PrivatSend is disabled And make it default to show all coins in that case.. Co-authored-by: Jonas Schnelli <dev@jonasschnelli.ch> Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

qt: Add min-height for CoinControlTreeWidget#treeWidget::item

0c394e3

The rows resize without it if they get locked and the lock icon appears besides the checkbox. Looks weird.. and especially if you press the lock all button its just not nice.

xdustinface added this to the 16 milestone Sep 11, 2020

xdustinface added the backport-candidate-16.x label Sep 11, 2020

qt: Set background transparency for CoinControl item::hover

142906d

xdustinface changed the title ~~qt: Add min-height for CoinControlTreeWidget#treeWidget::item~~ qt: Finetune CoinControlDialog Sep 12, 2020

jonasschnelli and others added 11 commits September 16, 2020 22:04

qt: Add border-bottom for tree items in CoinControl

69bed8d

qt: Stretch address column in CoinControlDialog

55d6d64

Adjust column width for a couple of columns

1ea1435

qt: Hide PrivateSend rounds column for normal Send tab's CoinControl

f0fb60f

qt: Hide unrelated coins in CoinControl based on active mode. Still a…

7327ed1

…llow to show them.

qt: Hide empty top level items in CoinControlDialog's tree mode

38144e3

qt: Hide tree/list radio buttons and default to list for PrivateSend

fcdf46b

qt: Hide address/label column in CoinControl for PrivateSend

9272da7

qt: Remove obsolete empty columns

f2e2d70

qt: Rename column "PS Rounds" to "Mixing Rounds"

631d08b

xdustinface changed the title ~~qt: Finetune CoinControlDialog~~ qt: Finetune CoinControlDialog + bitcoin#14828 Sep 16, 2020

qt: Move border-bottom in already existing css selector

fdd2b67

Reveal all PS related coins in coincontrol while in PS mode, not only…

50f6740

… ones with rounds>=1 Also tweak button text

UdjinM6 previously approved these changes Sep 17, 2020

View reviewed changes

xdustinface added 2 commits September 17, 2020 21:06

qt: Only moving a statement a bit

bc01633

qt: Hide the "hideButton" in CoinControlDialog if PrivatSend is disabled

933373c

And make it default to show all coins in that case..

xdustinface dismissed UdjinM6’s stale review via 933373c September 17, 2020 19:11

UdjinM6 approved these changes Sep 17, 2020

View reviewed changes

PastaPastaPasta self-requested a review September 18, 2020 02:37

PastaPastaPasta approved these changes Sep 23, 2020

View reviewed changes

UdjinM6 merged commit b10dc1f into dashpay:develop Sep 23, 2020

PastaPastaPasta mentioned this pull request Sep 24, 2020

[v0.16.x] V16 more backports #3732

Merged

PastaPastaPasta removed the backport-candidate-16.x label Sep 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qt: Finetune CoinControlDialog + bitcoin#14828 #3701

qt: Finetune CoinControlDialog + bitcoin#14828 #3701

xdustinface commented Sep 11, 2020 •

edited

UdjinM6 commented Sep 12, 2020

xdustinface commented Sep 12, 2020 •

edited

UdjinM6 commented Sep 12, 2020

UdjinM6 commented Sep 13, 2020 •

edited

xdustinface commented Sep 13, 2020

xdustinface commented Sep 16, 2020

UdjinM6 commented Sep 17, 2020

UdjinM6 left a comment

UdjinM6 left a comment

PastaPastaPasta commented Sep 18, 2020 •

edited

xdustinface commented Sep 18, 2020

PastaPastaPasta commented Sep 18, 2020

xdustinface commented Sep 18, 2020

PastaPastaPasta commented Sep 18, 2020

xdustinface commented Sep 18, 2020 •

edited

PastaPastaPasta commented Sep 18, 2020

UdjinM6 commented Sep 18, 2020

PastaPastaPasta commented Sep 19, 2020

xdustinface commented Sep 19, 2020

PastaPastaPasta left a comment

qt: Finetune CoinControlDialog + bitcoin#14828 #3701

qt: Finetune CoinControlDialog + bitcoin#14828 #3701

Conversation

xdustinface commented Sep 11, 2020 • edited

Update

UdjinM6 commented Sep 12, 2020

xdustinface commented Sep 12, 2020 • edited

UdjinM6 commented Sep 12, 2020

UdjinM6 commented Sep 13, 2020 • edited

xdustinface commented Sep 13, 2020

xdustinface commented Sep 16, 2020

Preview

UdjinM6 commented Sep 17, 2020

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Sep 18, 2020 • edited

xdustinface commented Sep 18, 2020

PastaPastaPasta commented Sep 18, 2020

xdustinface commented Sep 18, 2020

PastaPastaPasta commented Sep 18, 2020

xdustinface commented Sep 18, 2020 • edited

PastaPastaPasta commented Sep 18, 2020

UdjinM6 commented Sep 18, 2020

PastaPastaPasta commented Sep 19, 2020

xdustinface commented Sep 19, 2020

PastaPastaPasta left a comment

Choose a reason for hiding this comment

xdustinface commented Sep 11, 2020 •

edited

xdustinface commented Sep 12, 2020 •

edited

UdjinM6 commented Sep 13, 2020 •

edited

PastaPastaPasta commented Sep 18, 2020 •

edited

xdustinface commented Sep 18, 2020 •

edited