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

qt: Add warning messages to the debug window #14879

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@hebasto
Copy link
Member

commented Dec 5, 2018

Fix: #11016

This PR adds warning messages to the debug window in -disablewallet mode.

screenshot from 2018-12-06 01-01-27

@fanquake fanquake added the GUI label Dec 5, 2018

@promag
Copy link
Member

left a comment

An alternative approach is to extract the existing alert from the OverviewPage and add that to the QMainWindow. In addition you could try to use addDockWidget(Qt::TopDockWidgetArea, alert_widget).

Anyway, my point is to avoid duplicate code.

src/qt/rpcconsole.cpp Outdated
@@ -9,25 +9,24 @@
#include <qt/rpcconsole.h>
#include <qt/forms/ui_debugwindow.h>

#include <qt/bantablemodel.h>

This comment has been minimized.

Copy link
@promag

promag Dec 5, 2018

Member

No need to touch the headers.

This comment has been minimized.

Copy link
@hebasto

hebasto Dec 16, 2018

Author Member

Agree.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@promag Thank you for your review.

An alternative approach is to extract the existing alert from the OverviewPage and add that to the QMainWindow. In addition you could try to use addDockWidget(Qt::TopDockWidgetArea, alert_widget).

That was my first thought exactly :)
In that case warning messages will be exposed not only on Overview tab but on Send, Receive and Transactions tabs too. Can such behaviour be annoying for a user?

No need to touch the headers.

I've just moved #include <qt/walletmodel.h> under the #ifdef ENABLE_WALLET guard as all rpcconsole.cpp code supposed. All others headers moves were made by clang-format-diff.py. Let code formatting approach to an ideal step by step :)

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

@hebasto hebasto force-pushed the hebasto:20181205-debugwindow-warnings branch Dec 6, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

Can such behaviour be annoying for a user?

IMO it'd be preferable

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

Concept ACK

@hebasto hebasto force-pushed the hebasto:20181205-debugwindow-warnings branch Dec 16, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Dec 16, 2018

Rebased.

@DrahtBot DrahtBot removed the Needs rebase label Dec 16, 2018

@hebasto hebasto force-pushed the hebasto:20181205-debugwindow-warnings branch to e4fb571 Dec 17, 2018

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

Rebased on top of 3e21b69.

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Jan 1, 2019

@promag

... you could try to use addDockWidget(Qt::TopDockWidgetArea, alert_widget).

By design, a QDockWidget consists of a title bar and the content area. An alert label can be placed to any of these elements of alert_widget. The other element (without the alert label) cannot be hidden, unfortunately. In addition, such implementation should handle resize events properly that adds some new code and possibly introduces new GUI-related bugs.

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

@Sjors would you mind reviewing this PR?

@Sjors

Sjors approved these changes Jan 25, 2019

Copy link
Member

left a comment

tACK e4fb571

I don't mind the slight duplication of view code.

In that case warning messages will be exposed not only on Overview tab but on Send, Receive and Transactions tabs too. Can such behaviour be annoying for a user?

The Overview tab is the first thing you see when you launch Bitcoin Core, so I don't see the point in showing warning in other places. That's especially true the first time you launch the app, so everyone will have seen that it's not a release build.

The only exception I can think of is when you open a BIP21/70 URI, so maybe some really critical warnings could also be on the send screen, but currently the warnings, e.g. "new version bit" aren't informative enough for that to be useful.

@@ -563,6 +564,17 @@ bool RPCConsole::eventFilter(QObject* obj, QEvent *event)
void RPCConsole::setClientModel(ClientModel *model)
{
clientModel = model;

auto wallet_enabled = false;

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 25, 2019

Member

nit: bool

wallet_enabled = WalletModel::isWalletEnabled();
#endif // ENABLE_WALLET
if (model && !wallet_enabled) {
// Show warning if this is a prerelease version

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 25, 2019

Member

Should be: // Show warning, for example if

@hebasto hebasto force-pushed the hebasto:20181205-debugwindow-warnings branch from e4fb571 to c0c9613 Jan 25, 2019

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

@Sjors
Thank you for your review. All your comments have been addressed.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

re-tACK c0c9613 assuming Travis is happy (I just restarted two instances, they seem to have been confused by cached code, I noticed that locally too).

@hebasto hebasto force-pushed the hebasto:20181205-debugwindow-warnings branch from c0c9613 to 593ba69 Jan 25, 2019

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2019

@Sjors Travis is happy now :)

After merging #15101 the <qt/walletmodel.h> header gets required even with --disable-wallet.

@hebasto

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2019

@fanquake would you mind reviewing this PR?

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

utACK 593ba69

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.