Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PNMixer rewrite #120

Closed
elboulangero opened this issue Jan 21, 2016 · 23 comments
Closed

PNMixer rewrite #120

elboulangero opened this issue Jan 21, 2016 · 23 comments

Comments

@elboulangero
Copy link
Collaborator

I believe PNMixer needs a rewrite.

PNMixer history

Let's recall a little bit PNMixer history (as far as I know it).

First came volumecontrol by Rob Eberts. Never found the source for this one.

Then, built on top of that, AbsVolume from Paul Sherman. I managed to find the source of AbsVolume on the net, but I'm not sure if it's the official source, or some modified version that run on some specific distro. Initial release is from 2006. Then it was modified in 2007 by a guy named Will Davies (this is stated in the README file). Let me quote what I found in the main.c.

WARNING do not try and learn to program from reading this code
I have hacked the original extensively and all the bad programming is mine.

Sweet, isn't it ?

Next came OBMixer by Lee Ferrett. Let me quote the comments from main.c.

OBmixer was programmed by Lee Ferrett, derived
from the program "AbsVolume" by Paul Sherman.

So, we don't know if it was build on top of an official clean AbsVolume version, or if it was built on the "extensively hacked" version.

On top of that finally came PNMixer by @nicklan (Nick Lanham).

So it seems like a long and turbulent story, more than 10 years long ! And when I play with the code, this lively life of PNmixer shows. It shows big deal. Let's state it: PNMixer code is a mess. Global variables everywhere, every files are included in every other files, no namespace... Every time I change anything, it's like adding a card to a house of cards. I prey that it won't crumble down.

PNMixer today

Ok, nowadays PNMixer is maintained mainly by @hasufell and @elboulangero (myself).

I met PNMixer when I started using the Debian-based CrunchBang distro (now dead and replaced by BunsenLabs), which included PNMixer as the default mixer. Somehow, I came to like the little mixer, fixed a few things and started to get involved.

But to be honest, I'm not really interested in maintaining the code in its current state. I've spent some time doing that, it's wasted time. In my opinion, the code has been let rusting for a long time, and now it's really in a bad state.

PNMixer rewriting

That's what I've been wanting to do from the very beginning, when I started with PNMixer 2 years ago roughly. But event for a small piece of software, rewriting takes time, time I never had.

But it just happens that today I have this time. So I started a full rewrite of the code.

I'm trying to make a clean code, something that everybody enjoys maintaining. Well-thought, intuitive, elegant. I don't pretend I achieve this goal, but that what aim for.

OK, so here it is, you can find the work in progress on the branch pnmixer-rewrite-dirty.

Looking at the diff is a waste of time, it's so big that it doesn't make sense. Looking at the commits is also pointless. If you're interested, just have a look at how the code look now, and give feedback. Tell me if you're happy with it, or shocked to see such a change.

@SilverRainZ
Copy link
Contributor

I am glad to see that as a pnmixer user,
and I will be concerned about this branch.

if Chinese Translations need to be updated, welcome to @ me.

@elboulangero
Copy link
Collaborator Author

Thanks, indeed there will be a need to update translation. For the next version I would like to handle the translations through the Translation Project. Ever heard about it ?

I've been in touch with the guys when we released the previous version. We almost worked with them, but in the end it didn't happen, because I was a little bit messy and asked too many people to translate PNMixer at the same time :)

But for the next version it will be different, they will probably translate it.

As a previous translator, you can join the Translation Project if you're interested, and keep on translating PNMixer from the TP. I guess it will be pretty much the same for you, except that you have to take some time to learn what's the workflow of the TP, how it works, etc...

You can also decide that you don't care anymore, and just let someone else do the translation. Hopefully someone in the TP translate in chinese.

At last, you can also insist on translating PNMixer by yourself, not joining the TP. Since you're the owner of the translation, I can't refuse, I respect your choice in that matter.

Needless to say, I'd prefer you to make choice number one :) The whole point of the TP is to ease the translation process for me. Last time I had to contact the previous translator, half of the email addresses I had didn't work anymore. I had to look in the po files, and also in the git commits, to find different addresses for the same person. One of the guy replied suddenly after two months without a word. In the end PNMixer was translated, and I thank all of the translators, but it was complicated. I hope with the TP it's easier.

