utils: Implement the infrastructure for custom UI widget for effect configuration
ClosedPublic

Authored by suhas2go on May 29 2017, 6:05 PM.

Details

Summary

Co-authored-by: Jean-François Fortin Tam <nekohayo@gmail.com>
Co-authored-by: Thibault Saunier <thibault.saunier@collabora.com>

Diff Detail

Repository
rPTV Pitivi
There are a very large number of changes, so older changes are hidden. Show Older Changes
suhas2go updated this revision to Diff 5932.Jun 3 2017, 11:30 PM

importlib instead of imp and other small changes

suhas2go marked 2 inline comments as done.Jun 3 2017, 11:33 PM
suhas2go added inline comments.
pitivi/utils/widgets.py
713

Ah, must be when I was porting and didn't declare self._widgets_by_keyframe_button. Will remove.

suhas2go updated this revision to Diff 5936.Jun 5 2017, 9:59 PM
suhas2go marked an inline comment as done.

Fixed reset bug dirtily

Looks good (and pretty much like my 5 years old code xD)

pitivi/utils/widgets.py
266

nitpick:

if adjustment:
    self.adjustment = adjustment
    return
549

Well, this is your own note, you should not add it here! :-)

suhas2go updated this revision to Diff 5952.Jun 13 2017, 2:24 PM

Added tests

LINTER FAILURE:

Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
autopep8 wrapper.....................................(no files to check)Skipped
Check docstring is first.................................................Passed
Debug Statements (Python)................................................Passed
Flake8...............................................(no files to check)Skipped
Validate Pre-Commit Config...........................(no files to check)Skipped
Validate Pre-Commit Manifest.........................(no files to check)Skipped
Reorder python imports...................................................Failed
hookid: reorder-python-imports

Files were modified by this hook. Additional output:

Reordering imports in tests/test_custom_effect_ui.py
suhas2go updated this revision to Diff 5953.Jun 13 2017, 2:27 PM

Fix import order

suhas2go marked 2 inline comments as done.Jun 13 2017, 2:29 PM
aleb requested changes to this revision.Jun 15 2017, 4:22 AM
aleb added inline comments.
tests/test_custom_effect_ui.py
35

Could you use instead common.create_timeline_container() and get the app from it?

This revision now requires changes to proceed.Jun 15 2017, 4:22 AM
suhas2go updated this revision to Diff 5987.Jun 26 2017, 2:11 AM

Added 'create_widget' signal

suhas2go marked an inline comment as done.Jun 26 2017, 2:13 AM
suhas2go updated this revision to Diff 5989.Jun 26 2017, 2:24 AM

minor changes

Mathieu_Du accepted this revision.Jun 27 2017, 2:36 AM

I think this is looking pretty good now, apart from the copyright change I had noted :)

tests/test_custom_effect_ui.py
4

Need to change this :)

This revision is now accepted and ready to land.Jun 27 2017, 2:36 AM

Note that should probably not be merged in the current master branch, we need to make a plan to handle the fact that master/1.0 is totally feature frozen and that new code needs to go somewhere else :-)

suhas2go updated this revision to Diff 5990.Jun 27 2017, 3:09 PM

Change copyright notice

suhas2go marked an inline comment as done.Jun 27 2017, 3:19 PM
suhas2go updated this revision to Diff 6036.Jul 11 2017, 5:21 PM

connect signal outside EffectManager

aleb requested changes to this revision.Jul 15 2017, 9:41 PM

Just some cosmetic changes:

Could you change the new strings to use only quotes, for consistency? Except maybe when you have a string which uses quotes so you don't have to escape them.

Please reformat a bit the docstrings you added:

  • the first word of the first sentence must be a verb, 3rd person
  • the first line of text must be on the same line with """, and be a complete sentence with dot at the end.
  • if there is some more text, an empty line and a new paragraph. Otherwise the final """ must also be on the first line.
pitivi/effects.py
617

Add a comment above this line to explain when can this happen.

This revision now requires changes to proceed.Jul 15 2017, 9:41 PM
suhas2go updated this revision to Diff 6201.Aug 8 2017, 10:32 PM

cosmetic changes only

This revision is now accepted and ready to land.Aug 8 2017, 10:32 PM
suhas2go updated this revision to Diff 6205.Aug 8 2017, 11:06 PM

minor cosmetic

suhas2go updated this revision to Diff 6206.Aug 8 2017, 11:21 PM

more cosmetic

suhas2go updated this revision to Diff 6250.Aug 14 2017, 7:19 PM

make wrapper for the checkbutton

suhas2go updated this revision to Diff 6254.Aug 14 2017, 7:37 PM

set checkbutton to default in this commit

suhas2go updated this revision to Diff 6263.Aug 15 2017, 12:18 AM

added get_widget_of_prop function to GstElementSettingsWidget

aleb requested changes to this revision.Aug 26 2017, 11:08 PM

Two tests fail for me:

FAIL: test_prop_reset (tests.test_custom_effect_ui.TestCustomEffectUI)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/aleb/dev/pitivi/pitivi/tests/test_custom_effect_ui.py", line 98, in test_prop_keyframe
    self.assertEqual(track_element.ui_element._TimelineElement__controlledProperty, self.prop)
AssertionError: <GParamDouble 'alpha'> != <GParamUInt 'black-sensitivity'>
FAIL: test_prop_keyframe (tests.test_custom_effect_ui.TestCustomEffectUI)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/aleb/dev/pitivi/pitivi/tests/test_custom_effect_ui.py", line 98, in test_prop_keyframe
    self.assertEqual(track_element.ui_element._TimelineElement__controlledProperty, self.prop)
AssertionError: <GParamDouble 'alpha'> != <GParamUInt 'black-sensitivity'>
tests/test_custom_effect_ui.py
95

If you use \, the next line should be indented,

109

Maybe use (default_value + 1) modulo max_value, to make sure it's different.

114

indentation

117

You should check the default value also first thing after the register_alpha_widget call, to make sure the default value is what it should be, because the

This revision now requires changes to proceed.Aug 26 2017, 11:08 PM
aleb added inline comments.Aug 27 2017, 9:38 AM
tests/test_custom_effect_ui.py
95

If you convert a dict's keys to a list, the order is not guaranteed.. Maybe add a util method which finds the proper widget based on the name/label, for example.

Same issue on line 113

suhas2go updated this revision to Diff 6442.Aug 30 2017, 6:38 AM

Fixed tests and other minor changes

This revision is now accepted and ready to land.Aug 30 2017, 6:38 AM
suhas2go marked 4 inline comments as done.Aug 30 2017, 6:50 AM
aleb requested changes to this revision.Aug 30 2017, 8:27 AM

looks good!

tests/test_custom_effect_ui.py
50

use the _cb suffix for the method name

54

quotes

This revision now requires changes to proceed.Aug 30 2017, 8:27 AM
suhas2go added inline comments.Aug 30 2017, 8:31 AM
tests/test_custom_effect_ui.py
50

Seems like I'll never get used to this :( Sorry

suhas2go updated this revision to Diff 6443.Aug 30 2017, 8:51 AM

Required minor changes

This revision is now accepted and ready to land.Aug 30 2017, 8:51 AM
aleb accepted this revision.Aug 30 2017, 8:53 AM
suhas2go marked 2 inline comments as done.Aug 30 2017, 8:54 AM
Closed by commit rPTV4f9f0b0795cb: utils: Implement the infrastructure for custom UI widget for effect… (authored by Suhas Nayak <suhas2go@gmail.com>, committed by aleb). · Explain WhySep 5 2017, 10:55 PM
This revision was automatically updated to reflect the committed changes.