Observer's Changed-Value Test -- We Are Overlooking an Opportunity

[vwheeler63]

Introduce the problem

In lv_observer.c, the new lv_subject_notify_if_changed() function does not test pointer values to see if they changed.

        case LV_SUBJECT_TYPE_GROUP :
        case LV_SUBJECT_TYPE_POINTER :
            /* Always notify as we don't know how to compare this */
            lv_subject_notify(subject);
            break;

This makes sense if you are thinking about doing a "deep comparison" of the objects being pointed to (which will never be practical in this context), but I think we are missing an opportunity to also upgrade this Subject type by not sending out notifications when the pointer value has not changed. I know I would use this in my Dashboard firmware projects.

Simply put: this is useful, and can simply be documented as a "shallow comparison" -- not a "deep comparison" -- such that if the pointer differs from its previous value that only then is a notification sent out, and not otherwise. This would also make it send a notification when the pointer value changes TO or FROM a NULL value. (Of course, we cannot hope to ever do a "deep comparison" without knowing the object type.)

Proposal

1. Change the above to:

        case LV_SUBJECT_TYPE_GROUP :
            /* Always notify as we don't know how to compare this */
            lv_subject_notify(subject);
            break;
        case LV_SUBJECT_TYPE_POINTER :
            if(subject->value.pointer != subject->prev_value.pointer) {
                lv_subject_notify(subject);
            }
            break;

2. Update observer.rst and API documentation surrounding updating a pointer-Subject's value.

[AndreCostaaa]

Hi @vwheeler63

I don't quite agree, here's why:

Currently, the SUBJECT_TYPE_POINTER is the only way to pass a complex data type using this mechanism

// On Producer Side
static my_data_t data;
static lv_subject_t subject;
lv_subject_init_pointer(&subject, &data);

....
// Something happens, time to update data
data.X = Y;

lv_subject_set_pointer(&subject, &data); // Consumer gets notified here.

Adding that check would make it more complicated to do this and would definitely use more memory, as it would mean keeping a subject but having to add another way of passing the actual data. It would also make it much less intuitive on how to use the api for this concrete case - How do you pass the data to the actual observer ?

Also, in the case the pointer is meant to point somewhere else, a simple check would solve the problem mentioned

if(lv_subject_get_previous_pointer(&subject) != new_ptr)
{
   lv_subject_init_pointer(&subject, new_ptr);
}

When using pointers I don't think making assumptions on how the user expects it to work is the way to go

[liamHowatt]

I can think of one concrete example where it's an issue to not notify on all pointer changes. Imagine the pointer is allocated memory which is resized with realloc so it returns the same pointer sometimes. You add more characters to the end and update the subject, but the event isn't sent because the new value compares equal to the previous value.

