Bugfix: GUI: Remove broken ability to edit the address field in the sending address book #18194

1. 
   luke-jr commented at 5:06 am on February 22, 2020: member

   Apparently in the sending address book, it is possible to edit the address field of an address/label row.

   Doing so, however, has 2 pretty big bugs:

   Bug 1 (since 0.17, #10244): Instead of moving the label, the new address’s label was set to the new address itself, and the actual label was lost completely.

   Bug 2 (effectively since 0.19, #13756): All metadata associated with the old address (prior to edit) is deleted, except for label and purpose. Probably some should remain with the old address (eg, whether the associated key was used to spend already), and others should be moved to the new address (no existing cases, just hypothetically). I doubt we should ever just outright delete it.

   Edit: Bug 3 (unconfirmed, debatable): Editing both the label and address in the label-editing dialog would still delete the label for the old address.

   Proposed resolution: Remove the ability to edit the address field. It doesn’t really make sense.

2. luke-jr renamed this:
   GUI: Remove broken ability to edit the address field in the sending address book
   Bugfix: GUI: Remove broken ability to edit the address field in the sending address book
   on Feb 22, 2020
3. luke-jr requested review from jamesob on Feb 22, 2020
4. luke-jr requested review from achow101 on Feb 22, 2020
5. luke-jr requested review from jnewbery on Feb 22, 2020
6. luke-jr requested review from kallewoof on Feb 22, 2020
7. luke-jr requested review from laanwj on Feb 22, 2020
8. luke-jr requested review from MarcoFalke on Feb 22, 2020
9. luke-jr requested review from meshcollider on Feb 22, 2020
10. luke-jr requested review from Sjors on Feb 22, 2020
11. fanquake added the label GUI on Feb 22, 2020
12. 
    DrahtBot commented at 9:24 am on February 22, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20480 (Replace boost::variant with std::variant by MarcoFalke)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

13. 
    instagibbs commented at 5:00 pm on February 25, 2020: member

    can you give a “tutorial” on how to change address on master? I can’t seem to figure this out.
14. 
    MarcoFalke commented at 5:30 pm on February 25, 2020: member

    @instagibbs : Click “sending addresses” and then right click on an address and then “edit”.

    

    Same on master:

    

15. 
    jonasschnelli commented at 8:40 am on February 27, 2020: contributor

    The edit function is already somehow hidden. IMO Having a “address books” without an edit function seems quite limiting. @luke-jr: what would it mean to fix bug1/2?

    Would deprecating the address book (then removing it at some point) make sense?

16. 
    luke-jr commented at 12:10 pm on February 27, 2020: member

    How would you edit labels then?
17. 
    luke-jr commented at 11:23 pm on February 27, 2020: member

    @MarcoFalke Actually, this was only addressing inline editing, so the dialog is another place to disable the editing… Fixed
18. 
    luke-jr commented at 11:27 pm on February 27, 2020: member

    @instagibbs I suspect “how to inline edit” is platform-dependent, but on my KDE-based system, double-clicking on the field I intend to edit does it.
19. 
    emilengler commented at 5:11 pm on February 28, 2020: contributor

    NACK sorry. I agree with @jonasschnelli, instead of removing it I would improve it
20. 
    luke-jr commented at 5:13 pm on February 28, 2020: member

    @emilengler What is the use case? Clearly nobody uses it since it doesn’t work. @jonasschnelli seems confused about the PR, and isn’t NACKing it…
21. 
    emilengler commented at 5:17 pm on February 28, 2020: contributor

    @luke-jr Do you really know that nobody is using it? Also I was agreeing on the reason not on his choice though
22. 
    luke-jr commented at 7:29 pm on February 28, 2020: member

    It doesn’t work at all… And hasn’t for a long time now. How could anyone be using it?
23. 
    emilengler commented at 7:36 pm on February 28, 2020: contributor

    It doesn’t work at all…

    That’s why I said improve it instead of remove it

    How could anyone be using it?

    You’re right on that point though but maybe some users are frustrated because it didn’t worked

24. luke-jr referenced this in commit e9aec6ef89 on Mar 5, 2020
25. luke-jr referenced this in commit b6a90cd685 on Mar 5, 2020
26. 
    laanwj commented at 2:14 pm on March 12, 2020: member

    FWIW I tend to edit labels by right-clicking on a transaction then doing ’edit label’. Haven’t used the address book for ages.

    But as long as it’s there it should probably work.

27. 
    luke-jr commented at 2:28 pm on March 12, 2020: member

    This has no impact whatsoever on the ability to edit labels anywhere.

    This is about when the user attempts to edit the address.

28. hebasto changes_requested
29. 
    hebasto commented at 4:09 am on May 6, 2020: member

    Approach ACK 2667478697b044e57d9cb619bbf32cadd86fd295, tested on Linux Mint 19.3.

    Some notes:

    1. could the last commit “GUI: Disallow editing the address in EditAddressDialog with mode=EditSendingAddress” be squashed into the first one “Bugfix: GUI: Disable inline editing of sending addresses”

    2. the tooltip content is misleading now: “Right click to edit address or label”.

30. Bugfix: GUI: Disable editing of sending addresses

    This has been broken since 0.17 and nobody noticed, so just remove it entirely.
    
    Bug 1 (since 0.17, #10244): Instead of moving the label, the new address's label was set to the new address itself, and the actual label was lost completely.
    
    Bug 2 (since 0.19, #13756): "used" DestData should have been retained at the old address rather than deleted entirely.
    
    Bug 3: Editing both the label and address in the label-editing dialog would still delete the label for the old address.
    
    This commit just turns off the GUI editing capabilities, which should be the minimal needed to disable the functionality and therefore the bugs.

    0e9e25ade0
31. GUI: Delete now-dead code to handle inline edits of sending addresses 0a44e08992
32. luke-jr force-pushed on May 6, 2020
33. 
    luke-jr commented at 6:15 pm on May 6, 2020: member

    Squashed & fixed
34. hebasto approved
35. 
    hebasto commented at 5:56 am on May 8, 2020: member

    ACK 0a44e08992f5c2f4a5077071c13599da99559905
36. 
    DrahtBot commented at 4:46 am on January 11, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

37. DrahtBot added the label Needs rebase on Jan 11, 2021
38. 
    fanquake commented at 7:13 am on March 26, 2021: member

    If this is still relevant, it should be rebased and then re-opened in the gui repo.
39. 
    fanquake commented at 2:20 am on April 8, 2021: member

    Closing per #18194 (comment).
40. fanquake closed this on Apr 8, 2021

    ---

41. DrahtBot locked this on Aug 16, 2022

Contributors
luke-jr DrahtBot instagibbs MarcoFalke jonasschnelli emilengler laanwj hebasto fanquake

Review Requested
Sjors jamesob laanwj kallewoof jnewbery meshcollider achow101 MarcoFalke

Labels
GUI Needs rebase