GUI: merge sign/verify message into a single window with tabbed UI #1469

Diapolo commented at 8:08 AM on June 15, 2012: none

This pull merges the currently seperate dialogs for signing and verifying a message into a single tabbed dialog.

(new) Features:

add UI-feedback via QValidatedLineEdit
copy button for generated signature was moved to the signature output field
add an addressbook button to verify message tab
input fields are now evenly ordered for sign and verify tabs
update FIRST_CLASS_MESSAGING support to ensure a good UX
add a button and context menu entry in addressbook for verify message (to be consistent with sign message)
focus is now only set/changed, when clearing input fields or adding an address via addressbook
re-work / update some strings
ensure model gets initialized in the SignVerifyMessageDialog constructor
add checks for a valid model to both addressbook buttons
remove unneeded includes for Qt GUI elements that are listed in ui_signverifymessagedialog.h anyway

Todo:

generalize some functions, as some use nearly identical code for sign / verify

See: tabbed sign- / verifymessage window

luke-jr commented at 7:27 PM on June 15, 2012: member

WARNING: Failure to find: src/qt/messagepage.cpp WARNING: Failure to find: src/qt/verifymessagedialog.cpp WARNING: Failure to find: src/qt/messagepage.h WARNING: Failure to find: src/qt/verifymessagedialog.h WARNING: Failure to find: src/qt/forms/messagepage.ui WARNING: Failure to find: src/qt/forms/verifymessagedialog.ui ...

make[1]: *** No rule to make target src/qt/forms/messagepage.ui', needed bybuild/ui_messagepage.h'. Stop.

Diapolo commented at 7:35 PM on June 15, 2012: none

I need to run mingw32-make.exe clean and re-run qmake for this to work, but it compiles fine afterwards. Damn, sorry I guess I forgot to attach the updated .pro file ... just a sec.

Diapolo commented at 7:39 PM on June 15, 2012: none

@luke-jr Can you re-test now :)?

in bitcoin-qt.pro:None in ac4f587d7b outdated

  99 | @@ -100,7 +100,7 @@ HEADERS += src/qt/bitcoingui.h \
 100 |      src/qt/optionsdialog.h \
 101 |      src/qt/sendcoinsdialog.h \
 102 |      src/qt/addressbookpage.h \
 103 | -    src/qt/messagepage.h \
 104 | +    src/qt/signverifymessagedialog.h \

luke-jr commented at 8:53 PM on June 15, 2012:

I think messagepage is still a better name for this. :p

Diapolo commented at 9:08 PM on June 15, 2012:

It is possible to rename this, sure, so I'll count your +1 and will wait for other comments :).

luke-jr commented at 9:28 PM on June 15, 2012:

Renaming it will let GitHub show the changes properly too ;)

Diapolo commented at 9:29 PM on June 15, 2012:

That's one thing I didn't want, as I find it very hard to read if you change that much.

laanwj commented at 9:23 AM on June 30, 2012:

Even though there are a lot, at least it makes it possible to see the changes. Now it has to be compared manually.... that's not better than letting github do an half-assed job first

Diapolo commented at 10:28 AM on June 30, 2012:

Sorry guys for the inconvenience, how could I have just renamed the file without replacing it?

in src/qt/bitcoingui.cpp:None in ac4f587d7b outdated

 223 | +    tabGroup->addAction(signMessageAction);
 224 | +
 225 | +    verifyMessageAction = new QAction(QIcon(":/icons/transaction_0"), tr("&Verify message..."), this);
 226 | +    verifyMessageAction->setToolTip(tr("Verify a message to ensure it was signed with a specified Bitcoin address"));
 227 | +#ifdef FIRST_CLASS_MESSAGING
 228 | +    verifyMessageAction->setCheckable(true);

luke-jr commented at 8:55 PM on June 15, 2012:

I'm not sure Sign/Verify need their own tabs. How about just a single "Messaging" tab?

Diapolo commented at 9:11 PM on June 15, 2012:

