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

Proposed modifications for the example of "Widgets" #3913

Closed
jrycw opened this issue Dec 20, 2023 · 4 comments
Closed

Proposed modifications for the example of "Widgets" #3913

jrycw opened this issue Dec 20, 2023 · 4 comments
Assignees
Labels
documentation Improvements or additions to documentation wontfix This will not be worked on

Comments

@jrycw
Copy link
Contributor

jrycw commented Dec 20, 2023

This issue is intended to propose modifications to the example provided in guide/widgets, and the original code can be accessed here.

  • In reality, the on_bit_switch_bit_changed of ByteEditor receives BitSwitch.BitChanged but does not use it. Consequently, there is no need to pass the bit and value to BitSwitch.BitChanged while initializing.

  • The value(textual.reactive.reactive) in BitSwitch appears to indicate the switch status (on/off). Therefore, it seems more natural to use value = reactive(False) instead of value = reactive(0).

To summarize, I would suggest modifying BitSwitch with the following code:

class BitSwitch(Widget):
    """A Switch with a numeric label above it."""

    DEFAULT_CSS = """
    BitSwitch {
        layout: vertical;
        width: auto;
        height: auto;
    }
    BitSwitch > Label {
        text-align: center;
        width: 100%;
    }
    """

    class BitChanged(Message):
        """Sent when the 'bit' changes."""

    value = reactive(False)

    def __init__(self, bit: int) -> None:
        self.bit = bit
        super().__init__()

    def compose(self) -> ComposeResult:
        yield Label(str(self.bit))
        yield Switch()

    def watch_value(self, value: bool) -> None:
        """When the value changes we want to set the switch accordingly."""
        self.query_one(Switch).value = value

    def on_switch_changed(self, event: Switch.Changed) -> None:
        """When the switch changes, notify the parent via a message."""
        event.stop()
        self.value = event.value
        self.post_message(self.BitChanged())
@rodrigogiraoserrao
Copy link
Contributor

Hey @jrycw, thanks for opening this issue!

We represent BitSwitch.value with a switch because bits are commonly thought of in terms of on/off, like you said, but BitSwitch.value is the value of the binary expansion of the number in that bit, so it makes sense to keep it as an integer.
I'll go through the example to make sure it's clear when we're working with integers vs Booleans.

As for the event accepting the bit and value parameters, I believe the author of that guide added them because those are sensible things to include in that sort of message...
But it may be confusing for people reading the code because they can't see those parameters being used...
Point taken.

@Textualize Textualize deleted a comment from github-actions bot Dec 21, 2023
@rodrigogiraoserrao
Copy link
Contributor

Apparently the remainder of the code also thinks BitSwitch.value is a Boolean, so I'll go with your suggestion!

rodrigogiraoserrao added a commit that referenced this issue Dec 21, 2023
rodrigogiraoserrao added a commit that referenced this issue Dec 21, 2023
@rodrigogiraoserrao rodrigogiraoserrao added the documentation Improvements or additions to documentation label Dec 21, 2023
@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Dec 21, 2023
@rodrigogiraoserrao
Copy link
Contributor

See the PR linked above for the resolution of this issue.

@rodrigogiraoserrao rodrigogiraoserrao added the wontfix This will not be worked on label Jan 3, 2024
Copy link

github-actions bot commented Jan 3, 2024

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants