Schedule reactive callbacks on watcher.

[rodrigogiraoserrao]

The async reactive callbacks are now scheduled on the message pump of the watcher of the reactive instead of on the owner of the reactive attribute. Related issues: #3036.

[willmcgugan]

Had some additional thoughts about this.

We're posting a callback message which will result the callback running after all other messages.

On reflection, there is need to wait for that. We have a call_next method which calls the callback immediately after the handlers returns. I think we should use that mechanism. That way the watcher is called as close as possible to when the value was set.

[rodrigogiraoserrao]

@willmcgugan the reasoning makes sense to me, but the tests didn't like it. It breaks test_message_pump.py::test_prevent.
Do you have a clue on why it may be breaking the workings of the prevent context manager?

[willmcgugan]

Nothing comes to mind. What was the error?

[rodrigogiraoserrao]

Nothing comes to mind. What was the error?

The prevent just doesn't work 😆

with input.prevent(Input.Changed):
    input.value = "bar"

The code above will still post the input changed event.

Full error:

    async def test_prevent() -> None:
        app = PreventTestApp()

        async with app.run_test() as pilot:
            assert not app.input_changed_events
            input = app.query_one(Input)
            input.value = "foo"
            await pilot.pause()
            assert len(app.input_changed_events) == 1
            assert app.input_changed_events[0].value == "foo"

            with input.prevent(Input.Changed):
                input.value = "bar"

            await pilot.pause()
>           assert len(app.input_changed_events) == 1
E           AssertionError: assert 2 == 1
E            +  where 2 = len([Input.Changed(input=Input(), value='foo', validation_result=None), Input.Changed(input=Input(), value='bar', validation_result=None)])
E            +    where [Input.Changed(input=Input(), value='foo', validation_result=None), Input.Changed(input=Input(), value='bar', validation_result=None)] = PreventTestApp(title='PreventTestApp', classes={'-dark-mode'}).input_changed_events

tests/test_message_pump.py:88: AssertionError

[rodrigogiraoserrao]

I might be reading this incorrectly, but it looks like the post_message(Callback(...)) goes through a code path where the Callback message is updated with the currently active prevented messages.

By using call_next with the actual watcher, I'm bypassing all of that.

[willmcgugan]

Ah. You might want to investigate if call_next could be made to work with prevent.

[rodrigogiraoserrao]

Ah. You might want to investigate if call_next could be made to work with prevent.

I was looking at it.
prevent works by putting information on the Message instance that is posted.
Maybe call_next (and the others) could store Callback events instead of just storing the callback itself?

[rodrigogiraoserrao]

I investigated further and what's happening is much more interesting and nuanced than what I first thought.

Premise: I think yes, call_next could be made to work in this scenario but I think we shouldn't do it because it will add too much complexity for little/no gains.

Reasoning: My experiments actually showed cases where using call_next will call the handler later than using post_message.

The case in point is the test test_prevent in test_message_pump.py, and these two lines of code in particular:

textual/tests/test_message_pump.py

Lines 84 to 85 in 1270b8d

	with input.prevent(Input.Changed):
	input.value = "bar"

When we use call_next instead of post_message, Textual actually exists the context manager before the watcher gets called, which means that when the watcher gets called and the Input.Changed message is posted, we no longer know that we wanted to prevent those messages.
I concluded this by tracing the code and setting breakpoints in the watcher and the context manager prevent.

[willmcgugan]

I think we should call_next here.

If prevent isn't working with call_next, then I would consider that a bug. We would need to record the prevented messages at the time call_next was invoked.

[rodrigogiraoserrao]

If prevent isn't working with call_next, then I would consider that a bug. We would need to record the prevented messages at the time call_next was invoked.

So, what you are saying is that call_next(callback) should run the callback with the same prevented messages as when the callback was scheduled?
That doesn't sound right to me: with prevent(...): ... prevents messages inside that context manager, at that point in time.
call_next schedules a callback for later, when you're already out of the context manager and where I claim the messages shouldn't be prevented 🤷

I'll be happy to chat about it tomorrow, if you want.

[rodrigogiraoserrao]

@willmcgugan call_next now works with prevented messages (ac62096) and this PR now uses call_next instead of post_message to schedule the invoking of the watcher (9980148).

Feel free to review.

[willmcgugan]

@rodrigogiraoserrao would you mind resolve the CHANGELOG

[rodrigogiraoserrao]

@rodrigogiraoserrao would you mind resolve the CHANGELOG

Done. I'll wait for tests and then merge.