Skip to content

Conversation

@xdustinface
Copy link

@xdustinface xdustinface commented Sep 16, 2020

  • Move PrivateSend related settings from Options -> Wallet into a new tab Options -> PrivateSend
  • Add Enable PrivateSend interface in Options ->Wallet. This checkbox allows to dynamically show/hide the PrivateSend related UI elements all over the place

Based on #3716 and #3715

Before

Bildschirmfoto 2020-09-16 um 17 10 45

With this PR

PrivateSend UI hidden PrivateSend options tab
Bildschirmfoto 2020-09-16 um 17 17 54 Bildschirmfoto 2020-09-16 um 17 18 01

@xdustinface xdustinface added this to the 16 milestone Sep 16, 2020
@xdustinface xdustinface force-pushed the pr-ui-privatesend-options branch from bd6e77d to e711ffb Compare September 16, 2020 16:35
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

Pls see suggestions below

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

Pls see below + there is a bug somewhere but I can't figure it out yet. The issue is like that: disable ps features in options, close and reopen the wallet (no ps ui as expected), enable ps in options -> still no ps ui on overview screen, ps tab and tx filters do appear though. If you close and reopen the wallet now ps ui would appear like it should.

@xdustinface
Copy link
Author

there is a bug somewhere

Ah right good catch! The PrivateSend UI update timer isn't created/started and the slots aren't connected if the wallet gets started with PrivateSend disabled. Fixing it 👍

https://github.com/dashpay/dash/blob/develop/src/qt/overviewpage.cpp#L310
https://github.com/dashpay/dash/blob/develop/src/qt/overviewpage.cpp#L172

@xdustinface xdustinface force-pushed the pr-ui-privatesend-options branch from cb6338e to f03bce9 Compare September 17, 2020 18:48
@xdustinface
Copy link
Author

xdustinface commented Sep 17, 2020

@UdjinM6 Okaaay, there were even some more things i missed.. like two signal/slot connections weren't correct and whatnot.. see commits starting with 506614d.

Now it instantly updates all UI elements if the checkbox gets toggled.. did only update the "PrivateSend" button of the options instantly before and then only all over the place after you pressed "Ok" in the dialog. It also recovers the previous state properly now if you reject the dialog.

And i included #3715 into 4cdd3ca

@PastaPastaPasta
Copy link
Member

Why?? We just removed a basically pointless tab, since it made sense to have it under another tab, and yet now we're going to add another "PrivateSend" tab that makes total sense to be under the Wallet tab??

@PastaPastaPasta PastaPastaPasta self-requested a review September 18, 2020 03:45
@UdjinM6
Copy link

UdjinM6 commented Sep 18, 2020

Why?? We just removed a basically pointless tab, since it made sense to have it under another tab, and yet now we're going to add another "PrivateSend" tab that makes total sense to be under the Wallet tab??

I would argue that Wallet tab has too many options already (7 checkboxes and 2 edit fields) and most of them are PS only, adding yet another one "Enable PrivateSend features" would make it look pretty clogged and weird imo. The new Wallet tab has 4 checkboxes and the new PrivateSend tab has 4 checkboxes and 2 edit fields, the later is exactly as for the new Main tab after #3709 on non-mac machines (on mac it's 1 checkbox and 2 edit fields only). If we ever think about exposing more PS options in GUI (and I think it would actually make sense to do) e.g. denoms goals/limits or number of mixing sessions etc. then having a separate tab is the only way to go imo.

@PastaPastaPasta
Copy link
Member

concept ACK on splitting settings out from inside of wallet. however privatesend must be enabled by default

@UdjinM6
Copy link

UdjinM6 commented Sep 25, 2020

Needs rebase.

@xdustinface xdustinface force-pushed the pr-ui-privatesend-options branch from f03bce9 to bf0c95d Compare September 25, 2020 16:51
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

cropped names
Adding the tab resulted in some not great cropping at my maybe default gui settings

@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Sep 25, 2020

Starting with default settings made it way worse...
default

EDIT: It's probably fine to merge as long as we open an issue about this

Copy link
Member

Choose a reason for hiding this comment

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

This tooltip needs to wrap onto multiple lines

@PastaPastaPasta
Copy link
Member

Also, noenableprivatesend gets viewed by gui as enableprivatesend=0 so we probably don't need that change specific to noenableprivatesend