@hasufell
Copy link
Collaborator

For a rewrite I'd actually ask myself two additional questions:

  • do I even want this to be coded in C?
  • do I even want to use Gtk+ as a toolkit?

@nicklan
Copy link
Owner

nicklan commented Jan 21, 2016

Yes, this little project has ended up generating a lot more interest than I ever expected, and huge thank you's to @hasufell and @elboulangero for keeping it alive. My time is pretty monopolized by grad-school, so I really don't have time for much input. I agree the code is not pretty. Standard disclaimers aside (new bugs etc) a re-write is probably not a bad idea.

I would take the time to consider what the goals are. Some things to consider:

  • Does c/gtk+ make sense (mentioned by Julian). I still mostly like gtk, but the project has been somewhat hostile to developers that don't share their vision. libappindicator is a mess, so there is basically no good tray-icon story for gtk. I have no idea what the state is in something like QT, or maybe even FLTK. That said, a lot of people already use the gtk version, and it fits into a desktop environment well, so it's probably a decent choice.
  • In relation to the above, would it make sense to start a separate project for a decent tray-icon library for gtk? I bet there are other projects that would love this, and it could end up being quite useful outside of the pnmixer world. On the other hand, it's probably a decent amount of work.
  • pulseaudio support. This is a big lacking feature for me in pnmixer at the moment. The ability to recognize pulse sinks and control individual volumes would be great. I would at least consider in the new architecture keeping things flexible enough to handle this (i.e. don't assume there's only one universal volume to control like pnmixer mostly does now). At the moment adding pulse support would require a huge amount of re-engineering, so during this re-write would be a great time to do it.

Thanks again for keeping this project going, and I'll try and chime in with input and maybe even code as time allows!

Cheers,

Nick

On Thu, 21 Jan 2016 06:22:22 -0800,
El Boulangero wrote:

[1 <text/plain; UTF-8 (7bit)>]

[2 <text/html; UTF-8 (7bit)>]
PNMixer history

Let's recall a little bit PNMixer history (as far as I know it).

First came volumecontrol by Rob Eberts. Never found the source for this one.

Then, built on top of that, AbsVolume from Paul Sherman. I managed to find the source of AbsVolume on the net, but I'm not sure if it's the official
source, or some modified version that run on some specific distro. Initial release is from 2006. Then it was modified in 2007 by a guy named Will Davies
(this is stated in the README file). Let me quote what I found in the main.c.

WARNING do not try and learn to program from reading this code
I have hacked the original extensively and all the bad programming is mine.

Sweet, isn't it ?

Next came OBMixer by Lee Ferrett. Let me quote the comments from main.c.

OBmixer was programmed by Lee Ferrett, derived
from the program "AbsVolume" by Paul Sherman.

So, we don't know if it was build on top of an official clean AbsVolume version, or if it was built on the "extensively hacked" version.

On top of that finally came PNMixer by @nicklan (Nick Lanham).

So it seems like a long and turbulent story, more than 10 years long ! And when I play with the code, this lively life of PNmixer shows. It shows big deal.
Let's state it: PNMixer code is a mess. Global variables everywhere, every files are included in every other files, no namespace... Every time I change
anything, it's like adding a card to a house of cards. I prey that it won't crumble down.

PNMixer today

Ok, nowadays PNMixer is maintained mainly by @hasufell and @elboulangero (myself).

I met PNMixer when I started using the Debian-based CrunchBang distro (now dead and replaced by BunsenLabs), which included PNMixer as the default mixer.
Somehow, I came to like the little mixer, fixed a few things and started to get involved.

But to be honest, I'm not really interested in maintaining the code in its current state. I've spent some time doing that, it's wasted time. In my opinion,
the code has been let rusting for a long time, and now it's really in a bad state.

PNMixer rewriting

That's what I've been wanting to do from the very beginning, when I started with PNMixer 2 years ago roughly. But event for a small piece of software,
rewriting takes time, time I never had.

