After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 721342 - shout2send: Some minor cleanups
shout2send: Some minor cleanups
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal minor
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-02 15:47 UTC by Reynaldo H. Verdejo Pinochet
Modified: 2014-01-03 17:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add sample pipeline (1.21 KB, patch)
2014-01-02 15:56 UTC, Reynaldo H. Verdejo Pinochet
needs-work Details | Review
finish adding shout2send to docs (2.23 KB, patch)
2014-01-02 15:56 UTC, Reynaldo H. Verdejo Pinochet
needs-work Details | Review
clarify meaning of the URL prop (2.22 KB, patch)
2014-01-02 15:57 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
break too long line (912 bytes, patch)
2014-01-02 15:58 UTC, Reynaldo H. Verdejo Pinochet
rejected Details | Review
Warn at unsupported mimetype (Is the only field we are checking) (759 bytes, patch)
2014-01-02 15:59 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
Add and use DEFAULT_FORMAT macro (1.29 KB, patch)
2014-01-02 15:59 UTC, Reynaldo H. Verdejo Pinochet
needs-work Details | Review
change audio_format to format (Code is no longer about audio anymore) (3.24 KB, patch)
2014-01-02 16:00 UTC, Reynaldo H. Verdejo Pinochet
accepted-commit_now Details | Review
Add sample pipeline and finish adding shout2send to the docs (3.23 KB, patch)
2014-01-02 16:21 UTC, Reynaldo H. Verdejo Pinochet
accepted-commit_now Details | Review
Add sample pipeline and finish adding shout2send to the docs (3.27 KB, patch)
2014-01-02 17:29 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review
clarify meaning of the URL prop (2.27 KB, patch)
2014-01-02 17:29 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review
Warn at unsupported mimetype (Is the only field we are checking) (809 bytes, patch)
2014-01-02 17:31 UTC, Reynaldo H. Verdejo Pinochet
rejected Details | Review
change audio_format to format (Code is no longer about audio anymore) (3.86 KB, patch)
2014-01-02 17:31 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review

Description Reynaldo H. Verdejo Pinochet 2014-01-02 15:47:14 UTC
As on $Summary
Comment 1 Reynaldo H. Verdejo Pinochet 2014-01-02 15:56:28 UTC
Created attachment 265143 [details] [review]
Add sample pipeline
Comment 2 Reynaldo H. Verdejo Pinochet 2014-01-02 15:56:50 UTC
Created attachment 265144 [details] [review]
finish adding shout2send to docs
Comment 3 Reynaldo H. Verdejo Pinochet 2014-01-02 15:57:38 UTC
Created attachment 265145 [details] [review]
clarify meaning of the URL prop
Comment 4 Reynaldo H. Verdejo Pinochet 2014-01-02 15:58:08 UTC
Created attachment 265146 [details] [review]
break too long line
Comment 5 Reynaldo H. Verdejo Pinochet 2014-01-02 15:59:02 UTC
Created attachment 265147 [details] [review]
Warn at unsupported mimetype (Is the only field we are checking)
Comment 6 Reynaldo H. Verdejo Pinochet 2014-01-02 15:59:41 UTC
Created attachment 265148 [details] [review]
Add and use DEFAULT_FORMAT macro
Comment 7 Reynaldo H. Verdejo Pinochet 2014-01-02 16:00:28 UTC
Created attachment 265149 [details] [review]
change audio_format to format (Code is no longer about audio anymore)
Comment 8 Tim-Philipp Müller 2014-01-02 16:07:10 UTC
Comment on attachment 265143 [details] [review]
Add sample pipeline

merge this with the other docs patch please.
Comment 9 Tim-Philipp Müller 2014-01-02 16:07:54 UTC
Comment on attachment 265144 [details] [review]
finish adding shout2send to docs

merge this with the other docs patch please.
Comment 10 Tim-Philipp Müller 2014-01-02 16:11:10 UTC
Comment on attachment 265146 [details] [review]
break too long line