As I have no clue what FIRST_CLASS_MESSAGING is or does for me you are free to assist me in fixing this ;). You could start by posting a screenshot how that dialog looks with it ^^.

luke-jr commented at 9:26 PM on June 15, 2012:

qmake FIRST_CLASS_MESSAGING=1

Diapolo commented at 9:28 PM on June 15, 2012:

This works on Windows?

luke-jr commented at 9:29 PM on June 15, 2012:

Don't know why it wouldn't...

in src/qt/bitcoingui.cpp:None in ac4f587d7b outdated

 749 | +#ifdef FIRST_CLASS_MESSAGING
 750 | +    verifyMessageAction->setChecked(true);
 751 | +    centralWidget->setCurrentWidget(signVerifyMessageDialog);
 752 | +
 753 | +    exportAction->setEnabled(false);
 754 | +    disconnect(exportAction, SIGNAL(triggered()), 0, 0);

luke-jr commented at 8:56 PM on June 15, 2012:

Whatever this is supposed to do, I don't think it works. Clicking the "Verify Message" tab gets me to another set of tabs (Sign and Verify) with Sign selected...

Diapolo commented at 9:12 PM on June 15, 2012:

As written above FIRST_CLASS_MESSAGING is not used for me and so I only duped what was there ...

Diapolo commented at 12:11 AM on June 16, 2012: none

@luke-jr Can you please try again and report back :), thanks.

luke-jr commented at 12:34 AM on June 16, 2012: member

I found the Verify Message pane confusing because you set the focus when switching to it, which clears the placeholder text. Furthermore, some systems don't support the placeholder text. So it's just 3 unlabelled text entries.

I suggest adding an Address Book selector and Paste button to the address textline, and moving the Signature entry below the Message like it is on the Sign Message tab.

Perhaps a warning about messages not implying anything beyond what is written in them is due to the Verify tab.

Diapolo commented at 9:46 AM on June 16, 2012: none

@luke-jr Good suggestions, I'm going to update this.

Diapolo commented at 10:31 AM on June 16, 2012: none

@luke-jr Can you test it once more please.

I did not add a paste button for address field on verify message, as we have one there next to the signature field (which makes sense as we have a copy signature button on sign message).

The warning message thing I did not understand, can you explain for a native non english speaking person :)?

Todo:

add a button / context menu entry in addressbook for verify message to be consistent with sign message
generalize some functions, as some use nearly identical code for sign / verify

Diapolo commented at 5:03 PM on June 16, 2012: none

Updated to now include add a button / context menu entry in addressbook for verify message.

Diapolo commented at 12:54 AM on June 17, 2012: none

Btw. is there any protocol or RPC command limit for the message-length? It seems one can DoS it's client by inputting a very long message. Signing is quick, but Qt seems to be not that good in handling such long strings in an input field.

sipa commented at 1:22 AM on June 17, 2012: member

You typically assume that a user who can access the UI or RPC is trusted, as otherwise you're more likely to lose your savings than the uptime of your node :)

laanwj commented at 7:22 PM on June 17, 2012: member

Hehe, yes, we don't protect people against DDoSing themselves on purpose, especially not through the UI. No fatal crashes should happen, of course, but slowing down is fine.

Diapolo commented at 7:44 PM on June 17, 2012: none

I just wanted to ask, as I DoSed my node yesterday (GUI freeze / slowdown ... no crash though).

luke-jr commented at 9:23 PM on June 18, 2012: member

I think the Paste button for Verify/Address and also for Verify/Signature are both needed.

For a warning, perhaps something like "Enter the signing address, message (ensure you copy line breaks, spaces, tabs, etc exactly) and signature below to verify the message. Be careful not to read more into the signature than what is in the signed message itself, to avoid being tricked by a man-in-the-middle attack!"

Diapolo commented at 5:40 AM on June 19, 2012: none

