Re: [Musicpd-dev-team] [git pull] mixer pulse
On 2009/01/20 21:29, Linel Patrice patnathan...@gmail.com wrote: I recoded the mixer , in a way it does not change the existing output pulse. git://git.musicpd.org/pat/mpd.git You should only need to pick the last commit. I cannot pick the last commit, because it is a merge commit. Don't merge, rebase on master instead (git pull --rebase or stg rebase). If you need any help with git usage, you're welcome to ask. There are two patches with identical description, but with a different subject (9218b2e3a and b354929). I don't understand the description at all. There are more patches, most of which I don't understand. e.g. 4fe20df is described as envoi pr david ??? The patch Begin implementation for pulse mixer (d8efc9e0) looks pointless, because it only adds some commented code. Your code doesn't compile with --enable-werror (always enable this option during development!): cc1: warnings being treated as errors mixer/pulse_mixer.c:22: error: unused parameter 'context' mixer/pulse_mixer.c:44: error: unused parameter 'context' mixer/pulse_mixer.c: In function 'subscribe_cb': mixer/pulse_mixer.c:61: error: declaration of 'index' shadows a global declaration /usr/include/string.h:309: error: shadowed declaration is here mixer/pulse_mixer.c: At top level: mixer/pulse_mixer.c:61: error: unused parameter 'c' mixer/pulse_mixer.c: In function 'context_state_cb': mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_UNCONNECTED' not handled in switch mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_CONNECTING' not handled in switch mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_AUTHORIZING' not handled in switch mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_SETTING_NAME' not handled in switch mixer/pulse_mixer.c: In function 'pulse_mixer_configure': mixer/pulse_mixer.c:160: error: unused variable 'ret' mixer/pulse_mixer.c:159: error: unused variable 'o' mixer/pulse_mixer.c: In function 'pulse_mixer_open': mixer/pulse_mixer.c:194: error: unused variable 'o' mixer/pulse_mixer.c:193: error: unused variable 'pm' mixer/pulse_mixer.c: In function 'pulse_mixer_close': mixer/pulse_mixer.c:208: error: unused variable 'pm' make[2]: *** [mpd-pulse_mixer.o] Error 1 More formal stuff: try to keep lines less than 80 characters long. Exceptions are allowed when every other solution is even less readable. That's not the case in pulse_mixer.c, please break those overlong lines. Try to keep MPD's code style. Why a closure for each case statement? pulse_mixer.c has lots of whitespace errors (spaces at the end of the line). Your code doesn't have proper error handling: if(!(pm-mainloop = pa_threaded_mainloop_new())) g_debug(Failed mainloop\n); if(!(pm-context = pa_context_new(pa_threaded_mainloop_get_api(pm-mainloop),Mixer mpd))) g_debug(Failed context\n); pa_context_set_state_callback(pm-context, context_state_cb, pm); So you check the result, but continue to work with NULL pointers. This will crash! I need some feedback on that please. It still not work in daemon. What do you mean, it does not work? Do not send a pull request if you know it doesn't work - we can talk about your code, but I'll pull only when every single patch you submit works and is self-contained, and doesn't introduce a regression. It's important that every commit works (at the best of our knowledge), because a broken commit would make bisecting hard or impossible. Please describe what exactly does not work. Max -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] [git pull] mixer pulse
Max Kellermann wrote: On 2009/01/20 22:51, Linel Patrice patnathan...@gmail.com wrote: This is an intermediate commit to do a push on another server. Don't submit intermediate commits. Having intermediate commits in the official repository may break bisections. A patch should be complete and self-contained, i.e. it improves MPD, and doesn't introduce a regression. If you fix bugs in a patch which isn't merged yet, fold the fixes into the existing patch. The patch Begin implementation for pulse mixer (d8efc9e0) looks pointless, because it only adds some commented code. Yes, but it is an old one , you want me to remove it ? Yes. I see no reason to merge this commit, it doesn't improve MPD. What does not work, is when i launch mpd without --no-daemon it fails to get the volume on the mixer. So I don't understand why. Will you be able to find out? Or do you need help? I will try , and I tell you know. Do you want me to rewrite all my commit before doing the rebase? I don't know if it makes sense to split the pulse mixer into separate patches. So the easiest way to get your code merged would be to copy your pulse_mixer.c into a new clone of the master repository, and only apply the Makefile.am changes. This way, you don't have to go through all the rebasing/merging pain again. Ok i'll do this after fixing the warnings. You prefer not to have a empty function (solving the certain unused variable), or have the warning? There are some of the warning ,I can not fix cause it is a callback function of pulse. I will never get tired of telling people to use stgit! stgit allows you to refine your patches until they are complete, without creating tons of useless non-self-cointained intermediate patches. Max -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
Re: [Musicpd-dev-team] [git pull] mixer pulse
Max Kellermann wrote: On 2009/01/20 21:29, Linel Patrice patnathan...@gmail.com wrote: I recoded the mixer , in a way it does not change the existing output pulse. git://git.musicpd.org/pat/mpd.git You should only need to pick the last commit. I cannot pick the last commit, because it is a merge commit. Don't merge, rebase on master instead (git pull --rebase or stg rebase). If you need any help with git usage, you're welcome to ask. There are two patches with identical description, but with a different subject (9218b2e3a and b354929). I don't understand the description at all. It is the merge commit, i thought i had to rewrite it. There are more patches, most of which I don't understand. e.g. 4fe20df is described as envoi pr david ??? This is an intermediate commit to do a push on another server. The patch Begin implementation for pulse mixer (d8efc9e0) looks pointless, because it only adds some commented code. Yes, but it is an old one , you want me to remove it ? Your code doesn't compile with --enable-werror (always enable this option during development!): cc1: warnings being treated as errors mixer/pulse_mixer.c:22: error: unused parameter 'context' mixer/pulse_mixer.c:44: error: unused parameter 'context' mixer/pulse_mixer.c: In function 'subscribe_cb': mixer/pulse_mixer.c:61: error: declaration of 'index' shadows a global declaration /usr/include/string.h:309: error: shadowed declaration is here mixer/pulse_mixer.c: At top level: mixer/pulse_mixer.c:61: error: unused parameter 'c' mixer/pulse_mixer.c: In function 'context_state_cb': mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_UNCONNECTED' not handled in switch mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_CONNECTING' not handled in switch mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_AUTHORIZING' not handled in switch mixer/pulse_mixer.c:92: error: enumeration value 'PA_CONTEXT_SETTING_NAME' not handled in switch mixer/pulse_mixer.c: In function 'pulse_mixer_configure': mixer/pulse_mixer.c:160: error: unused variable 'ret' mixer/pulse_mixer.c:159: error: unused variable 'o' mixer/pulse_mixer.c: In function 'pulse_mixer_open': mixer/pulse_mixer.c:194: error: unused variable 'o' mixer/pulse_mixer.c:193: error: unused variable 'pm' mixer/pulse_mixer.c: In function 'pulse_mixer_close': mixer/pulse_mixer.c:208: error: unused variable 'pm' make[2]: *** [mpd-pulse_mixer.o] Error 1 More formal stuff: try to keep lines less than 80 characters long. Exceptions are allowed when every other solution is even less readable. That's not the case in pulse_mixer.c, please break those overlong lines. Try to keep MPD's code style. Why a closure for each case statement? pulse_mixer.c has lots of whitespace errors (spaces at the end of the line). Your code doesn't have proper error handling: if(!(pm-mainloop = pa_threaded_mainloop_new())) g_debug(Failed mainloop\n); if(!(pm-context = pa_context_new(pa_threaded_mainloop_get_api(pm-mainloop),Mixer mpd))) g_debug(Failed context\n); pa_context_set_state_callback(pm-context, context_state_cb, pm); So you check the result, but continue to work with NULL pointers. This will crash! I need some feedback on that please. It still not work in daemon. What do you mean, it does not work? Do not send a pull request if you know it doesn't work - we can talk about your code, but I'll pull only when every single patch you submit works and is self-contained, and doesn't introduce a regression. It's important that every commit works (at the best of our knowledge), because a broken commit would make bisecting hard or impossible. Please describe what exactly does not work. Max What does not work, is when i launch mpd without --no-daemon it fails to get the volume on the mixer. So I don't understand why. Do you want me to rewrite all my commit before doing the rebase? -- This SF.net email is sponsored by: SourcForge Community SourceForge wants to tell your story. http://p.sf.net/sfu/sf-spreadtheword ___ Musicpd-dev-team mailing list Musicpd-dev-team@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team