I also want to share a tiny "regression" in a demo from the change to the observer which only sends a notification if something changed (#7678). I was counting on an int subject sending a notification even if it didn't change: liamHowatt@2e9a962. And that's just with an int subject.

[AndreCostaaa]

Starting to think it wasn't such a good idea to make that change, the check can easily be done by the user if he wants to make sure the observer only gets notified when the value changes 🤔

[vwheeler63]

Currently, the SUBJECT_TYPE_POINTER is the only way to pass a complex data type using this mechanism

This is a good point, and I can definitely see use cases where you would want notification on every update for such a circumstance.

[vwheeler63]

Starting to think it wasn't such a good idea to make that change, the check can easily be done by the user if he wants to make sure the observer only gets notified when the value changes 🤔

I initially proposed it as an option, to be chosen by way of a passed Boolean at subscription time. The Boolean would indicate the choice to receive updates only on value change, and it would need to be stored it in an additional bit in the lv_observer_t object. So if the user only wants notification "on value changed", that Boolean would govern the execution path during notification.

cc: @kisvegabor

Thoughts?

[AndreCostaaa]

Currently, adding an observer consists on calling one of these 3 functions:

lv_observer_t * lv_subject_add_observer(lv_subject_t * subject, lv_observer_cb_t observer_cb, void * user_data);
lv_observer_t * lv_subject_add_observer_obj(lv_subject_t * subject, lv_observer_cb_t observer_cb, lv_obj_t * obj,
                                            void * user_data);
lv_observer_t * lv_subject_add_observer_with_target(lv_subject_t * subject, lv_observer_cb_t observer_cb,
                                                    void * target, void * user_data);

So here's my idea,

1. Revert the previous change - by default we don't check for the previous value
2. We add 3 new functions

#define LV_OBSERVER_ON_CHANGE_ONLY 1
lv_observer_t * lv_subject_add_observer_f(lv_subject_t * subject,
                                          lv_observer_cb_t observer_cb,
                                          uint32_t flags,
                                          void * user_data);
lv_observer_t * lv_subject_add_observer_obj_f(lv_subject_t * subject,
                                          lv_observer_cb_t observer_cb,
                                          lv_obj_t * obj,
                                          uint32_t flags,
                                          void * user_data);
lv_observer_t * lv_subject_add_observer_with_target_f(lv_subject_t * subject,
                                               lv_observer_cb_t observer_cb,
                                               void * target,
                                               uint32_t flags,
                                               void * user_data);

This doesn't break the api and we can easily add new flags in the future without fear of breaking the api by simply adding a OR between multiple options

And it also is more powerful than the current implementation because each observer can choose how they want to be notified

[vwheeler63]

And it also is more powerful than the current implementation because each observer can choose how they want to be notified

Hi, @AndreCostaaa! So if I am understanding your thoughts correctly, you're asserting that all the ..._bind_...() functions retain their checks for change?

Edit:

And further, the intent for the flags argument is to simply carry 1 Boolean value indicating whether they want change-filtering or not. Correct?

Note 1: I'm not the one authorized to give permissions here, so I am very interested in @kisvegabor's thoughts.

Note 2: I woke up this morning realizing the case for NOT doing change-filtering for a POINTER Subject MIGHT actually be a mis-use of the concept of the Observer pattern, and this MIGHT replace it in your use case:

• The use case you described above — as I understand it — is a pointer that points to a complex data structure that requires complex evaluation to determine if a change to the UI (or anything else in the system) is warranted after a notification.
• I have such use cases in 3 dashboard products I created.
• However, in my use cases, instead of subscribing with a POINTER Subject, there is always one KEY value in that complex structure that changes coincident with when I want to get notified -- even when more complex evaluation is needed during the notification to determine if it is a real ________ event. Let's say it is an INTEGER.
• Instead, I subscribe using an INTEGER Subject and get the notification when it changes. Since it is not written to the complex data structure yet, I use this opportunity to write it there and then...
• do the more complex evaluation and when it really is a real _______ event, only then do I _______ (whatever is called for, e.g. update the UI).
• And what PREVIOUSLY was stored in the pointer value as the value being "Observed" -- the pointer is then carried as user_data so I can extract the pointer and do the evaluation of the complex structure with it.

Does that effectively handle the use case you described?

Edit 2:

However, I see this does not handle the use case @liamHowatt presented.... Hmm...

[AndreCostaaa]

Hi, @AndreCostaaa! So if I am understanding your thoughts correctly, you're asserting that all the ..._bind_...() functions retain their checks for change?

I'm thinking we should revert the check by default, which would make "sense", by default the user has the choice of notifying even if the value hasn't changed and he can always opt in to that option using the new function

And further, the intent for the flags argument is to simply carry 1 Boolean value indicating whether they want change-filtering or not. Correct?

Correct, but it allows us to add more flags in the future by ORing new flags without breaking the API

Does that effectively handle the use case you described?

Yes, it does. I actually didn't mean to say it was impossible to work around it (notice i'm using "working around it" which is a red flag), i meant it is too complicated and it isn't as intuitive

The application needs to find a way to share the complex data type before starting observing the subject and then you need to use another variable to actually handle the notification when the problem you're trying to solve is to just share a pointer between two entities.
You're probably going to say that we already need to find a way to share the subject variable in the first place but that is a given, nothing can be done to circumvent that, the part that i dislike is

"I'm sharing A with you"
"Okay"
"But i can't share A with you directly, let me give you B, and when you see that B changes, you can check the new value in A"
"Okay..."