But it just happens that today I have this time. So I started a full rewrite of the code.

I'm trying to make a clean code, something that everybody enjoys maintaining. Well-thought, intuitive, elegant. I don't pretend I achieve this goal, but
that what aim for.

OK, so here it is, you can find the work in progress on the branch pnmixer-rewrite.

â€> Reply to this email directly or view it on GitHub.*

@elboulangero
Copy link
Collaborator Author

Hi everyone, thanks for your replies.

First, let me tell you that I'm pretty happy with PNMixer as I see it as an user. I'm also pretty happy with the current technologies it uses, that's to say C language & Gtk toolkit. The only thing I'm concerned is the way the code is written. This is my only problem, and it just happens that I like to design software. I like to think it, split the problem in different smaller problems, solve them separately in different pieces of code, and come up with a nice solution in the end. Code that is clearly written, easy to read and understand, and runs smooth. Code so obvious that you barely need to comment it.

Alright, now so you know me better, let me give my point of view on the points you mentioned above :)

Technical points

do I even want this to be coded in C?

I don't really mind which language. The only thing is that, in the process right now, I'm not rewriting PNMixer from scratch (which would allow me to pickup another language), I'm moving and reshaping the current code. So I stick to the current language. But even if I started again from zero, I would still use C, simply because it's the language I'm fluent in, and my current goal is just to improve PNMixer in a reasonable amount of time, not to learn a new language.

So yeah, I stick with C, but I don't pretend it's the best choice. It's just my choice.

do I even want to use Gtk+ as a toolkit?
Does c/gtk+ make sense

