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

Investigate an improvement to how the namespace of messages is calculated #1814

Closed
davep opened this issue Feb 16, 2023 · 4 comments · Fixed by #2038
Closed

Investigate an improvement to how the namespace of messages is calculated #1814

davep opened this issue Feb 16, 2023 · 4 comments · Fixed by #2038
Assignees
Labels
enhancement New feature or request Task

Comments

@davep
Copy link
Contributor

davep commented Feb 16, 2023

Consider the following code, where there is a base class for two widgets that share a lot in common but have slightly different uses/APIs/reasons-to-exist. They both have a value in common and so will have a Changed message (consider this almost pseudocode to save me trying out all of the details):

class MyBaseWidget( Widget ):

    value: reactive[bool] = reactive( False )

    class Changed( Message ):
        pass

    def watch_value( self ) -> None:
        self.post_message_no_wait( self.Changed() )

class HelloWidget( MyBaseWidget ):
    ...

class GoodbyeWidget( MyBaseWidget ):
   ...

To allow for sharing the bulk of the Changed message code between the two child classes, it would be useful if they could be declared something like this:

class HelloWidget( MyBaseWidget ):
    class Changed( MyBaseWidget.Changed ):
        pass

class GoodbyeWidget( MyBaseWidget ):
    class Changed( MyBaseWidget.Changed ):
        pass

This almost works perfectly, expect that the message handlers that are looked for end up always being called on_my_base_widget_changed when we would want them to be on_hello_widget_changed and on_goodbye_widget_changed. This can be achieved and works perfectly if you do the following:

class HelloWidget( MyBaseWidget ):
    class Changed( MyBaseWidget.Changed ):
        namespace = "hello_widget"

class GoodbyeWidget( MyBaseWidget ):
    class Changed( MyBaseWidget.Changed ):
        namespace = "goodbye_widget"

With this setup the parent widget can be "internal" and do almost all of the work and the two child classes can just handle their own differences and the events end up with the right names.

So... so far so good, it works fine, there's no issue here.

However, in conversation about this with Will, we believe that the Textual internals that work out the namespace for a given message should get the above right without the need to declare the namespace.

So the purpose of this issue is, at some point, to dive into this code or thereabouts and see if there's a reason why the namespace isn't worked out as we'd expect here.

@davep davep added enhancement New feature or request Task labels Feb 16, 2023
@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Mar 9, 2023
@rodrigogiraoserrao
Copy link
Contributor

Thanks for the thorough message and pseudocode! You made finding the issue and the fix a breeze! PR incoming. 🚀

@davep
Copy link
Contributor Author

davep commented Mar 13, 2023

Reminder to ourselves that once this is sorted we can tweak the code for Checkbox and for RadioButton.

@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

@rodrigogiraoserrao
Copy link
Contributor

Reminder to ourselves that once this is sorted we can tweak the code for Checkbox and for RadioButton.

On it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants