InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.37k stars 660 forks source link

ENH: allow calling RemoveObserver() from a const itkObject #4685

Closed tm9k1 closed 1 month ago

tm9k1 commented 1 month ago

Exposes a const version of RemoveObserver() API just like the already existing AddObserver() API here .

Helps with application architecture when building observer design pattern with itkObject as subject.

@mentions: @thewtex - I apologize for mentioning you without any sort of prior discussion. I'm a new contributor, and would be very grateful for some feedback on this PR!

PR Checklist

Thanks for reviewing this change!

dzenanz commented 1 month ago

You need to fix compile errors, of course.

thewtex commented 1 month ago

@tm9k1 thank you for the contribution. :pray: Please also exercise the addition in the test suite.

tm9k1 commented 1 month ago

Thanks so much for the prompt review! I've added a couple lines within the basic architecture test suite to include testing of the const version of RemoveObserver.

Given that I just wanted to contribute in-passing, I haven't looked much into the way of working of the team here beyond your feedback and a quick look over the general contribution guidelines. So, please do let me know if I missed some important requirements for the patch, including the test.

tm9k1 commented 1 month ago

for some reason the macOS build step failed after 45 minutes.. re-triggered.

N-Dekker commented 1 month ago

for some reason the macOS build step failed after 45 minutes.. re-triggered.

@tm9k1 Just for your information, you may also retrigger a single build by adding a comment of the form /azp run <build machine>. For example: https://github.com/InsightSoftwareConsortium/ITK/pull/4650#issuecomment-2101219120 But in your case: /azp run ITK.macOS.Arm64

N-Dekker commented 1 month ago

Wow, the beautiful unit test has failed! At https://open.cdash.org/tests/1563467945

Error in !shrink->HasObserver(itk::StartEvent())


Oops, I see now: at that point in time, shrink still has an itk::AnyEvent as observer. Apparently that will make shrink->HasObserver(event) return true for any event!!!

Apparently, before your first ITK_TEST_EXPECT_TRUE, you should do:

shrink->RemoveObserver(anyEventTag);

With anyEventTag being initialized by the return value of the existing shrink->AddObserver(itk::AnyEvent(), allEvents) call.

Unless you have a better idea, of course!

tm9k1 commented 1 month ago

Thanks for looking into the issue, Niels. I can confirm that your diagnosis was correct. The code now removes the AnyEvent observer and then the endEvent observer as const. P.S: I also spent some time actually building and testing the damn thing, so it should be all green this time. Sorry for the inconvenience!

N-Dekker commented 1 month ago

@tm9k1 Thank you very much for your contribution, and for addressing our comments so carefully, Piyush!