Hope this analogy makes it clearer to understand what i mean

However, I see this does not handle the use case @liamHowatt presented.... Hmm...

Like I mentioned, i don't think doing shallow comparisons with "generic" pointers is the way to go

[vwheeler63]

I'm thinking we should revert the check by default, which would make "sense", by default the user has the choice of notifying even if the value hasn't changed and he can always opt in to that option using the new function

I agree — that makes it a FEATURE of Observer — allowing the user to adapt it to whatever needs/algorithm he needs to set up.

Correct, but it allows us to add more flags in the future by ORing new flags without breaking the API

Good thinking.

Hope this analogy makes it clearer to understand what i mean

Quite clear, yes. My own use cases in the dashboard firmware I created/maintain has many varied notification scenarios, some simple, and some complex, and even some where the Observer is allowed to access the original data (a complex data structure), but must agree only to READ from it, not write to it. But that latter is simply a "protocol agreement" between the Subject and the Observer. What is nice is that the flexibility of the notification system (not LVGL Observer, but similar) is designed to allow that flexibility, because I have reusable Subjects (that exist in several different devices) and varied Observers that follow the Subject's (published) protocol for sharing data.

Like I mentioned, i don't think doing shallow comparisons with "generic" pointers is the way to go

Agreed. And in alignment with that — I definitely like the new functions giving the end programmer a CHOICE of how he wants to be notified, and it:

• would be a really nice feature of Observer — handling those use cases that we had not already foreseen,
• is a nice, clean design, and
• doesn't break the API (with all the consequences that go with that).

I really like this and I think @kisvegabor will like it also for the same reasons.

[vwheeler63]

@AndreCostaaa @liamHowatt @kisvegabor

Also, be advised of 2 other issues related to Observer that we could tie into this if desired: #7649 and #7694. I have the fix to #7694 pending in PR #7651, but if fixing it here caused a conflict, that is easily resolved. Or we could let PR #7651 address #7694 and simply address #7649.

[AndreCostaaa]

I vote for fixing both in #7651 and when that gets merged we can look into opening a PR for this

Also note @liamHowatt #7699 shouldn't be merged in case we decide to revert the "notify on change" change

[vwheeler63]

@AndreCostaaa @liamHowatt @kisvegabor

I just added Issue #7701 -- another bug connected to Observer.

[liamHowatt]

@kisvegabor can you comment regarding observer notification? I see here you said:

there is no way that notifying with the same value makes any difference, unless the user messes up something

So do you think we should not revert "notify on change" (#7678)? Users who have accidentally misused observer should update their logic accordingly, i.e. 2e9a962 from #7699 is a necessary correction?

RE pointer subjects, what do you think? Currently pointer observers are always updated even if the pointer did not change. We made some points that this is maybe a good thing to keep this way.

[kisvegabor]

Hi All,

From this comment, this is my standpoint:

I'd like to emphasis that no notification should be triggered when when setting the same value because of the philosophy of subject-observers: the observers are always in sync with the subject because when they subscribe a notification will be fired to get the current value (probably it should be documented too) and when the value changes a notification is fired as well. So there is no way that notifying with the same value makes any difference, unless the user messes up something, but it's out of scope now.

So I vote for comparing and NOT notifying when possible (int, string, color). And - in lack of a feasible comparison solution - for groups and pointer we should always send a notification. Note that not notifying with the same value can NOT cause any harm by concept.

If for any strange reason the user need to force notifying a subject on change, they can just call lv_subject_notify manually.

In light of that I think

1. The current implementation is good as it is
2. We should update the docs.

What do you think?

[AndreCostaaa]

Sounds good, i believe this issue can be closed then

[vwheeler63]

In light of that I think

1. The current implementation is good as it is

2. We should update the docs.

What do you think?

Done in PR #7651 ! Docs are fully updated, clean-up logic (what can and cannot be called) is fully clarified and ready to review. So since no change is needed stemming from this conversation, I am closing it. Thank you everyone for weighing in in this little "whiteboard session" to resolve, and thank you @kisvegabor for your time and evaluation! 😄