Please don't do things like this. It adds 0 value and obscures the history of that comment. If you are working on a section of code, it's ok to do something like this at the same time, but let's not start doing it just because.
Comment 11 Reynaldo H. Verdejo Pinochet 2014-01-02 16:21:02 UTC
Created attachment 265160 [details] [review]
Add sample pipeline and finish adding shout2send to the docs
Comment 12 Reynaldo H. Verdejo Pinochet 2014-01-02 16:22:23 UTC
(In reply to comment #9)
> (From update of attachment 265144 [details] [review])
> merge this with the other docs patch please.

Done, merged with 265143 (as stated above)
Comment 13 Tim-Philipp Müller 2014-01-02 16:28:58 UTC
Comment on attachment 265160 [details] [review]
Add sample pipeline and finish adding shout2send to the docs

Thanks, that's fine in principle. Please change the commit message summary line to say something about 'docs', so it's clear immediately what it's about, and also add the bug url at the bottom of the commit message. For extra points, also use uridecodebin uri=file:///path/to/audiofile instead of filesrc ! decodebin.
Comment 14 Tim-Philipp Müller 2014-01-02 16:32:58 UTC
Comment on attachment 265148 [details] [review]
Add and use DEFAULT_FORMAT macro

I don't think this adds anything really, but if you must do it, then merge with the other patch that changes the audio_format variable name to format. And why did you move the assignment?
Comment 15 Tim-Philipp Müller 2014-01-02 16:36:41 UTC
Comment on attachment 265149 [details] [review]
change audio_format to format (Code is no longer about audio anymore)

Ok. The commit message is a tad on the verbose side :) In the commit start with a lower case 'c' after the ':' please, and add the bug url at the bottom.
Comment 16 Reynaldo H. Verdejo Pinochet 2014-01-02 17:00:22 UTC
(In reply to comment #14)
> (From update of attachment 265148 [details] [review])
> I don't think this adds anything really, but if you must do it, then merge with
> the other patch that changes the audio_format variable name to format. And why
> did you move the assignment?

Would like to keep them separated if you don't mind. They
are not strictly related. I addressed all your other comments
locally (Caps on commit msgs, bug numbers, uridecodebin, etc). Would
you like me to attach new patches and obsolete the whole set?
Comment 17 Reynaldo H. Verdejo Pinochet 2014-01-02 17:05:05 UTC
(In reply to comment #14)
> [..]
> the other patch that changes the audio_format variable name to format. And why
> did you move the assignment?

Forgot to comment about that line placement. Sry. Did it cause all
the defaults are being set on the previous block, made no functional
difference and looked ordered+
Comment 18 Tim-Philipp Müller 2014-01-02 17:12:36 UTC
> Would like to keep them separated if you don't mind.

I do mind. Merge or drop. As I said, I don't think it adds or fixes anything. It's just gratuitious cleanup.
Comment 19 Reynaldo H. Verdejo Pinochet 2014-01-02 17:29:12 UTC
Created attachment 265168 [details] [review]
Add sample pipeline and finish adding shout2send to the docs
Comment 20 Reynaldo H. Verdejo Pinochet 2014-01-02 17:29:58 UTC
Created attachment 265169 [details] [review]
clarify meaning of the URL prop
Comment 21 Reynaldo H. Verdejo Pinochet 2014-01-02 17:31:04 UTC
Created attachment 265172 [details] [review]
Warn at unsupported mimetype (Is the only field we are checking)
Comment 22 Reynaldo H. Verdejo Pinochet 2014-01-02 17:31:59 UTC
Created attachment 265173 [details] [review]
change audio_format to format (Code is no longer about audio anymore)
Comment 23 Reynaldo H. Verdejo Pinochet 2014-01-02 17:33:47 UTC
(In reply to comment #18)
> [..] 
> I do mind. Merge or drop. As I said, I don't think it adds or fixes anything.
> It's just gratuitious cleanup.

Got it. Merged those two and updated the other patches
with your commit message amendment suggestions. Thanks
for your review.
Comment 24 Tim-Philipp Müller 2014-01-03 01:02:51 UTC
Comment on attachment 265172 [details] [review]
Warn at unsupported mimetype (Is the only field we are checking)

This should never happen (caps are always a subset of the template caps advertised), so don't see point in adding a warning here.
Comment 25 Tim-Philipp Müller 2014-01-03 01:03:57 UTC
Comment on attachment 265169 [details] [review]
clarify meaning of the URL prop

The comments at the ARG_FOO bit should just be removed really, but whatever.
Comment 26 Tim-Philipp Müller 2014-01-03 01:04:43 UTC
Comment on attachment 265173 [details] [review]
change audio_format to format (Code is no longer about audio anymore)

Again, please add bug url to commit message.
Comment 27 Reynaldo H. Verdejo Pinochet 2014-01-03 01:53:55 UTC
(In reply to comment #26)
> (From update of attachment 265173 [details] [review])
> Again, please add bug url to commit message.

uh? it's there already
Comment 28 Tim-Philipp Müller 2014-01-03 08:03:13 UTC
oh indeed it is, sorry :)
Comment 29 Sebastian Dröge (slomo) 2014-01-03 10:18:05 UTC
Also just do such trivial changes in the future in a single commit. "Random minor cleanup" or something like that.
Comment 30 Reynaldo H. Verdejo Pinochet 2014-01-03 14:18:58 UTC
(In reply to comment #24)
> (From update of attachment 265172 [details] [review])
> This should never happen (caps are always a subset of the template caps
> advertised), so don't see point in adding a warning here.

Oh, true :), thanks for that. Would you mind if I just wipe out the
else clause then?
Comment 31 Tim-Philipp Müller 2014-01-03 14:34:30 UTC
Please just leave it alone if there's nothing to be fixed actually.