GNOME Bugzilla – Bug 721342
shout2send: Some minor cleanups
Last modified: 2014-01-03 17:26:14 UTC
As on $Summary
Created attachment 265143 [details] [review] Add sample pipeline
Created attachment 265144 [details] [review] finish adding shout2send to docs
Created attachment 265145 [details] [review] clarify meaning of the URL prop
Created attachment 265146 [details] [review] break too long line
Created attachment 265147 [details] [review] Warn at unsupported mimetype (Is the only field we are checking)
Created attachment 265148 [details] [review] Add and use DEFAULT_FORMAT macro
Created attachment 265149 [details] [review] change audio_format to format (Code is no longer about audio anymore)
Comment on attachment 265143 [details] [review] Add sample pipeline merge this with the other docs patch please.
Comment on attachment 265144 [details] [review] finish adding shout2send to docs merge this with the other docs patch please.
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.
Created attachment 265160 [details] [review] Add sample pipeline and finish adding shout2send to the docs
(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 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 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 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.
(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?
(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+
> 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.
Created attachment 265168 [details] [review] Add sample pipeline and finish adding shout2send to the docs
Created attachment 265169 [details] [review] clarify meaning of the URL prop
Created attachment 265172 [details] [review] Warn at unsupported mimetype (Is the only field we are checking)
Created attachment 265173 [details] [review] change audio_format to format (Code is no longer about audio anymore)
(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 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 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 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.
(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
oh indeed it is, sorry :)
Also just do such trivial changes in the future in a single commit. "Random minor cleanup" or something like that.
(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?
Please just leave it alone if there's nothing to be fixed actually.