I like Gtk. As a user, I like the look & feel. As a developer, I'm still learning it, with PNMixer mostly. But the more I know the more I like it. Up to know I find the library consistent and comfortable to use, the documentation is good. This being said, I must admit we face some problems here and there. GtkStatusIcon (#81, #87, #71) is the biggest. But there's also other little things. Lately a useless margin appeared in the popup menu, and there's no apparent way to remove it (BTW we should create a ticket about that, just to keep track of our complaints against gtk3). Also, there's a problem with the volume slider and keyboard shortcuts, especially if the slider is horizontal (https://bugzilla.gnome.org/show_bug.cgi?id=407242). The most annoying is that it's something outstanding (at least to me), but apparently the bug has been there for almost ten years an not fixed yet. Not event someone from Gtk3 dropped a line explaining why is the problem, why it's difficult to solve, if it will be solved at all. People drop patches, but nothing happens. This is what I dislike the most with Gtk. You find a bug, analyse it, make the best bug report you can, and ten years after you're still waiting. I don't blame anybody on that. But this is the kind of thing that would make me move away to another toolkit, where you have people to speak to, who are willing to improve their library. So that when you face a problem, you can solve it with the help of the maintainers. This is why I like open-source. Sharing and improving the whole stuff for everyone. Alright, this being said, I have another example where someone from Gtk actually solved a bug I reported https://bugzilla.gnome.org/show_bug.cgi?id=748761. So, it happens. It's just that you have no clue when. May be never.

Ok, to sum that up, I kind of like Gtk3, but don't deny that there are some issues. I never really worked seriously with any graphical toolkit before, PNMixer was the opportunity to learn one. It just happens that it was Gtk, it could have been anything else. Nowadays I still have a lot to learn from it, so I'm happy to stick with it.

In relation to the above, would it make sense to start a separate project for a decent tray-icon library for gtk?

Definitely agree with that. But I won't be the one doing that sorry. It's a big amount of work. You would have to learn GtkStatusIcon by heart, understand perfectly what are the strong points of it, and also where are the flaws. Know about the long-time bugs that were never solved due to initial design mistakes. Maybe some people involved with it already know that, but I don't, so just this part would already take a fair amount of time. Then come up with an new GtkStatusIcon, but better than the previous, cause if you reproduce the same mistakes there's no point doing it, right ? Furthermore, maintaining a library means keeping the API, it's something I never did before.

Apart from that, it's just my personal life. At the moment I spend a lot on time of computer, doing stuff I wanted to do for a long time. But after that, in a few months, I'm done with it and I'll do something else. I'll keep in touch for maintenance, PNMixer and other things. But I won't spend so much time on it.

So yeah, coming up with a good tray icon library is definitely something needed. There's a real need for it, and I'm pretty sure that someone will jump in. If you think about it, it's a good opportunity to have your little share in the open-source world. I'm sure plenty of people are waiting for it, so if you do it and you do it good, you will be the maintainer of a fairly popular library. Sounds like success, doesn't it ? 😄

pulseaudio support.

Yep, this is also something I've been thinking about. Pulse Audio support and OSS support maybe. The fact is, I don't need that. I'm not going to install PulseAudio on my laptop just to implement it in PNMixer, then uninstall it. The guy who implements it must also be the guy who maintains it, meaning he's a regular PulseAudio and PNMixer user. I'm not this guy. But I believe that PNMixer would be a better and more useful software if it could support PulseAudio. We would have more happy users.

OK, so I won't do it. But at least I can prepare the code a little bit for that. Because in the current state of things, this is impossible, the code is too messy. But for the new code I have that in mind. So I'll do my best to isolate the ALSA code in one file, so that adding PulseAudio just boils down to adding a new file with PulseAudio related code.

Upcoming design

At the moment, I created an audio.h file. The goal is that all the code changes audio through the audio.h API, so that it doesn't deal with any ALSA specific code. It's just a simple thing at the moment, but we need that level of abstraction on top of the low-level audio library.

Then, when I'll rework the ALSA code in a few days, I'll make it more "signal oriented". I think the whole code will benefit from that, in term or readability and maintainability. What I want is that the ALSA code don't call anything explicitly, like it does now, when the volume/mute state is changed, but just send Glib signals instead. Because ALSA is low-level part of PNMixer, it shouldn't include anything high-level like the UI. It should be completely isolated and shouldn't include any other files, except ALSA library.

When this is done, audio code and UI code will be completely separated entities. UI will access ALSA through the audio.h abstraction layer, meaning that it should be easy to change the underlying audio library, may it be pulse or OSS. On the other hand, UI should also be easy to rewrite using another library, it won't impact the audio code which don't know about the UI anyway.

Future for PNMixer

OK, so I hope you understand my point. I rewrite PNMixer just for the fun of it, and the pleasure to have a code elegant, well designed and flexible. I however don't change any technologies. But I try to make it ready for such changes to be possible.

So the PNMixer I'll come up with in some days won't be the end of it, but just a new base from where to build upon.

After that, changing toolkit, we can discuss, I'm not especially against it or in favor of it. But I probably won't be very much involved in that since I'm happy with the current Gtk, and I won't be the one asking for that change.

Changing language, looks quite radical to me. If it's a language we all speak, maybe we can do that and keep on being the same guys maintaining it. But as I said, I mainly speak C, so I think you will go on without me if you rewrite in another language. But I have no problem with that :)

All right, I think you're exhausted after reading such a long text ! I am after writing it :) Just to finish, thanks to you @nicklan for allowing us to maintain PNMixer. It's a nice experience and I enjoy it.

Cheers

Elbo

@SilverRainZ
Copy link
Contributor

@elboulangero I am no a professional translator and not very good at English, Translation Project looks like professional translate organization and require people join before doing translation.
I'm afraid I can not join it well because of my poor translation skill.
I also have not much time to deal with it.

But, how to handle translateion still depends on you, although if you choice TP, I may have no way to contribute translation.

@elboulangero
Copy link
Collaborator Author

Hey, when I contacted them I also felt that they wouldn't be interested, because PNMixer is a little humble piece of code, and they looked quite professional. But in fact the people I spoke to was really, how to say, easy-going. He took time to discuss the matter with me, and he was willing to do the translation.

So if you want to know more about them, don't be shy, just get in touch ;) Don't worry about your English. And I think that also they are quite flexible. If you join the TP project, but just want to translate PNMixer, I think they're OK. Well, at least that's what I understood from the previous conversation I had with the TP.

@hasufell
Copy link
Collaborator

So, I personally would be more interested to rewrite pnmixer in rust while still using Gtk+. There are not many programming languages that open you up to more programming errors like C. Rust introduces memory safety and gives you a lot more high-level tools to deal with code and improve maintainability, while still being pretty low-level compatible and making interfacing with C libraries easy. It already has bindings for Gtk+ and Alsa. Ofc that's all very fancy and will require learning this language. But pnmixer seems like a good fit to do exactly that.

@ghost
Copy link

ghost commented Feb 4, 2016

I, myself, also came to pnmixer through #!, which I have now added BunsenLabs and regular Debian to in the sources.list since #! stopped. OpenBox is my primary WM, with a Tint2 taskbar.
Please try to retain old functionality where you can; I listed an issue with the new icon for one.

@elboulangero
Copy link
Collaborator Author

Hi everyone !

I finished the PNMixer complete rewrite. It's available on my PNMixer fork, here:
pnmixer-rewrite-dirty. Check it out and give it a try !

SO what's next ? Well I'm eager to merge that into PNMixer and make a release to have people using it asap, so that little bugs here and there can be fixed quickly while everything is fresh in my mind :)

