Skip to content

Change notifications to use "source_" prefix.#4237

Merged
freakboy3742 merged 9 commits intobeeware:mainfrom
corranwebster:toga-sources
Mar 18, 2026
Merged

Change notifications to use "source_" prefix.#4237
freakboy3742 merged 9 commits intobeeware:mainfrom
corranwebster:toga-sources

Conversation

@corranwebster
Copy link
Copy Markdown
Contributor

@corranwebster corranwebster commented Mar 13, 2026

This fixes #4064. Backwards compatibility is maintained by continuing to look for methods without the source_ prefix if the expected method is not found. Existing widgets also continue to implement the old API. In both cases a DeprecationWarning will be issued if the old API is used.

The changes other than to the Source class and the examples were mainly done by search and replace.

This also takes the opportunity to make the new listener method arguments on widget implementations keyword-only, so they match the listener protocols exactly.

Fixes #4064
Ref #3760

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

This fixes beeware#4064. Backwards compatibility is maintained by continuing to
look for methods without the source_ prefix if the expected method is not
found.  Existing widgets also continue to implement the old API. In both
cases a DeprecationWarning will be issued if the old API is used.

Testbed tests still need to be added, and it is possible there are problems
with the testbed probes.
Comment thread core/src/toga/sources/base.py
@corranwebster
Copy link
Copy Markdown
Contributor Author

This PR has a minor conflict with #4235 - the winforms tree widget will need to have the methods renamed, either in this PR or the other, depending on which is merged first.

@corranwebster corranwebster marked this pull request as ready for review March 13, 2026 12:54
Copy link
Copy Markdown
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

A lot of code, but almost all mechanical. A couple of cosmetic issues flagged.

The only question of substance is, sadly, whether most of the specific changes in this are needed. I'm not sure if the compatibility shims in the backend implementations are actually needed. The widget implementation isn't a formally documented interface; and there isn't a use case for installing a v0.5.3 backend with a v0.5.4 core (because the backends are all strictly version locked with their cores).

So - the best argument I could make is "a user is calling an undocumented method on a private API" - and even then, it's an undocumented API that is designed to be called by a formal interface, not by end-users. That's getting sufficiently rarified that I wonder if the juice is worth the squeeze.

Comment thread changes/4046.bugfix.md Outdated
Comment thread changes/4046.removal.md Outdated
@@ -0,0 +1 @@
Sources now look for methods with names of the form `source_{notification}`, rather than just `notification` when the `notify` method is called.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should include a migration note - something to the effect of "if you have defined a custom listener on a source, you should rename the methods on the Listner interface (such as insert, remove, and change) to have a source_ prefix.

Comment thread core/src/toga/sources/base.py Outdated

def insert(self, *, index: int, item: object, parent: ItemT | None = None) -> None:
def source_insert(
self, *, index: int, item: object, parent: ItemT | None = None
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wish we had a ruff rule for this...

Suggested change
self, *, index: int, item: object, parent: ItemT | None = None
self,
*,
index: int,
item: object,
parent: ItemT | None = None

(Similarly in other uses in this file)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's the core problem with Black/Ruff - "any colour you like as long as it's black" - in particular I have a rant about writing Python tooling in Rust (rather than wrapping Rust) that I probably should put up on my blog sometime that relates to this problem - in practice there's no way to add a rule like that to Ruff without learning Rust.

But yes, I'll fix. There appear to be a fair number of other cases where this happens in the codebase, if I have time I may do a style only PR to fix those as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's the core problem with Black/Ruff - "any colour you like as long as it's black" - in particular I have a rant about writing Python tooling in Rust (rather than wrapping Rust) that I probably should put up on my blog sometime that relates to this problem - in practice there's no way to add a rule like that to Ruff without learning Rust.

Yeah - I'm definitely conflicted on this one. While it's definitely nice to have fast tools, the speed of black was never a thing I had a major problem with - it's more that it's got a team of people maintaining it that makes it useful as a tool.

But yes, I'll fix. There appear to be a fair number of other cases where this happens in the codebase, if I have time I may do a style only PR to fix those as well.

Yeah - not having an automated tool for this makes it difficult to enforce.

I do wonder how hard it would be to define a custom Ruff extension... there's a couple of these sorts of rules that would be preferable to have as automated checks...


# Alias for backwards compatibility:
# March 2026: In 0.5.3 and earlier, notification methods
# didn't start with 'source_'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know it's annoying because the overwhelming volume of changes in this PR are of this form... but is this style of shim needed? Is there a use case for an Android DetailedList implementation connecting to an old data source?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The use-case would be if someone implemented their own list source rather than inheriting from it, or inherited and overrode notify. Not sure why they would do that but there could be a number of reasons (instrumentation, testing and different dispatch mechanisms come to mind).

But also, if the next release is a 0.6.0 rather than a 0.5.x, it's probably an OK thing to break, and it's easy enough to remove them: they were added by a big regex, they can be taken away by a big regex too!

@corranwebster
Copy link
Copy Markdown
Contributor Author

corranwebster commented Mar 17, 2026

So - the best argument I could make is "a user is calling an undocumented method on a private API" - and even then, it's an undocumented API that is designed to be called by a formal interface, not by end-users. That's getting sufficiently rarified that I wonder if the juice is worth the squeeze.

As noted in my previous comment, the problem isn't people directly calling the implementation, it's that the sources are given the implementations as opaque objects and expect them to adhere to the listener protocols. This PR changes the listener protocol. And the listener protocol wasn't well documented as of last release, but that was more of a gap in the docs than a choice. So sources which have their own notify which expects the old listener protocols will break without the compatibility shim (sources which don't mess with notify are fine).

But also, this is easy to rip out.

corranwebster and others added 2 commits March 17, 2026 09:06
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
@freakboy3742
Copy link
Copy Markdown
Member

So - the best argument I could make is "a user is calling an undocumented method on a private API" - and even then, it's an undocumented API that is designed to be called by a formal interface, not by end-users. That's getting sufficiently rarified that I wonder if the juice is worth the squeeze.

As noted in my previous comment, the problem isn't people directly calling the implementation, it's that the sources are given the implementations as opaque objects and expect them to adhere to the listener protocols. This PR changes the listener protocol. And the listener protocol wasn't well documented as of last release, but that was more of a gap in the docs than a choice. So sources which have their own notify which expects the old listener protocols will break without the compatibility shim (sources which don't mess with notify are fine).

That's fair. It's niche, and I do wonder how many people will actually be impacted, but it's possible, and it's not a huge overhead to retain the compatibility shim, so I guess we might as well.

Copy link
Copy Markdown
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

👍

@freakboy3742 freakboy3742 merged commit 09f5a40 into beeware:main Mar 18, 2026
58 checks passed
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.

Collision between Listener and ListSource protocols

2 participants