@xdustinface xdustinface force-pushed the pr-ui-privatesend-options branch from 680e85b to 5ceb59b Compare September 25, 2020 19:01
@xdustinface
Copy link
Author

Also, noenableprivatesend gets viewed by gui as enableprivatesend=0 so we probably don't need that change specific to noenableprivatesend

Reverted and added a linebreak to the tooltip, should be good enough in two lines?

xdustinface and others added 2 commits September 25, 2020 21:53
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
@UdjinM6
Copy link

UdjinM6 commented Sep 25, 2020

Re cropping: an easy fix would be to bump https://github.com/dashpay/dash/blob/develop/src/qt/res/css/general.css#L1257 - 635px worked ok-ish in my case but might need to bump it even more for Windows, smth like 700px or so I guess oh, wait, there is no Window tab anymore 🙈 .

@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Sep 25, 2020

Re cropping: an easy fix would be to bump https://github.com/dashpay/dash/blob/develop/src/qt/res/css/general.css#L1257 - 635px worked ok-ish in my case but might need to bump it even more for Windows, smth like 700px or so I guess.

I had to go all the way up to 710px in order to make it work on ubuntu with default gui settings

EDIT: I think this would probably be the easiest fix for now, we can work on making it properly dynamic later

@UdjinM6
Copy link

UdjinM6 commented Sep 26, 2020

ESC key no longer cancels the changes that were made to ps checkbox, should do smth like 5ea108b88f

Handle all reject reasons not only the cancle button.
@xdustinface
Copy link
Author

@UdjinM6 good catch 👍 Added 06cdc89 instead which also takes care of other reject reasons like clicking the window's "x" close button.

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

ACK

(assuming font issues will be solved via #3734 )

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

ACK

@UdjinM6 UdjinM6 merged commit 9a9e21c into dashpay:develop Sep 28, 2020
xdustinface added a commit to xdustinface/dash that referenced this pull request Sep 28, 2020
…ashpay#3717)

* qt: Add PrivateSend tab in OptionsDialog, allow to show/hide PS UI

* qt: Decrease height of OptionsDialog

* Apply suggestions from code review


* qt: Remove obsolete visibility adjustments

Not longer needed since the page is just not reachable if the button is
hidden.

* qt: Make sure PrivateSend related parts are always initialized properly

Not only if its enabled on startup..

* qt: Make updatePrivateSendVisibility a slot to fix the signal connection

* qt: Fix UI updates on OverviewPage if PrivateSend enabled gets toggled

Other way of connecting the slot with true as parameter didn't work..

* qt: Only update and emit the signal for advanced PS UI if required

* qt: Update fPrivateSendEnabled in OptionsModel instead of OptionsDialog

* qt: Recover the PrivateSend enabled state if OptionsDialog gets rejected

* qt: Enable PrivateSend UI by default

* qt: Add some brackets

* qt: Add a comment

* qt: Add a linebreak to the "Enable PrivateSend features" tooltip

* qt: Remove obsolete comment


* qt: Move comment

* qt: Properly reset the previous PS state if OptionsDialog gets rejected

Handle all reject reasons not only the cancle button.
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 8, 2022
…ashpay#3717)

* qt: Add PrivateSend tab in OptionsDialog, allow to show/hide PS UI

* qt: Decrease height of OptionsDialog

* Apply suggestions from code review

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

* qt: Remove obsolete visibility adjustments

Not longer needed since the page is just not reachable if the button is
hidden.

* qt: Make sure PrivateSend related parts are always initialized properly

Not only if its enabled on startup..

* qt: Make updatePrivateSendVisibility a slot to fix the signal connection

* qt: Fix UI updates on OverviewPage if PrivateSend enabled gets toggled

Other way of connecting the slot with true as parameter didn't work..

* qt: Only update and emit the signal for advanced PS UI if required

* qt: Update fPrivateSendEnabled in OptionsModel instead of OptionsDialog

* qt: Recover the PrivateSend enabled state if OptionsDialog gets rejected

* qt: Enable PrivateSend UI by default

* qt: Add some brackets

* qt: Add a comment

* qt: Add a linebreak to the "Enable PrivateSend features" tooltip

* qt: Remove obsolete comment

Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>

* qt: Move comment

* qt: Properly reset the previous PS state if OptionsDialog gets rejected

Handle all reject reasons not only the cancle button.

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
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.

3 participants