I don't really expect anyone to read the code and make comments, I suppose you have other things to do. But if you feel like it, it will be appreciated. Otherwise, just little test and feedback, would be great.

From a user point of view, of course there's no change. We're just chasing bugs here :)

There's still a little bit of work to be done, documentation is not completely finished. Btw, that's a point that I wanted to share with you. Do you guys out there really like/use Doxygen ? I personnaly never used it before. While rewriting PNMixer I didn't really like it so much. My points are that:

  • sometimes it get in the way, there's more comment than code. See the Gtk Signal Handlers in ui-tray-icon.c. The code would be easier to read without this big amount of comments.
  • sometimes it just look like copy/pasting Gtk doc (same example as above), what's the point ?

I'm not against documentation, and if you look at the code you'll see that I actually document a lot. But I'm more like inline doc (describing the main steps inside a function), and documenting what's really needed. If the software is well-designed, it speaks by itself, there's not much to document. My problem with Doxygen is that it forces me to write tons of seemingly useless doc. It's a pain to write and to maintain.

Here's my point. What do you think about it ? It's no big deal though, I'm not gonna fight for/against that. I don't care so much.

OK, waiting for your feedback, cheers :)

@elboulangero
Copy link
Collaborator Author

One quick word about the new design: it's quite object-oriented programming. No global variable. Almost no structure defined in headers. Mostly objects that are manipulated through functions.

There are two main parts:

  • audio system (low-level): audio is the interface to access it, alsa is the underlying implementation. The audio system is manipulated through functions (get/set volume/mute), and send signals each time something changes. Signal emission is implemented in a simple and cheap way, but it's enough for the use we make.
  • ui (high-level): every ui object strives to be independent from the rest of the world, and just listen to signals from the audio subsystem.
  • the rest (high-level) also: notif, hotkeys. Straight forward: Hotkeys manipulate audio. Notif waits for signals.

Support has been split in several files for the sake of sorting and grouping stuff by category (I tend to overdo things sometimes). Logs are now pretty... That's all there is about it.

@elboulangero
Copy link
Collaborator Author

By the way I won't be much available for the next 15 days. I'll see u when I come back from holidays :D

@elboulangero
Copy link
Collaborator Author

OK, I finished with the rewrite. In the end I completed the doc, Doxygen style. I still believe Doxygen doc is just too much hassle and is likely to be unmaintained the day someone look at it. But hell, now it's done.

So basically I'm waiting to push that upstream (I'll squash the commits since they're messy). A simple thumb up is enough !

Then, I'll take care of the translation. In the meantime, feedback is appreciated :)

@hasufell
Copy link
Collaborator

I have the following compiler warnings on gtk3 build:

alsa.c:979:6: warning: variable 'mixer' is used uninitialized whenever 'if' condition
      is true [-Wsometimes-uninitialized]
        if (hctl == NULL)
            ^~~~~~~~~~~~
