Qt: re-order GUI code and remove FIRST_CLASS_MESSAGING #1705

pull Diapolo wants to merge 2 commits into bitcoin:master from Diapolo:cleanup_bitcoingui changing 3 files +26 −82
  1. Diapolo commented at 10:46 AM on August 23, 2012: none
    • re-order Qt Actions and connect() calls to match the real GUI layout, which makes things easier to read and understand
    • remove FIRST_CLASS_MESSAGING from the client
    • remove signMessageAction and verifyMessageAction from tabGroup as we didn't use this anyway (as tooltips are not displayed in the menu remove these too)
  2. TheBlueMatt commented at 3:39 PM on August 23, 2012: member

    Can we just remove all of FIRST_CLASS_MESSAGING? Last I checked it was just a luke-jr thing to move where the message signing stuff is in the gui, does anyone actually run builds with it for regular use?

  3. laanwj commented at 3:51 PM on August 23, 2012: member

    Agree @TheBlueMatt it's only a lukejr thing, it's fine with me to remove it.

    I remember some time ago there was a redesign that moved the tabs to the side; when there is a lot of space for tabs it makes sense to make even message signing, which is a rarely used feature, a tab in itself. But with so limited space for tabs it really makes no sense.

  4. Diapolo commented at 4:16 PM on August 23, 2012: none

    I'm also fine with removing it entirely from the client, but I guess luke doesn't like that idea. This patch at least cleans up some stuff related to it.

  5. sipa commented at 4:34 PM on August 23, 2012: member

    No problem with removing it.

  6. Diapolo commented at 4:43 PM on August 23, 2012: none

    Alright, I'll update this pull to remove it then.

  7. Diapolo commented at 5:11 PM on August 23, 2012: none

    Updated to remove it entirely.

  8. luke-jr commented at 5:30 PM on August 23, 2012: member

    NACK. Don't remove it.

  9. luke-jr commented at 5:40 PM on August 23, 2012: member

    To expound a bit: Recall that first-class messaging was the default in the original implementation, and was only made optional as a temporary compromise while sign/verify message was still an uncommon function. Nowadays it is used by various services, including Bitcoin OTC, so if anything it should be restored as the default.

  10. sipa commented at 5:53 PM on August 23, 2012: member

    Please Luke, that's like claiming browsers need a proxy server bar as prominent as the search or URL bar. I'm sure various services use this, but making it so prominent just confuses people. I already see people assuming that they need to use that to manually confirm transactions.

    In my opinion, message sign/verification belongs in a "Tools" or "Utilities" menu option and nowhere else.

  11. TheBlueMatt commented at 5:57 PM on August 23, 2012: member

    IIRC, it was added not as a compromise not until it becomes a common feature, but to discourage it from becoming a common feature (which, IMHO, we should do more for, but probably first there is to create a good payment protocol)

  12. Diapolo commented at 5:58 PM on August 23, 2012: none

    @sipa I like the idea of a Tools / Utilities menu option, but that should go to another pull then :).

  13. luke-jr commented at 5:59 PM on August 23, 2012: member

    @sipa If there is still disagreement over this, then it should remain an option.

  14. sipa commented at 6:00 PM on August 23, 2012: member

    No offence, but I see no reason for maintaining this if you're the only proponent.

  15. luke-jr commented at 7:56 PM on August 23, 2012: member

    Early poll results suggest first-class messaging should be default.

  16. Diapolo commented at 8:40 PM on August 23, 2012: none

    I had that feeling ... and I was right. Everytime a decision is made that differs from what you would like it to be, some arguments are constructed even if you would be the only person in the world as proponent. Shall I open a poll everytime I argue with @laanwj over something... seems a little unconstructive in the end, sorry to say.

  17. luke-jr commented at 8:53 PM on August 23, 2012: member

    @Diapolo What is unconstructive is this removing of functionality people want for no reason.

  18. gavinandresen commented at 9:50 PM on August 23, 2012: contributor

    ACK, including removing FIRST_CLASS_MESSAGING (GUIs should not be designed via forum polls).

  19. luke-jr commented at 11:37 PM on August 23, 2012: member

    If FIRST_CLASS_MESSAGING is to be removed against consensus, could @Diapolo at least do it in a separate commit after the "re-order GUI code" so those of us who want it can revert in our own branches?

  20. Diapolo commented at 6:44 AM on August 24, 2012: none

    As I have overwritten my former version of that pull, I can't seperate to fullfill your whish, sorry... most of the re-order is cosmetic, so if you want your own version with FIRST_CLASS_MESSAGING just don't merge that one.

  21. luke-jr commented at 6:50 AM on August 24, 2012: member

    Check out .git/logs/refs/heads/cleanup_bitcoingui ;)

  22. BitcoinPullTester commented at 1:54 PM on August 24, 2012: none

    Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/e3eb634e44f677d02a08ed5bbc0d7d3acc0db6c7 for test log.

    This pull does not merge cleanly onto current master

  23. Diapolo commented at 3:23 PM on August 24, 2012: none

    @BitcoinPullTester The last error was fatal: 'e3eb634e44f677d02a08ed5bbc0d7d3acc0db6c7' does not point to a commit and that should not relate to the contents of this pull, as current commit-ID is 0b35903d75803eb983e6a4f4c3c888da5ae4e3d7.

  24. TheBlueMatt commented at 5:34 PM on August 24, 2012: member

    That probably means between the time pull tester got the list of pulls and the time it checked this pull, the commit that was this pull's head was removed from the git repo in question, it should give an update when it runs the new version. (also, please dont tag @BitcoinPullTester, I wont see it, please tag me instead)

  25. Diapolo commented at 12:22 AM on August 26, 2012: none

    @luke-jr Sorry, I don't know what you mean by Check out .git/logs/refs/heads/cleanup_bitcoingui and I don't want to spend too much time with this. I thought the change was ACKed and as I said you could just skip that pull?

  26. luke-jr commented at 1:11 AM on August 26, 2012: member

    You can't just "skip" pulls. If you split it up, you can revert the commit removing it. I can walk you through it on IRC...

  27. BitcoinPullTester commented at 6:20 AM on August 26, 2012: none

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

  28. Diapolo commented at 8:20 AM on August 29, 2012: none

    To do @luke-jr a small favor, I split up that pull into 2 commits, one removes the 1stclass stuff and the other does the GUI re-ordering. I hope everyone is happy now and this get's in soon to not have the need to further discuss it...

  29. remove FIRST_CLASS_MESSAGING support from the client
    - removes the FIRST_CLASS_MESSAGING support from the client, which was no
  default setting anyway
    5fc3a0f707
  30. Qt: re-order GUI code
    - re-order Qt Actions and connect() calls to match the real GUI layout,
  which makes things easier to read and understand
- remove signMessageAction and verifyMessageAction from tabGroup as we
  didn't use them anyway (as tooltips are not displayed in the menu remove
  these too)
- update 2 comments
    382e9e25ff
  31. BitcoinPullTester commented at 9:08 AM on September 6, 2012: none

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

  32. Diapolo commented at 5:26 PM on September 20, 2012: none

    As this didn't make it into 0.7, can please a dev merge this now? I find it rather hard to keep track of that many small pulls, which leads to a slow down on my side ^^.

  33. laanwj referenced this in commit 1ba4925755 on Sep 21, 2012
  34. laanwj merged this on Sep 21, 2012
  35. laanwj closed this on Sep 21, 2012
  36. suprnurd referenced this in commit 470e5435c0 on Dec 5, 2017
  37. DrahtBot locked this on Sep 8, 2021
Contributors
DiapoloTheBlueMattlaanwjsipaluke-jrgavinandresenBitcoinPullTester
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:16 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me