@luke-jr I'm fine with such a warning message, will change that. As for the paste button. Even if I place them next to the input field, they are generic. This means copying a signatute from sign and click the paste button on verify - address input, will try to paste the signature. If you say the user has to take this into account that's okay adding them, but there is no technical solution to distinguish them via different clipboards afaik.

laanwj commented at 5:57 AM on June 19, 2012: member

Different clipboards?!? Please don't make this too complex. I think the UI on the sign and verify message as shown in the screenshot is fine.

Diapolo commented at 6:14 AM on June 19, 2012: none

@laanwj Don't bother, I also would not like to have different clipboards ... just wanted to know what luke had in his mind for the buttons.

Diapolo commented at 9:22 AM on June 21, 2012: none

@laanwj I currently think it would be even nicer to remove the paste button from sign for the address input field instead of adding mor of that buttons to verify. Everyone should be able to use Ctrl + C / Ctrl + V (or whatever is default on your OS).

Diapolo commented at 10:21 AM on June 26, 2012: none

Any more feedback here? Would be nice to see it in 0.7.

Diapolo commented at 11:11 AM on June 27, 2012: none

Rebased to fix a merge conflict.

luke-jr commented at 5:05 PM on June 27, 2012: member

Functionality ACK. Still think it needs Paste buttons on the Verify tab - most PC users don't use keyboard shortcuts.

Diapolo commented at 5:08 PM on June 27, 2012: none

But they should at least be able to use a context menu, right? I'm still in between removing the paste button from sign-tab or adding more paste buttons ... the more I think about it the more I would like to streamline that UI part ;). Perhaps @laanwj should throw a coin so we can get this in before 0.7.

laanwj commented at 8:28 PM on June 27, 2012: member

There's a lot of code changes in this pull already. I still need to test it and read all the changes.

I think "merging the windows" is enough change in one go, further improvements can go into a later pull.

Diapolo commented at 12:05 PM on June 29, 2012: none

@laanwj Alright, no further feature changes in this pull, just waiting for your report and fix stuff if needed, otherwise I consider this feature-complete.

in src/qt/bitcoingui.cpp:None in 5b12a5cbdf outdated

 221 | +    verifyMessageAction->setToolTip(tr("Verify a message to ensure it was signed with a specified Bitcoin address"));
 222 | +    tabGroup->addAction(verifyMessageAction);
 223 | +
 224 |  #ifdef FIRST_CLASS_MESSAGING
 225 | -    messageAction->setCheckable(true);
 226 | +    firstClassMessagingAction = new QAction(QIcon(":/icons/edit"), tr("&Messaging"), this);

laanwj commented at 9:24 AM on June 30, 2012:

I don't like the name "Messaging" very much. I think it will confuse people into thinking it can send messages somehow (like an IM client or email). What about simply Sign/Verify Message?

Diapolo commented at 10:29 AM on June 30, 2012:

ACK, that describes better, what can be done there. Will update.

in src/qt/bitcoingui.cpp:None in 9b45bfb8bc outdated

 221 | +    verifyMessageAction->setToolTip(tr("Verify a message to ensure it was signed with a specified Bitcoin address"));
 222 | +    tabGroup->addAction(verifyMessageAction);
 223 | +
 224 |  #ifdef FIRST_CLASS_MESSAGING
 225 | -    messageAction->setCheckable(true);
 226 | +    firstClassMessagingAction = new QAction(QIcon(":/icons/edit"), tr("Sign/Verify &Message"), this);

Diapolo commented at 10:58 AM on June 30, 2012:

Updated this string and the tooltip below, which doesn't need an additional Transifex translation that way :).

luke-jr commented at 11:58 AM on June 30, 2012:

@laanwj I think this is too long for a tab; "Messaging" is already pretty clear in English IMO, especially considering that to get this one needs to enable "FIRST_CLASS_MESSAGING" (from source) or "1stclassmsg" (Gentoo).

Diapolo commented at 12:44 PM on June 30, 2012:

With the correct german translation a resize of the main Window would be needed because of the string length it's not shown on the tab bar :-/. 3 options, find a better suiting name, revert to Messaging or resize main client window.

