Bitcoin-Qt: cleanup / optimise addressbookpage #2165

Diapolo commented at 8:00 AM on January 10, 2013: none

don't show QR Code context menu, when USE_QRCODE=1 was not specified when compiling the client
re-work on_showQRCode_clicked() for better readability and remove an unneeded duplicate check
re-work on_signMessage_clicked() and on_verifyMessage_clicked() to match foreach in on_showQRCode_clicked(), which seems more robust / cleaner
re-order context menu stuff to match real context menu layout
add comments for all private slots in the class

laanwj commented at 8:15 AM on January 10, 2013: member

ACK on code changes, haven't tested yet

BitcoinPullTester commented at 8:20 AM on January 10, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/8910caf3ac09c91b6d2c6b3775780f57b3790e83 for binaries and test log.

Diapolo commented at 12:47 PM on January 10, 2013: none

@laanwj I intend to extend the addressbook context menu with an option to directly feed the send coins tab with address and label. This pull is nearly ready, but I thought it would be good to get this and the addrbook fixes merged before.

Diapolo commented at 9:48 PM on January 11, 2013: none

added:

don't show QR Code context menu, when USE_QRCODE=1 was not specified when compiling the client

This is related to #2170 and should fix that bug.

Diapolo commented at 11:46 PM on January 11, 2013: none

added:

comments for all slots that are not related to a button (to ensure to not use a on_foo_clicked() format, which causes warnings from the Qt IDE)

in src/qt/addressbookpage.cpp:None in 3ae584fb15 outdated

 249 | @@ -252,7 +250,6 @@ void AddressBookPage::selectionChanged()
 250 |              // In sending tab, allow deletion of selection
 251 |              ui->deleteButton->setEnabled(true);
 252 |              ui->deleteButton->setVisible(true);
 253 | -            deleteAction->setEnabled(true);

laanwj commented at 10:20 AM on January 13, 2013:

Shouldn't this be changed to deleteEntryAction instead of removed?

Diapolo commented at 10:28 AM on January 13, 2013:

Would this work, as deleteEntryAction is local in the constructor? Indeed, deleteEntryAction is not accessible here ;).

laanwj commented at 2:31 PM on January 19, 2013:

Ok, but what makes sure that the action is no longer enabled, if it's not accessible here? It used to be accessible, why change that?

Diapolo commented at 2:46 PM on January 19, 2013:

We can't be sure it's not enabled, so it would be safer to revert, if we really need to disable the action.

laanwj commented at 3:54 PM on January 19, 2013:

It should be disabled, otherwise 'delete' is enabled when nothing is selected (at least I think so?)

Diapolo commented at 6:11 PM on January 19, 2013:

I'm thinking out loud, please prove me wrong. Addressbook has a delete button, whereas sendcoins has none, as we disable and hide it for the sendcoins page. The same is true for the context menu, the delete entry is ONLY added on the addressbook page and not visible or accessible on sendcoins (we only add it when if(tab == SendingTab)).

So even if the action itself is not disabled, it is not accessible in any way on sendcoins, which I veriefied by just looking on that page ^^.

laanwj commented at 1:09 PM on January 22, 2013:

But in my mind the point of that code is to disable all the actions (including deleting) if there is no selection. This is orthogonal the the send/receive coins tab. Or not?

Diapolo commented at 2:41 PM on January 22, 2013:

The important phrase here is if there is no selection I'll verify this, as I now understand your problem with that change :).

BitcoinPullTester commented at 11:33 PM on January 13, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3ae584fb15e05e098cce3d818e39a590ba9269cd for binaries and test log.

Diapolo commented at 1:00 PM on January 22, 2013: none

@laanwj ping (seems 0.8 progress is speeding up, thats why I'm pinging you more often ^^)

Diapolo commented at 6:37 PM on January 22, 2013: none

@laanwj This is the screen directly after starting the client and switching to the addressbook:

It seems like the first address is "somewhat" selected, so I tried to access the context menu via Keyboard, which works. Selected "delete" and nothing happened. This is really the only case, when I could create a "nothing is selected". When I delete an address there is indeed a situation, where really NOTHING is selected, but this case doesn't even allow to open the context menu.

Now switching to send coins leads to his screen:

When I now try to access the context menu via keyboard there is NO "delete" action at all. So I really dunno how one can trigger a delete action for own addresses...

Diapolo commented at 9:10 PM on January 23, 2013: none

@laanwj My whish is to get your final decision on this, so we can get it into 0.8 RC :)! Your final vote counts.

laanwj commented at 9:31 PM on January 23, 2013: member

I like to merge this because of the QR_CODE context menu...

But I'm still not really happy about the deleteAction. I don't see an obvious way to trigger it either, but explicitly disabling it is safer if, perhaps due to some other change, it suddenly becomes possible.

Please try to make pull requests that solve reported issues, make the UI clearer/more pretty, etc, but not just massage and move around the code a bit, with the hope it will not introduce any new bugs.

Diapolo commented at 9:35 PM on January 23, 2013: none

In that state I consider your vote a help, you say revert that deleteAction change and I'll do this asap :).

Edit: @laanwj I chose to revert that change!

Bitcoin-Qt: cleanup / optimise addressbookpage

- don't show QR Code context menu, when USE_QRCODE=1 was not specified
  when compiling the client
- re-work on_showQRCode_clicked() for better readability and remove an
  unneeded duplicate check
- re-work on_signMessage_clicked() and on_verifyMessage_clicked() to match
  foreach in on_showQRCode_clicked(), which seems more robust / cleaner
- re-order context menu stuff to match real context menu layout
- add comments for all private slots in the class

bb0726a8cf

BitcoinPullTester commented at 10:17 PM on January 23, 2013: none

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/bb0726a8cfbe985beb668790bb5d01a4caaaa5c4 for binaries and test log.

This could happen for one of several reasons:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

Diapolo commented at 10:23 PM on January 23, 2013: none

cp: writing out/test_bitcoin.exe': No space left on device` seems not to be related to my last update!

laanwj referenced this in commit 05e5e15887 on Jan 23, 2013

laanwj merged this on Jan 23, 2013

laanwj closed this on Jan 23, 2013

laudney referenced this in commit c9ed612885 on Mar 19, 2014

sidhujag referenced this in commit 4f5bfd533d on Jul 13, 2018

DrahtBot locked this on Sep 8, 2021