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

Binary encoding #337

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Binary encoding #337

merged 1 commit into from
Nov 13, 2023

Conversation

robertmin1
Copy link

No description provided.

@robertmin1 robertmin1 changed the title Binary encoding WIP: Binary encoding Jun 28, 2023
@JeremyRand
Copy link
Member

It looks like this contains a copy of #338 . Is that because there would be merge conflicts otherwise?

@JeremyRand
Copy link
Member

I'd prefer not to commit compiled form files into Git; those files are already generated at build time, which is preferred by the Debian guys.

@robertmin1
Copy link
Author

Alright! I'm not sure why it has a copy of #338. I will check this out

@robertmin1
Copy link
Author

Fixed the two issues

@JeremyRand
Copy link
Member

It looks like you're using bytes.fromhex etc. for messing with the encoding. We already have encoding/decoding functions in names.py that you can use for this, which will ensure consistency with Namecoin Core behavior.

@robertmin1
Copy link
Author

Switched to the encoding/decoding functions in names.py

@JeremyRand
Copy link
Member

This looks pretty good so far.

Initial feedback:

  1. It might be useful to put the ASCII textbox and the hex textbox into tabs, so that the user only sees one at a time. You should be able to copy the tab GUI code from the Manage DNS dialog.
  2. The initial value of the hex textbox in the Buy Names tab is empty; it should get automatically filled in when the page loads, based on the default ASCII value.
  3. Some visual feedback if the user enters invalid hex strings (either non-hex characters or an odd number of hex characters) would be nice.

Keep it up! :)

@@ -3360,9 +3363,24 @@ def update_buy_names_preview(self):
identifier = self.buy_names_new_name_lineedit.text().encode('ascii')
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use the hex textbox instead here?

Copy link
Author

@robertmin1 robertmin1 Aug 5, 2023

Choose a reason for hiding this comment

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

I am presently updating the hex text box using this line (HERE)
Is it essential to set the hex text box and then retrieve its content again to update the name preview?

Copy link
Author

@robertmin1 robertmin1 Aug 5, 2023

Choose a reason for hiding this comment

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

Alternatively, I can implement a separate function to read the ASCII text_box (For the Hex text_box) then name_preview to read from the hex text_box. But I think the current implementationis preferable.

@JeremyRand
Copy link
Member

I tried typing hex that was invalid ASCII (e.g. ffffff) and the preview didn't update; I suspect that the preview is incorrectly retrieving its data from the ASCII textbox instead of the Hex textbox.

@robertmin1
Copy link
Author

robertmin1 commented Aug 16, 2023

A helper function might be helpful to handle the state of buttons and labels?

def update_buttons_and_label(self, condition=None, button=None, label=None, error_message=None):
    if button is not None:
        button.setEnabled(condition)
    if label is not None:
        if error_message:
            label.setText(error_message)
        else:
            label.clear()

@JeremyRand
Copy link
Member

Is the idea to reuse that helper function across the Buy Names page, the Configure Name page, etc.? If so, yeah I think that's reasonable.

@robertmin1 robertmin1 force-pushed the binary_encoding branch 4 times, most recently from 1197eb4 to 0cdf435 Compare September 5, 2023 17:20
@JeremyRand
Copy link
Member

Typing a string that doesn't end in .bit to the Domain textbox seems to cause the string to be copied verbatim to the ASCII textbox instead of producing an error.

@JeremyRand
Copy link
Member

As discussed, no further changes are needed, but can you squash the commits, and then remove WIP: from the PR title?

@robertmin1 robertmin1 changed the title WIP: Binary encoding Binary encoding Oct 31, 2023
@robertmin1 robertmin1 force-pushed the binary_encoding branch 17 times, most recently from af77b1a to aea6368 Compare October 31, 2023 12:50
Support binary encoding of names in Electrum-NMC GUI (exposed as hex in GUI)
@JeremyRand
Copy link
Member

ACK 32af8d8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants