widgets: Custom validation checks for GstElementSettingsWidget fields
ClosedPublic

Authored by stefanzzz on Jul 25 2017, 2:47 PM.

Details

Summary

Added a mechanism which allows custom validation checks on the inputs
of a GstElementSettingsWidget. Also, a validation check for the
multipass-cache-file property of the x264enc encoder was added.

Fixes T7580.

stefanzzz created this revision.Jul 25 2017, 2:47 PM
aleb added inline comments.Jul 25 2017, 3:08 PM
pitivi/render.py
876 ↗(On Diff #6114)

This should be removed?

stefanzzz updated this revision to Diff 6115.Jul 25 2017, 6:44 PM
stefanzzz retitled this revision from render: Recover properly when rendering fails. to render: Fix x264enc encoding crash..
stefanzzz edited the summary of this revision. (Show Details)

Make sure the "Multipass cache file" property is valid.

thiblahute requested changes to this revision.Jul 25 2017, 7:15 PM
thiblahute added inline comments.
pitivi/utils/widgets.py
912

This should come with a mechanism where we list the properties for the various elements and have specific check on them,
also making sure there is no collision (I doubt it is a probleme in this case though!).

Also add some debug logging, this kind of things needs to be 'simple' to catch when debugging later on.

This revision now requires changes to proceed.Jul 25 2017, 7:15 PM
aleb requested changes to this revision.Jul 25 2017, 9:41 PM

There seem to be two fixes in the same commit, one of them a one-liner - could you split them?

pitivi/utils/misc.py
112

If you open it, it should be closed

pitivi/utils/widgets.py
183

Could you please clean up a bit this method?

Here it should be "if" instead of "elif", and then the block above can be simplified: move the validity checks to an _is_text_valid method and it should look much better. No need to have the if self.matches or self.valid_filename check anymore, we'll always check for validity..

914

valid_filename=filename can look confusing. Better rename the local variable and use valid_filename=valid_filename

thiblahute added inline comments.Jul 26 2017, 8:49 PM
pitivi/render.py
872 ↗(On Diff #6115)

I do not think this is correct, here we are shutting down the pipeline, to go back to PREVIEW mode, we should not go back to PAUSED meanwhile.

stefanzzz added inline comments.Jul 27 2017, 6:52 AM
pitivi/render.py
872 ↗(On Diff #6115)

I noticed that going to NULL and then changing to PREVIEW mode after a pipeline error crashes. If instead we do: NULL, PAUSED, NULL, change to PREVIEW mode, it works fine. Btw, set_mode automatically sets the pipeline to NULL.

stefanzzz added inline comments.Jul 28 2017, 9:06 AM
pitivi/render.py
872 ↗(On Diff #6115)

I think this won't be needed anymore. I can't reproduce the app crash after updating GES. It seems like this commit fixed the issue. Can you still reproduce?

thiblahute added inline comments.Jul 30 2017, 12:46 AM
pitivi/render.py
872 ↗(On Diff #6115)

heh, funny, but indeed it makes sense that fixes it. I do not remember what bug I was fixing when writting it, but it is for sure correct.

Please remove.

stefanzzz updated this revision to Diff 6180.Jul 31 2017, 10:38 AM
stefanzzz marked 8 inline comments as done.
stefanzzz retitled this revision from render: Fix x264enc encoding crash. to widgets: Custom validation checks for GstElementSettingsWidget fields.
stefanzzz edited the summary of this revision. (Show Details)

Added a mechanism which allows custom checks on element properties.

pitivi/utils/widgets.py
183

I changed my approach and there are no modifications in this class anymore.

aleb requested changes to this revision.Aug 12 2017, 12:37 AM

Interesting!

pitivi/utils/widgets.py
613

instead of 10 use SPACING

747

nitpick: You could replace this with widget = prop_widget, so that widget is not first set to a significant value (line 738) and then changed again just below (line 744).

764

prop_widget

768

prop_widget

845

This is not used - remove it

This revision now requires changes to proceed.Aug 12 2017, 12:37 AM
stefanzzz updated this revision to Diff 6249.Aug 14 2017, 1:39 PM
stefanzzz marked 5 inline comments as done.

Small changes.

Aleb Could you check this one and merge it? It should get in.

This revision was automatically updated to reflect the committed changes.