GNOME Bugzilla – Bug 703753
osxvideosink: remove legacy code for passing a windows ID
Last modified: 2014-06-28 08:32:42 UTC
Created attachment 248556 [details] [review] osxvideosink: remove legacy code for passwing a windows ID "have-ns-view" and the "embed" property was kept in 0.10 for backwards compatibility but it's no longer used in favor of the GstVideoOverlay interface.
Unfortunately this was already in a stable 1.0 gst-plugins-good release... I think it's ok to remove now still but others might disagree.
This sink shouldn't even be in good :) One can argue it breaks the element's API, but it's also bad API that should never be used.
I'm a bit undecided about this. Unless the element didn't work at all anyway, we shouldn't just remove it. On the other hand, the property clearly said 'don't use this'. Perhaps we can as a compromise phase it out more gracefully, e.g. add a g_warning() for now, then remove it completely in 1.3.x ?
Fine with me, yes. I'm also fine with just removing it immediately though, nobody should've ever used this in 1.0. And I agree that this sink should've never been in good ;)
Created attachment 248737 [details] [review] 0001-osxvideosink-remove-legacy-code-for-passing-a-window.patch I would remove it completely too. I have attached a new version of the patch with a g_warning in the setter.
Created attachment 248738 [details] [review] 0001-osxvideosink-remove-legacy-code-for-passing-a-window.patch Adding the correct file :)
Review of attachment 248738 [details] [review]: ::: sys/osxvideo/osxvideosink.m @@ +457,3 @@ switch (prop_id) { case ARG_EMBED: + g_warning ("This property will be deprecated in 1.3"); apps would call this as a result of the message, the message is not sent anymore so... :) Keep the functionality for now and just print a warning. Also should be 1.4, 1.3 will be just a development release.
So adding a g_warning to the setter only... but that's like doing nothing because the property's description already warns about not using it :)
Not sure, that would add an explicit warning that happens during runtime at least :) Tim?
Andoni: true, but the thing is no one checks the description unless they write new code, so people who have working code that they just kept using aren't going to check if the property is still there and what the description says. So I think a run-time warning is in order before removing it. If I may suggest a wording: g_warning ("The \"embed\" property of osxvideosink is deprecated and will be removed in the near future. Use the GstVideoOverlay interface instead.");
*** Bug 704342 has been marked as a duplicate of this bug. ***
Andoni, any news on this? Can you attach a new patch?
Created attachment 249394 [details] [review] 0001-osxvideosink-warn-about-the-future-deprecation-of-th.patch
Comment on attachment 249394 [details] [review] 0001-osxvideosink-warn-about-the-future-deprecation-of-th.patch Ok, please commit that. And the original patch after 1.3 is open (so keep this bug open)
Pushed: commit fbafca49f818666327bf43265de6b03af873955a Author: Andoni Morales Alastruey <ylatuya@gmail.com> Date: Thu Jul 11 19:45:17 2013 +0200 osxvideosink: warn about the future deprecation of the "embed" property
Can we close this now that the warning commit is pushed ? Or should we re-target it to 1.4/blocker to make sure we remove the code when that time comes ?
Keep open and remove the code in 1.3.x
commit 93653ae5f983d670fa5c24d16da42bbc3ec1e80d Author: Andoni Morales Alastruey <ylatuya@gmail.com> Date: Sun Jul 7 20:18:27 2013 +0200 osxvideosink: remove legacy code for passing a window handle "have-ns-view" and the "embed" property was kept in 0.10 for backwards compatibility but it's no longer used in favor of the GstVideoOverlay interface https://bugzilla.gnome.org/show_bug.cgi?id=703753