Skip to content

qt: Fix --disable-wallet build and --disablewallet mode#3762

Merged
UdjinM6 merged 8 commits into
dashpay:developfrom
UdjinM6:qtfixes
Oct 28, 2020
Merged

qt: Fix --disable-wallet build and --disablewallet mode#3762
UdjinM6 merged 8 commits into
dashpay:developfrom
UdjinM6:qtfixes

Conversation

@UdjinM6
Copy link
Copy Markdown

@UdjinM6 UdjinM6 commented Oct 5, 2020

Console is not showing up in --disable-wallet build and dash-qt is crashing in --disablewallet mode currently. This PR should fix both issues.

@UdjinM6 UdjinM6 added this to the 16 milestone Oct 5, 2020
@UdjinM6 UdjinM6 mentioned this pull request Oct 5, 2020
Merged
PastaPastaPasta
PastaPastaPasta reviewed Oct 6, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Tested running a "--disable-wallet" build, seems to work as expected, however, note, the dark mode isn't applied properly to it. Also tested running qt with "--disablewallet", and worked in the same way as the disable-wallet build. See one comment.

Comment thread src/qt/bitcoingui.cpp Outdated
@xdustinface
Copy link
Copy Markdown

xdustinface commented Oct 9, 2020

Looks like it works as expected now 👍 But i guess instead of setting the parent to nullptr we should just set the proper window flag if there is no wallet which fixes the issue we had when using the widget as central widget of BitcoinGUI, see c0dc7bc. I would prefer to see the early returns if there is no walletModel in a new line and encapsulated with brackets.. but thats just because i don't like the if without brackets that much and i think it would be better to avoid using them in general.

xdustinface
xdustinface previously approved these changes Oct 9, 2020
View reviewed changes
Copy link
Copy Markdown

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

ACK

@PastaPastaPasta
Copy link
Copy Markdown
Member

PastaPastaPasta commented Oct 12, 2020

Do these changes make sense to be done upstream?

PastaPastaPasta
PastaPastaPasta previously approved these changes Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
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.

utACK

@PastaPastaPasta
Copy link
Copy Markdown
Member

PastaPastaPasta commented Oct 12, 2020

Also, this should have the 17 milestone no?

@PastaPastaPasta PastaPastaPasta mentioned this pull request Oct 12, 2020
Merged
@UdjinM6 UdjinM6 modified the milestones: 16, 16.0.2 Oct 12, 2020
@UdjinM6
Copy link
Copy Markdown
Author

UdjinM6 commented Oct 12, 2020 •
edited
Loading

Also, this should have the 17 milestone no?

Should be 16.0.216.1 imo, re-assigned (to all currently open backport candidates).

@UdjinM6 UdjinM6 dismissed stale reviews from PastaPastaPasta and xdustinface via 2ee0168 October 14, 2020 17:31
@UdjinM6
Copy link
Copy Markdown
Author

UdjinM6 commented Oct 14, 2020

Rebased to fix merge conflicts after #3682

xdustinface
xdustinface previously approved these changes Oct 14, 2020
View reviewed changes
Copy link
Copy Markdown

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6
Copy link
Copy Markdown
Author

UdjinM6 commented Oct 16, 2020

Rebased again (because of #3771)

xdustinface
xdustinface approved these changes Oct 21, 2020
View reviewed changes
Copy link
Copy Markdown

@xdustinface xdustinface left a comment

Choose a reason for hiding this comment

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

utACK

PastaPastaPasta
PastaPastaPasta approved these changes Oct 22, 2020
View reviewed changes
Copy link
Copy Markdown
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.

utACK

@UdjinM6 UdjinM6 merged commit 6bb05eb into dashpay:develop Oct 28, 2020
UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Oct 28, 2020
@UdjinM6 @xdustinface
qt: Fix --disable-wallet build and --disablewallet mode (dashpay#3762)
9ceee5d 
* Fix --disable-wallet build and --disablewallet mode

Console is not showing up in --disable-wallet build and it's crashing in --disablewallet mode before this commit.

* Use nullptr

* Load a stylesheet for the RPCConsole when it's not going to be set as a central widget

This partially reverts the initial commit.

* Fix QMainWindow dark/light styles

* Avoid implicit pointer to bool conversions

* qt: Set RPCConsole's window flags based on wallet availability

* multi-line early returns

* Bring back ifdef to fix a recently introduced crash

Introduced in dashpay@2856f46#diff-f63c3d5094d55f88dbd1967774f85838b3aee5a40540b8c82b924574bca772a0R203

Co-authored-by: xdustinface <xdustinfacex@gmail.com>
@UdjinM6 UdjinM6 deleted the qtfixes branch November 26, 2020 13:28
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 10, 2022
@UdjinM6 @xdustinface @gades
qt: Fix --disable-wallet build and --disablewallet mode (dashpay#3762)
c0d8781 
* Fix --disable-wallet build and --disablewallet mode

Console is not showing up in --disable-wallet build and it's crashing in --disablewallet mode before this commit.

* Use nullptr

* Load a stylesheet for the RPCConsole when it's not going to be set as a central widget

This partially reverts the initial commit.

* Fix QMainWindow dark/light styles

* Avoid implicit pointer to bool conversions

* qt: Set RPCConsole's window flags based on wallet availability

* multi-line early returns

* Bring back ifdef to fix a recently introduced crash

Introduced in dashpay@2856f46#diff-f63c3d5094d55f88dbd1967774f85838b3aee5a40540b8c82b924574bca772a0R203

Co-authored-by: xdustinface <xdustinfacex@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@xdustinface xdustinface xdustinface approved these changes

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

16.1

Development

Successfully merging this pull request may close these issues.

3 participants

@UdjinM6 @xdustinface @PastaPastaPasta