Add view helper functions to Config class #2083
Conversation
There was a problem hiding this comment.
Thanks Annie, this is excellent work!
I made a few suggestions below. If you could look at the comments I suggested for OpenColorIO.h and check that there is a test for each aspect of the documented functionality for each new function, that would be appreciated.
I notice that one of the Linux CI runs is failing due to a frozen Python docs problem. Currently the frozen docs system is broken and so we will handle that separately. As long as the Windows and Mac tests are passing, that's all you need to be concerned with for now.
|
Hi, Doug! I haven't forgotten about this issue, but it has been a while, so I may have forgotten some items or fixes. Let me know if that's the case. While writing tests, I found that the |
|
Thank you for this update @annieln ! We have a few large high-priority projects happening at the moment, so it may take a few weeks for us to review your latest changes. I notice that the Linux CI builds failed but that looks to be unrelated to your work. Once we update the build scripts, I will re-run the Linux checks on this PR. Thanks again! |
|
Hi Doug! I'm hoping I can finish up this task for Dev Days 2025 - would we be able to re-run the Linux checks here? |
|
Hi Annie! I believe if you update this branch against main, it will re-run the CI! :) |
There was a problem hiding this comment.
@annieln, apologies for the delay in getting this reviewed!
I still need to review your unit tests, but there is one addition below I'd like you to make. When you push the commit, we will make sure the CI runs.
There was a problem hiding this comment.
@annieln, here is the rest of my review. I have a few more requests, but you've done really great work on this! Big thanks for all the work on the unit tests, we love to see this!
Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
…ewTransformName when shared Signed-off-by: annie <[email protected]>
…nt lines in tests, add virtualViewIsShared checks to getVirtualView-related functions Signed-off-by: annie <[email protected]>
…to tests/cpu/Display_tests.cpp Signed-off-by: annie <[email protected]>
…/cpu/Display_tests.cpp Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
…y_tests.cpp Signed-off-by: annie <[email protected]>
….cpp to use looks, rules, and description Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
|
I believe I've addressed all the suggested changes - let me know if I missed anything or if there's anything else! |
|
Your tests look great! Please ignore the failed Linux CI test, that is due to an unrelated problem in our build setup. |
…lays, compare_virtual_displays to Display_tests.cpp Signed-off-by: annie <[email protected]>
There was a problem hiding this comment.
Great work @annieln!!
Apologies for the delay getting it reviewed and thank you for being so diligent in addressing the comments. This is one of the biggest contributions we've received in the history of our Dev Days! These new capabilities will be very useful to the community!
|
Great work! |
|
@annieln, Cuneyt has raised a valid point about the Strcasecmp function calls. The behavior is undefined (and could crash) if a null pointer is passed as an argument. Please add the two checks he requested in Config.cpp and we should be good to merge this. Thanks again! |
…ared Signed-off-by: annie <[email protected]>
|
I've added the null/empty view name check to |
|
Merging this as the issues with the failing documentation-related CI tests are unrelated to this PR. Thanks again, @annieln !! |
doug-walker
merged commit 6c15d45
into
AcademySoftwareFoundation:main
…2083) * Add view helper functions to Config class Signed-off-by: annie <[email protected]> * add view helper functions to Config class and unit tests Signed-off-by: annie <[email protected]> * add view helper functions as python bindings Signed-off-by: annie <[email protected]> * add unit tests for view helper py bindings Signed-off-by: annie <[email protected]> * update with style changes and comments Signed-off-by: annie <[email protected]> * comprehensive unit tests for ViewsAreEqual and displayHasView Signed-off-by: annie <[email protected]> * improved in-line comments for unit tests Signed-off-by: annie <[email protected]> * test suite for comparing virtual displays, fix to getVirtualDisplayViewTransformName when shared Signed-off-by: annie <[email protected]> * update briefs for ViewsAreEqual + VirtualViewsAreEqual, remove redunant lines in tests, add virtualViewIsShared checks to getVirtualView-related functions Signed-off-by: annie <[email protected]> * move virtual_display_exceptions test from tests/cpu/Config_tests.cpp to tests/cpu/Display_tests.cpp Signed-off-by: annie <[email protected]> * move display_view_order test from tests/cpu/Config_tests.cpp to tests/cpu/Display_tests.cpp Signed-off-by: annie <[email protected]> * add usage of viewIsShared to compare_displays test Signed-off-by: annie <[email protected]> * add test of clearSharedViews to basic test module in tests/cpu/Display_tests.cpp Signed-off-by: annie <[email protected]> * change configs for compare_virtual_displays in tests/cpu/Config_tests.cpp to use looks, rules, and description Signed-off-by: annie <[email protected]> * improve test cases for python config tests Signed-off-by: annie <[email protected]> * restore old tests to Config_tests.cpp and move new tests compare_displays, compare_virtual_displays to Display_tests.cpp Signed-off-by: annie <[email protected]> * add null/empty view name handling to viewIsShared and virtualViewIsShared Signed-off-by: annie <[email protected]> --------- Signed-off-by: annie <[email protected]> Co-authored-by: Doug Walker <[email protected]>
…2083) * Add view helper functions to Config class Signed-off-by: annie <[email protected]> * add view helper functions to Config class and unit tests Signed-off-by: annie <[email protected]> * add view helper functions as python bindings Signed-off-by: annie <[email protected]> * add unit tests for view helper py bindings Signed-off-by: annie <[email protected]> * update with style changes and comments Signed-off-by: annie <[email protected]> * comprehensive unit tests for ViewsAreEqual and displayHasView Signed-off-by: annie <[email protected]> * improved in-line comments for unit tests Signed-off-by: annie <[email protected]> * test suite for comparing virtual displays, fix to getVirtualDisplayViewTransformName when shared Signed-off-by: annie <[email protected]> * update briefs for ViewsAreEqual + VirtualViewsAreEqual, remove redunant lines in tests, add virtualViewIsShared checks to getVirtualView-related functions Signed-off-by: annie <[email protected]> * move virtual_display_exceptions test from tests/cpu/Config_tests.cpp to tests/cpu/Display_tests.cpp Signed-off-by: annie <[email protected]> * move display_view_order test from tests/cpu/Config_tests.cpp to tests/cpu/Display_tests.cpp Signed-off-by: annie <[email protected]> * add usage of viewIsShared to compare_displays test Signed-off-by: annie <[email protected]> * add test of clearSharedViews to basic test module in tests/cpu/Display_tests.cpp Signed-off-by: annie <[email protected]> * change configs for compare_virtual_displays in tests/cpu/Config_tests.cpp to use looks, rules, and description Signed-off-by: annie <[email protected]> * improve test cases for python config tests Signed-off-by: annie <[email protected]> * restore old tests to Config_tests.cpp and move new tests compare_displays, compare_virtual_displays to Display_tests.cpp Signed-off-by: annie <[email protected]> * add null/empty view name handling to viewIsShared and virtualViewIsShared Signed-off-by: annie <[email protected]> --------- Signed-off-by: annie <[email protected]> Co-authored-by: Doug Walker <[email protected]> Signed-off-by: Michael Horsch <[email protected]>
#2049
#2082