alsa.c:992:6: note: uninitialized use occurs here
        if (mixer)
            ^~~~~
alsa.c:979:2: note: remove the 'if' if its condition is always false
        if (hctl == NULL)
        ^~~~~~~~~~~~~~~~~
alsa.c:966:20: note: initialize the variable 'mixer' to silence this warning
        snd_mixer_t *mixer;
                          ^
                           = NULL
ui-popup-window.c:97:23: warning: passing 'GCallback' (aka 'void (*)(void)') to
      parameter of type 'gpointer' (aka 'void *') converts between void pointer and
      function pointer [-Wpedantic]
        n_handlers_blocked = g_signal_handlers_block_by_func
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/gobject/gsignal.h:571:27: note: expanded from macro
      'g_signal_handlers_block_by_func'
                                          0, 0, NULL, (func), (data))
                                                      ^~~~~~
/usr/include/glib-2.0/gobject/gsignal.h:420:25: note: passing argument to parameter
      'func' here
                                               gpointer           func,
                                                                  ^
ui-popup-window.c:103:2: warning: passing 'GCallback' (aka 'void (*)(void)') to
      parameter of type 'gpointer' (aka 'void *') converts between void pointer and
      function pointer [-Wpedantic]
        g_signal_handlers_unblock_by_func
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/gobject/gsignal.h:585:27: note: expanded from macro
      'g_signal_handlers_unblock_by_func'
                                          0, 0, NULL, (func), (data))
                                                      ^~~~~~
/usr/include/glib-2.0/gobject/gsignal.h:428:25: note: passing argument to parameter
      'func' here
                                               gpointer           func,
                                                                  ^

On gtk2 there is also:

ui-popup-menu.c:68:23: warning: passing 'GCallback' (aka 'void (*)(void)') to
      parameter of type 'gpointer' (aka 'void *') converts between void pointer and
      function pointer [-Wpedantic]
        n_handlers_blocked = g_signal_handlers_block_by_func
                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/gobject/gsignal.h:571:27: note: expanded from macro
      'g_signal_handlers_block_by_func'
                                          0, 0, NULL, (func), (data))
                                                      ^~~~~~
/usr/include/glib-2.0/gobject/gsignal.h:420:25: note: passing argument to parameter
      'func' here
                                               gpointer           func,
                                                                  ^
ui-popup-menu.c:74:2: warning: passing 'GCallback' (aka 'void (*)(void)') to parameter
      of type 'gpointer' (aka 'void *') converts between void pointer and function
      pointer [-Wpedantic]
        g_signal_handlers_unblock_by_func
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/glib-2.0/gobject/gsignal.h:585:27: note: expanded from macro
      'g_signal_handlers_unblock_by_func'
                                          0, 0, NULL, (func), (data))
                                                      ^~~~~~
/usr/include/glib-2.0/gobject/gsignal.h:428:25: note: passing argument to parameter
      'func' here
                                               gpointer           func,
                                                                  ^

In addition, the variadic macros in support-log.h cause a lot of these warnings:

