Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Kdenlive adapter for split out #1438

Merged

Conversation

jlskuz
Copy link
Contributor

@jlskuz jlskuz commented Oct 1, 2022

Summarize your change.

As part of the ongoing outlining process of contrib adapters, this removes the Kdenlive adapter from the core repository. As discussed with the Kdenlive team (of which @vpinon, the original author, is part), the code has been moved to the KDE GitLab https://invent.kde.org/multimedia/kdenlive-opentimelineio

Related #1378

See also discussion in #1390

NOTE: while removing the section about AAF from docs/tutorials/otio-plugins.md is theoretically not part of this PR, it is required to make the tests pass.

@jlskuz jlskuz force-pushed the work/split-kdenlive branch 3 times, most recently from 6c4a8b9 to 72c0248 Compare October 1, 2022 11:54
@meshula
Copy link
Collaborator

meshula commented Oct 1, 2022

It's fantastic to see the adapter hosted at kde.org!

What's the nature of the test failure stemming from the AAF documentation block?

@jlskuz
Copy link
Contributor Author

jlskuz commented Oct 3, 2022

It is due to docs/tutorials/otio-plugins.md being auto-generated in sync with the code. The section about AAF was still in the doc, while the code as been (re)moved. See the first paragraph of docs/tutorials/otio-plugins.md:

This document is automatically generated by running the
autogen_plugin_documentation command, or by running make plugin-model. It
is part of the unit tests suite and should be updated whenever the schema
changes. If it needs to be updated, run: make doc-plugins-update and this
file should be regenerated.

This is the (shortened) output of the test:

======================================================================
FAIL: test_plugin_documentation (test_serialized_schema.PluginDocumentationTester)
Verify that the plugin manifest matches what is checked into the
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/julius/Programmierung/github/OpenTimelineIO/tests/test_serialized_schema.py", line 67, in test_plugin_documentation
    self.assertMultiLineEqual(
AssertionError: '# Pl[614 chars]\n- `opentimelineio/adapters/builtin_adapters.[17532 chars]\n\n' != '# Pl[614 chars]\n- `site-packages/otio_moviemaker_adapter/plu[2903 chars]\n\n'
[...]  
 : 
 The schema has changed and the autogenerated documentation in /home/julius/Programmierung/github/OpenTimelineIO/docs/tutorials/otio-plugins.md needs to be updated.  run: `make doc-plugins-update`

----------------------------------------------------------------------

@meshula
Copy link
Collaborator

meshula commented Oct 3, 2022

I'm approving, but I'd like to get a response from more of the TSC since this is a structural change to the project.

@meshula
Copy link
Collaborator

meshula commented Oct 3, 2022

Since the AAF plugin hasn't yet moved to its own repo, it would be good to get the plugin authors to have a look at the documentation issue, and perhaps resolve the AAF documentation bug as a separate PR.

@meshula
Copy link
Collaborator

meshula commented Oct 3, 2022

Note this PR which moves the AAF adapter to its own repo:

#1348

@ssteinbach
Copy link
Collaborator

The AAF adapter is still in the repo, you shouldn't have to delete those docs. The CI shows errors where that doc is missing:
https://github.com/AcademySoftwareFoundation/OpenTimelineIO/actions/runs/3164460045/jobs/5152761379#step:9:360

So I'm wondering if maybe some stale local state led to needing to delete it locally? They definitely should be there still. I think everything else about this PR is good to go.

@markreidvfx
Copy link
Contributor

markreidvfx commented Oct 3, 2022

@jlskuz I can replicate the docs behavior if I mess up my local install of pyaaf2. Does python -c "import aaf2" report a error?

As part of the ongoing outlining process of contrib adapters, this removes the Kdenlive adapter from the core repository.
As discussed with the Kdenlive team (of which @vpinon, the original author, is part), the code has been moved to the KDE GitLab https://invent.kde.org/multimedia/kdenlive-opentimelineio

Related AcademySoftwareFoundation#1378

Signed-off-by: Julius Künzel <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Merging #1438 (24eaed2) into main (531edf5) will decrease coverage by 7.72%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1438      +/-   ##
==========================================
- Coverage   86.38%   78.65%   -7.73%     
==========================================
  Files         201      201              
  Lines       20854    21758     +904     
  Branches     2298     4421    +2123     
==========================================
- Hits        18014    17114     -900     
- Misses       2244     2399     +155     
- Partials      596     2245    +1649     
Flag Coverage Δ
py-unittests 78.65% <ø> (-7.73%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...elineio/opentimelineio-bindings/otio_anyVector.cpp 30.76% <0.00%> (-69.24%) ⬇️
...entimelineio-bindings/otio_serializableObjects.cpp 24.54% <0.00%> (-67.03%) ⬇️
...timelineio/opentime-bindings/opentime_bindings.cpp 33.33% <0.00%> (-66.67%) ⬇️
...eio/opentimelineio-bindings/otio_anyDictionary.cpp 33.33% <0.00%> (-66.67%) ⬇️
...imelineio/opentime-bindings/opentime_timeRange.cpp 32.39% <0.00%> (-61.36%) ⬇️
...ineio/opentime-bindings/opentime_timeTransform.cpp 42.30% <0.00%> (-57.70%) ⬇️
src/opentimelineio/stackAlgorithm.cpp 28.12% <0.00%> (-54.23%) ⬇️
...lineio/opentime-bindings/opentime_rationalTime.cpp 45.04% <0.00%> (-48.79%) ⬇️
src/opentimelineio/composition.h 40.74% <0.00%> (-48.15%) ⬇️
...melineio/opentimelineio-bindings/otio_bindings.cpp 51.58% <0.00%> (-46.71%) ⬇️
... and 60 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 531edf5...24eaed2. Read the comment docs.

@jlskuz
Copy link
Contributor Author

jlskuz commented Oct 9, 2022

@jlskuz I can replicate the docs behavior if I mess up my local install of pyaaf2. Does python -c "import aaf2" report a error?

That command worked just fine. It turned out that make tests and make doc-plugins-update got confused likely by the Kdenlive adapter and the AAF adapter I had installed separately from their new repos. After I uninstalled them and reset my OpenTimelineIO git clone, it worked properly.

I just force pushed the fix, it should be okay now.

@ssteinbach ssteinbach merged commit f24c776 into AcademySoftwareFoundation:main Oct 11, 2022
@ssteinbach
Copy link
Collaborator

Thank you!

@ssteinbach ssteinbach added this to the Public Beta 16 milestone Oct 11, 2022
MichaelPlug pushed a commit to MichaelPlug/OpenTimelineIO that referenced this pull request Aug 5, 2023
As part of the ongoing outlining process of contrib adapters, this removes the Kdenlive adapter from the core repository.
As discussed with the Kdenlive team (of which @vpinon, the original author, is part), the code has been moved to the KDE GitLab https://invent.kde.org/multimedia/kdenlive-opentimelineio

Related AcademySoftwareFoundation#1378

Signed-off-by: Julius Künzel <[email protected]>
Signed-off-by: Michele Spina <[email protected]>
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.

5 participants