luke-jr commented at 2:34 PM on June 30, 2012:

If @laanwj has a strong opposition to "Messaging", then perhaps "Signatures" or "Contracts" (to stress the importance of being careful what you sign) would work?

laanwj commented at 9:25 AM on July 6, 2012:

"Signatures" is fine with me. "Contracts" implies something more, like advanced transactions.

Diapolo commented at 9:27 AM on July 6, 2012:

I will update and use "Signatures" :).

Diapolo commented at 10:58 AM on June 30, 2012: none

Rebased and only changed the above mentioned strings.

Diapolo commented at 8:12 AM on July 6, 2012: none

@laanwj Can we get this in before the 0.7 RC Window please? I have a strong feeling that phase is coming soon and this still lingers around here ;).

Small changes can occur in another pull if required!

GUI: merge sign/verify message into a single window with tabbed UI

- add UI-feedback via QValidatedLineEdit
- copy button for generated signature was moved to the signature output field
- add an addressbook button to verify message tab
- input fields are now evenly ordered for sign and verify tabs
- update FIRST_CLASS_MESSAGING support to ensure a good UX
- add a button and context menu entry in addressbook for verify message (to be consistent with sign message)
- focus is now only set/changed, when clearing input fields or adding an address via addressbook
- re-work / update some strings
- ensure model gets initialized in the SignVerifyMessageDialog constructor
- add checks for a valid model to both addressbook buttons
- remove unneeded includes for Qt GUI elements that are listed in ui_signverifymessagedialog.h anyway

47894585ae

in src/qt/bitcoingui.cpp:None in 47894585ae

 221 | +    verifyMessageAction->setToolTip(tr("Verify a message to ensure it was signed with a specified Bitcoin address"));
 222 | +    tabGroup->addAction(verifyMessageAction);
 223 | +
 224 |  #ifdef FIRST_CLASS_MESSAGING
 225 | -    messageAction->setCheckable(true);
 226 | +    firstClassMessagingAction = new QAction(QIcon(":/icons/edit"), tr("S&ignatures"), this);

Diapolo commented at 9:33 AM on July 6, 2012:

Updated to "S&ignatures" (&S is used by "Send Coins").

in src/qt/addressbookpage.cpp:None in 47894585ae

 200 | +    {
 201 | +        QVariant address = index.data();
 202 | +        addr = address.toString();
 203 | +    }
 204 | +
 205 | +    QObject *qoGUI = parent()->parent();

laanwj commented at 11:48 AM on July 7, 2012:

I don't really like this code as it makes assumptions about the parent object and does explicit casts. Why not use a signal 'verifyMessage(QString addr)'?

Diapolo commented at 1:05 PM on July 7, 2012:

As you said above, I just copied the code, which was there :).

Did I understand your idea right, you would to do an emit verifyMessage(addr); and connect a SLOT to this in bitcoingui?

Diapolo commented at 4:54 PM on July 8, 2012:

@laanwj See #1569 for a fix for this.

in src/qt/addressbookpage.cpp:None in 47894585ae

 185 | @@ -182,7 +186,25 @@ void AddressBookPage::on_signMessage_clicked()
 186 |      QObject *qoGUI = parent()->parent();

laanwj commented at 11:48 AM on July 7, 2012:

Same here...

laanwj commented at 11:58 AM on July 7, 2012:

Right, I see this was the original code. Never mind, let's fix this up later...

laanwj referenced this in commit 249856d557 on Jul 7, 2012

laanwj merged this on Jul 7, 2012

laanwj closed this on Jul 7, 2012

nifgraup referenced this in commit e7c625ed26 on Mar 30, 2014

suprnurd referenced this in commit 11121747b8 on Dec 5, 2017

lateminer referenced this in commit 5cdec3c536 on Jan 22, 2019

lateminer referenced this in commit 7521d6d193 on May 6, 2020

lateminer referenced this in commit 02ede334f2 on May 6, 2020

DrahtBot locked this on Sep 8, 2021