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

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

Closed
wants to merge 2 commits into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Feb 22, 2020

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.

@luke-jr luke-jr changed the title 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 Feb 22, 2020
@fanquake fanquake added the GUI label Feb 22, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 22, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@instagibbs
Copy link
Member

can you give a "tutorial" on how to change address on master? I can't seem to figure this out.

@maflcko
Copy link
Member

maflcko commented Feb 25, 2020

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

Screenshot from 2020-02-26 00-28-43

Same on master:

Screenshot from 2020-02-26 00-30-36

@jonasschnelli
Copy link
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?

@luke-jr
Copy link
Member Author

luke-jr commented Feb 27, 2020

How would you edit labels then?

@luke-jr
Copy link
Member Author

luke-jr commented Feb 27, 2020

@MarcoFalke Actually, this was only addressing inline editing, so the dialog is another place to disable the editing... Fixed

@luke-jr
Copy link
Member Author

luke-jr commented Feb 27, 2020

@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.

@cvengler
Copy link
Contributor

NACK sorry.
I agree with @jonasschnelli, instead of removing it I would improve it

@luke-jr
Copy link
Member Author

luke-jr commented Feb 28, 2020

@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...

@cvengler
Copy link
Contributor

@luke-jr Do you really know that nobody is using it?
Also I was agreeing on the reason not on his choice though

@luke-jr
Copy link
Member Author

luke-jr commented Feb 28, 2020

It doesn't work at all... And hasn't for a long time now. How could anyone be using it?

@cvengler
Copy link
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

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2020
This has been broken since 0.17 and nobody noticed, so just remove it entirely.

Bug 1 (since 0.17, bitcoin#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, bitcoin#13756): "used" DestData should have been retained at the old address rather than deleted entirely.

This commit just disables the Qt::ItemIsEditable flag, which should be the minimal needed to disable the functionality and therefore the bugs.

Github-Pull: bitcoin#18194
Rebased-From: 5e82176
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Mar 5, 2020
@laanwj
Copy link
Member

laanwj commented Mar 12, 2020

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.

@luke-jr
Copy link
Member Author

luke-jr commented Mar 12, 2020

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

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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK 2667478, 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".

luke-jr added 2 commits May 6, 2020 18:14
This has been broken since 0.17 and nobody noticed, so just remove it entirely.

Bug 1 (since 0.17, bitcoin#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, bitcoin#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.
@luke-jr luke-jr force-pushed the bugfix_gui_edit_sendaddr branch from 2667478 to 0a44e08 Compare May 6, 2020 18:15
@luke-jr
Copy link
Member Author

luke-jr commented May 6, 2020

Squashed & fixed

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0a44e08

@DrahtBot
Copy link
Contributor

🐙 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".

@fanquake
Copy link
Member

If this is still relevant, it should be rebased and then re-opened in the gui repo.

@fanquake
Copy link
Member

fanquake commented Apr 8, 2021

Closing per #18194 (comment).

@fanquake fanquake closed this Apr 8, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants