Indicate Empty track in otioview and display track name #677
Conversation
|
That looks great! Having the labels on the side will really help. |
|
I've fixed the frame number bug. I think this should be good to go. Please review. |
Codecov Report
@@ Coverage Diff @@
## master #677 +/- ##
==========================================
- Coverage 81.69% 81.69% -0.01%
==========================================
Files 72 72
Lines 2732 2731 -1
==========================================
- Hits 2232 2231 -1
Misses 500 500
Continue to review full report at Codecov.
|
|
@jminor could you please review the PR? |
| isinstance(i, track_widgets.BaseItem) or | ||
| isinstance(i, track_widgets.Marker) or | ||
| isinstance(i, ruler_widget.Ruler) | ||
| isinstance(i, ruler_widget.Ruler) or | ||
| isinstance(i, track_widgets.TimeSlider) |
There was a problem hiding this comment.
I believe that isinstance takes a tuple of classes to check in one shot, so you can do:
isinstance(i, (track_widgets.BaseItem, track_widgets.Marker, ...))and its the same as this list
| i for i in self.scene().items() | ||
| if isinstance(i, track_widgets.BaseItem) or | ||
| isinstance(i, track_widgets.Marker) or | ||
| isinstance(i, ruler_widget.Ruler) | ||
| isinstance(i, ruler_widget.Ruler) or | ||
| isinstance(i, track_widgets.TimeSlider) |
There was a problem hiding this comment.
see below comment about passing tuples of classes to isinstance
|
|
||
|
|
||
| class BaseItem(QtWidgets.QGraphicsRectItem): | ||
| x_value = 0.0 |
There was a problem hiding this comment.
Looking at the code, I suspect that you intend x_value to be an instance rather than class variable. These are typically assigned in the constructor (__init__) method rather than at the class level, because when assigned at the class level they are share for all instance of the class. For basic types (like floats) in practice this means they're still instance specific because of the rules around how those pointers work, but its still idiomatic and better practice to assign all of these in the constructor rather than the class. This helps signals their intent. (assuming you mean this to be a member variable, of course).
|
@ssteinbach PR updated |
* master: (23 commits) Indicate Empty track in otioview and display track name (AcademySoftwareFoundation#677) FCP 7 XML - Fix failure on empty name tags (AcademySoftwareFoundation#674) xges: Effects and Markers Support (AcademySoftwareFoundation#609) Add List of Supported Formats to Conform.py Help Text (AcademySoftwareFoundation#676) Update Copyright/License on ffmpeg_burnins.py (AcademySoftwareFoundation#679) Fix the windows build (AcademySoftwareFoundation#669) Version bump to beta 13 Set final version for beta 12.0 (AcademySoftwareFoundation#665) Rodeofx fix cmx 3600 multiple markers per clip issue 593 (AcademySoftwareFoundation#664) Tweaks to cmake so that pip and local builds both work (AcademySoftwareFoundation#663) Fixed issue where CMX3600 adapter would try to add the same clip to multiple tracks. Also moved some code out of a loop it didn't need to be in. (AcademySoftwareFoundation#644) RV adapter metadata updates (AcademySoftwareFoundation#640) Expose json indent to the otio_json adapter (AcademySoftwareFoundation#641) fix otioconvert for Kdenlive with python3 (AcademySoftwareFoundation#646) Add basic debugging instructions to quickstart. (AcademySoftwareFoundation#655) Add kdenlive adapter to adapters list. (AcademySoftwareFoundation#661) Timecode rate is ignored (AcademySoftwareFoundation#612) Detect if plugin doesn't have a docstring and return an error which says which plugin it is that doesn't have the docstring. (AcademySoftwareFoundation#635) Add hook function args to otioview and otioconvert (AcademySoftwareFoundation#651) Updating Copyright notices (AcademySoftwareFoundation#660) ... # Conflicts: # contrib/opentimelineio_contrib/adapters/advanced_authoring_format.py # src/py-opentimelineio/opentimelineio/adapters/fcp_xml.py
Fixes #654
I've removed the exclusion check for empty tracks and also displayed the name of the Tracks along with the kind of track.
Here's how it looks:
After changing the code I've introduced a bug in the FrameNumber displayed on the slider. I'll fix that tomorrow.
Meanwhile I look forward to your thoughts on this.
@jminor @ssteinbach @reinecke