In file included from ui-about-dialog.c:23:
./support-log.h:56:16: warning: token pasting of ',' and __VA_ARGS__ is a GNU
      extension [-Wgnu-zero-variadic-macro-arguments]
                        _DEBUG(fmt, ##__VA_ARGS__); \
                                    ^
./support-log.h:51:20: warning: token pasting of ',' and __VA_ARGS__ is a GNU
      extension [-Wgnu-zero-variadic-macro-arguments]
                __FILE__, ##__VA_ARGS__)

@elboulangero
Copy link
Collaborator Author

I don't have any of these even with -Wall -Wextra. My gcc is:

gcc version 5.3.1 20160220 (Debian 5.3.1-9)

What's your secret :) ?

Anyway, I fixed the first warning. The second warning about GCallback, I'm not sure, I think a simple cast to gpointer should fix it.

For variadic macros, yep, it's a gnu extension. It's so useful for debug, I can't live without ! Maybe we should explicitely set the gcc option -std=gnu99 ? Maybe that would disable the warning ?

@hasufell
Copy link
Collaborator

What's your secret :) ?

clang

For variadic macros, yep, it's a gnu extension. It's so useful for debug, I can't live without

Maybe it's better if we go back to compile-time debugging support then, which means these warnings would only happen if you want to do actual debugging (and then we can assume the guy knows what he is doing)? I'm not particularly a fan of forcing -std=gnu99 on everyone, even if he doesn't do debugging.

@elboulangero
Copy link
Collaborator Author

Hi, I tried clang but I can't get the same warnings as you. What cflags then ? Ahhhh OK, now I understand that you use -pedantic, that's why... THIS CFLAG IS SUCH A PAIN !!!

Maybe it's better if we go back to compile-time debugging support then

Definitely not agree. Up to now each time someone reports an issue I have to ask him/her to rebuild PNMixer with debug support, which is a pain for everyone. Having a -d option in the command-line makes debugging so much more easier.

I'm not particularly a fan of forcing -std=gnu99 on everyone

OK, so I rewrote the debug part without using ##__VA_ARGS__, the warning is gone by now.

The last warning about GCallback puzzles me a bit. The problem is that g_signal_handlers_unblock_by_func needs to be passed a function pointer, as the name suggests. But the signature takes a gpointer, which is a data pointer. So with or without explicit cast, there's a warning anyway, since "ISO C forbids conversion of function pointer to object pointer type". Weird...

I'm wondering how do people get away with that usually. Either nobody uses g_signal_handlers_unblock_by_func, or either nobody compiles with -pedantic ;)

@elboulangero
Copy link
Collaborator Author

It's widely used in Gtk, they just cast to GCallback as I do...

Interesting discussion on this topic here: http://compgroups.net/comp.lang.c/cast-function-pointer-to-void/722812

So basically, Glib mixes data pointer and function pointer sometimes, violating C standard and not caring. And in this case, Glib forces us to do the same. We can either do not use g_signal_handlers_unblock_by_func, or either do not use -pedantic.

By the way, what's the point of using this flag ? To me, -pedantic is only useful for code portability. PNMixer is Linux only, so portability is not an issue. As for compiler, Clang is fine with GNU extensions (see http://clang.llvm.org/docs/LanguageExtensions.html), there's no need to conform to strict ISO C. I don't really see the interest of using -pedantic in PNMixer. We depend on libraries that are not pedantic anyway, so really, what good does it bring ?

@hasufell
Copy link
Collaborator

OK, so I rewrote the debug part without using ##VA_ARGS, the warning is gone by now.

Great, I hope that was not too much hassle.

don't really see the interest of using -pedantic in PNMixer. We depend on libraries that are not pedantic anyway, so really, what good does it bring ?

Well, it sometimes reveals interesting things. If the reason of warnings is the API of some libraries we use, we sure can ignore them. I only occasionally enable it.

@elboulangero
Copy link
Collaborator Author

Great, I hope that was not too much hassle.

No it's no big deal. It just adds a little bit of code, not much. I still think it's a little bit sad. because ##__VA_ARGS__ is really fit for the job. It makes debug macros simple, straight-forward and efficient.

I only occasionally enable it.

Ok I get it. So if you use it I can understand that you don't want the compiler output filled with the ##__VA_ARGS__ warnings. No problem.

@elboulangero
Copy link
Collaborator Author

I also worked around the warning caused by g_signal_handlers_unblock_by_func, so now pedantic compile is clean

@elboulangero
Copy link
Collaborator Author

Hi,

lately the rewrite branch had a lot of polishing, and little bugs here and there have been chased down thanks to testing from @landroni. I'm confident to merge that in the main branch. Documentation is finished. Code is indented. Pedantic compile is clean.

It also seems to work along with PulseAudio, maybe @nicklan can give it a try to confirm.

Branch is still here: pnmixer-rewrite-dirty.

I'll squash it down before merging, the commits don't tell a story to be kept.

Waiting for your feedback before merging, but will merge anyway if no feedback for too long ;)

@elboulangero
Copy link
Collaborator Author

Merged !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants