GNOME Bugzilla – Bug 582774
When no video is being sent, the video output and preview should show an icon instead of being black.
Last modified: 2009-05-26 15:36:43 UTC
Currently, the call window's video output and video preview are black when no video is received or sent. Instead, the video output should show the remote contact's avatar and the video preview should show the user's avatar.
Branch containing the fix: http://git.collabora.co.uk/?p=user/jtellier/empathy.git;a=shortlog;h=refs/heads/call-video-widgets-show-icons
There are lots of code commented out, I think that patch could be a bit cleaned. I didn't read the code yet :p
Yeah, well if you read the last commit comment, you'll see that it's a work in progress and that the fix is not completed yet. I see that it brings confusion that I have posted the link to my branch before I'm finished addressing this bug. In the future, I won't post links to branches that don't contain a finished patches. Sorry about that.
I think posting links to branches that are incomplete is really useful. I would thoroughly recommend you continue doing so. However, I think you should say so in the bug report, not just in the commit message. We immediately jump to looking at the diff in gitweb and ignore the commit message as we're going to review the code anyway. After all, I made exactly the same observation as Xavier, but he beat me to commenting about it. Therefore, I think you should continue pasting links to branches (and I'm sure Xavier will agree with me) as long as you make it clear in the bug report that it's not finished yet -- just something like "work in progress". When you finish, simply follow up with another message saying "ok done pls review kthbye". Thanks for your contributions!
http://bugzilla.gnome.org/show_bug.cgi?id=580778 This seems saner to me.
I agree with Jonny Lamb, reading "Branch containing the fix" I though it was the final patch. Just write "Branch containing the work in progress" next time and it's perfect ;) @Andrea: First UI I made was doing that: the window resizes to show the video when the stream comes, and I think it was not a very good idea. I prefer keeping the UI at the same size and just display the avatar as placeholder. But that's just my opinion... What do others think about that?
*** Bug 580778 has been marked as a duplicate of this bug. ***
I agree, having the window resizing each time sending video or video preview is enabled/disabled doesn't sound like a good idea to me.
but at least, removing video when the protocol doesn't support it? (gtalk?) having a big window when it would not work... it's not so smart, but of course having less code to maintain is better :) (without special cases for each protocol...). I'm for reducing the window, then, starting from placing the preview over the incoming video as in your bugreport.
Created attachment 135134 [details] [review] I would have liked to be able to do more tests, but video calls work really poorly for me. As I result, I have been unable to perform every test cases I could think of. I hope everything works fine. src/empathy-call-window.c | 115 ++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 109 insertions(+), 6 deletions(-)
If the contact doesn't have an avatar the call-window looks really weird. You should display a default icon in that case (the same as on the avatar selector button when no avatar is set).
Created attachment 135207 [details] [review] Fixed and improved libempathy-gtk/empathy-video-widget.c | 4 +- libempathy-gtk/empathy-video-widget.h | 3 + src/empathy-call-window.c | 144 +++++++++++++++++++++++++++++++-- 3 files changed, 141 insertions(+), 10 deletions(-)
- empathy_call_window_constructed: the EmpathyTpContactFactory is leaked
You can probably squash the 2 first patches and re-record them with a better commit msg too.
Branched fixed: http://git.collabora.co.uk/?p=user/jtellier/empathy.git;a=shortlog;h=refs/heads/call-video-widgets-show-icons
Created attachment 135327 [details] [review] Latest patch libempathy-gtk/empathy-video-widget.c | 4 +- libempathy-gtk/empathy-video-widget.h | 3 + src/empathy-call-window.c | 146 +++++++++++++++++++++++++++++++-- 3 files changed, 143 insertions(+), 10 deletions(-)
Your call-video-widgets-show-icons branch looks good. Please merge
Merged to master. Thanks for the branch!