Compiz code bad?

[gnumdk]

>I do not belive writing CORE functions that are 1k LOC is
>healthy for the program over time.

Here is a beryl dev statement about compiz code... (about handleEvent() in event.c).

I just look at this function, it's a BIG (really) switch{} with all XEvent in it.

This code is GOOD, you don't have to clean this by adding multiples functions... It is structured and calling functions have a performance cost... (minor i know)

Otherwise, i agree with him, compiz core miss some comments/docs...

ps: sorry if my english sux... ;)

[wfarr]

I'd rather good quality code that may not be as comment-friendly rather than one that's commented out the yin-yang but is still pretty piss-poor. ;)

[iznogood]

This is the common style used for message handlers.

The code is fine, the beryl devs can write anyway they like, its up to them now...If they do not like it then they should stop copying from it. And not just the code but the ideas also...They said that after their next version (0.2 i believe) they should divert from compiz more so i am really waiting to see some original things there, that will justify the fork and all these comments

Afterall if something is implemented its much easier to criticize mistakes and say what you think. The hard part is writing it in the beginning. Refactoring is not so hard most of the times

I know almost nothing of X coding and OpenGL(just very basic stuff) but i could read the code without any effort at all

EDIT Its very easy to forget that david maintains much more code than the one in compiz, so its very important to maintain a common style for all the code base (XGL, X, compiz and others). Its harder to read-maintain code when you also have to adapt to different styles - formats, and most developers prefer to see the same piece of code when the applicaton logic permits it, instead of rewriting something differently. Plugin developers can not always understand that...

[mikedee]

I think Kristian is just applying beginner rules to projects which he admits he doesnt really understand (no offense :))

If you are a beginner coder and you want to have a very basic set of rules for how to write clean easy to maintain code, those rules are what Kristian keeps spouting out.

Personally I think that the handleEvent is fair enough. I don't think it would be too much more readable if each event type had its own function (it would certainly be slower). To be honest, nobody needs to go there. If you want to change something in that function and you don't understand it because its too long, then you shouldn't be messing with such core functions (Compiz basically runs around this function, changing it could severely break things).

I see that they also do not really believe what he says too much or they would have actually documented things first and changed this function. Kristian seems to think that if you document the code and split this function that Compiz would suddenly become really experimental and cool.

I don't think that the other beryl devs actually agree with him, he seems to have his own opinions which are not always that of he beryl collective.

Read any of his posts from the mailing list, he seems more of a tyrant than David ;)

http://lists.beryl-project.org/pipermail/beryl-dev/2007-February/thread.html

I think this recent thread is most telling - Kristian says categorically that they dont need to port Compiz changes and with his help they can make beryl 'really cool' (or something)

http://lists.beryl-project.org/pipermail/beryl-dev/2007-February/000163.html

Except nobody listens to him :D A couple of hours later and someone ports the fragment system

http://bugs.beryl-project.org/changeset/3994

Seems like they are clutching at straws

Davids code is very nice and clean, how else do you think so many people are able to read and understand it? (even if they have never seen Xlib before). If it was as horrible as they say, then they would not have been able to do anything with it.

Back in the day, quinn claimed the same thing about gconf and now beryl has a buggy broken configuration system (because the code was written as quickly as possible without any thought for the requirements). I have a feeling there was 1 bug there which they couldn't track down so it was rewritten.

[RYX]

That guy seems quite ridiculous to me (and his statements don't appear like there is a big team-spirit among the beryl-devs): I do not
care about input redirection or the new fragment system at this point.
I want to clean up core. I want to clean up cube. I want to create a
product that will work in 5 years. Not just today.

Yes, 5 years ... beryl ... sure [*5 minutes of dirty laughing*]. Never read any of his statements before, but looks like beryl now got a new "leader" ... farewell, guys :D

:)

[rememo]

Finally some good news, look at this quote from the main site of beryl-project:

Beryl is a fork of the Compiz project, started by David Reveman of Novell. We continue to port new changes from compiz, and considier them essentially our upstream. Beryl could not have existed were it not for the heavy lifting done both server side by David and in creating compiz, which is the base on which all of our code is built, and which still comprises probably 90% or more of our code, though this number is likely to change as the 0.3.0/0.4.0 release cycle gets started.


I couldn't believe it myself, but it seems like the xdevconf2007 was the right place for quinn to go :wink:
I hope they follow their statement and bring innovations to the core to justify the fork. Seems like not everybody from beryl has Kristians opinon on compiz bad code.

[RYX]

I think that is also a late result of my bashing in the beryl-forums (http://forum.beryl-project.org/viewtopic.php?p=14488#p14488) about one month ago. QuinnStorm changed the FAQ right that day but apparently had no access to the main page ... but the "lighting-fast" beryl-project managed to get it done in only three weeks ... congratulations. Most interesting was that only few beryl-users know where beryl really comes from ...

[rememo]

Alright, I didn't see this thread (there are way to many on this topic). Then we have to thank also you RYX for bringing this up :D . Well I think now this seems to be a state that is acceptable, despite the unfriendly way the fork was done. Every new user is informed about the origins and the beryl-devs promised to do real innovations to the core and not just copypaste compiz core. I'm curious with what they can come up with although there is a slight disbelief. Also with this, they admit to their users that the fork was for no technical reasons. But unfortunately I doubt that there will be many users that are interested in this. As sad as this is, I think there is nothing one can do anymore this time (look at the mailing list, they decided to go on with beryl allthough jiggish had some good points). Now lets just wait and see and stop trying to revert everything back to the old days, this won't happen I guess.

[mikedee]

Alright then, so now their official reason for existing is so that they can take code from Compiz but never giving anything back. As Robert pointed out, if you take code from another project regularly you are NOT a fork, you are a branch. There is no two ways about it.

Beryl has always and will always be about reacting to public opinion in order to justify themselves and make it look like they are better than Compiz.

- We have forked Compiz - quick! pretend David does not want to do what the community wants. Say we will eventually rewrite compiz and fix all the bugs (we can all see what a joke that was).
- Some people think that Compiz requires gnome - quick! write a backend which does not depend on gconf and say things like David will not give up gconf.
- Ubuntu shows an interest - quick! lets make our sole aim to be included in Ubuntu by default. Point out how David works for Novell so only wants to do Compiz for the benefit of Suse. Colored overlays and stuff, think of people less fortunate.
- People think that beryl is just about bling - quick! lets write some useful plugins. Everyone think of something useful we can do. Those plugins are all in 'unsupported' now.
- People starting to think there is no difference between Compiz and beryl - quick! think of things we can change so that we can claim we have rewritten this or that.

And on and on... They still haven't answered the 2 major questions which are:

1. What is the reason for forking beryl, what can you do with beryl that you cannot with Compiz?
2. Since David gave so much and like you say, you wouldn't exist without his hard work, why have you never given back any improvements? Its hardly a nice way to work together is it? If the improvements are good there is no reason why they would not be wanted in Compiz.

Oh, and what exactly are these 'major' improvements we keep hearing about? Replacing WRAP/UNWRAP? Rewriting the options system (again) :D :D

Please just write this major stuff and then show it to the world, dont keep talking about how one day you will really really do something good.

I think Jigish's ideas were always the natural course for the beryl project, but unfortunately I doubt that will happen as they are going to spend the next 5 years making beryl stable (again). They have always wanted the attention so they will take the core as well.

Their actions speak much more strongly than their words, at the moment they are just take take take. I mean how many people are likely to read the third paragraph on the home page. Their FAQ still says that the beryl devs wrote beryl (oh, with a bit of help from someone called David something)

[mikedee]

Oh, and whilst they are in an honest mood, they should probably explain how they are funded and by who.

They like people to believe that they are struggling and are supported by donations from the community. This as far as I know is not true and they are funded by a commercial company.

[PaK]

1. What is the reason for forking beryl, what can you do with beryl that you cannot with Compiz?

I hope you have on mind sth like "For forking Compiz [..]":). Well imho main reason was that QuinStomr didn't want to commit patches for compiz in they way that David would approve them, am i right ? All the rest is their ideologist. But the way, there was sth about that all compiz development talk was by mailing list ? Funy that they have own mailinglist, all development was moved into that list from their forum.

Well mike, chill out ;) if they dont have right, and only pretend to have _right_ they will fail sooner or later. Look at David, he is over all that stuff. He know what to do and he is doing that. This is his job ;-) I thnik he know that sooner or later _branched_ compiz (a.k. beryl;) will fail, and even if not ? who cares ?

[stjepan]

1. What is the reason for forking beryl, what can you do with beryl that you cannot with Compiz?
2. Since David gave so much and like you say, you wouldn't exist without his hard work, why have you never given back any improvements? Its hardly a nice way to work together is it? If the improvements are good there is no reason why they would not be wanted in Compiz.
1. Beryl has it's own path. Compiz-quinn was pretty much like Beryl - quick hacks, more features, more plugins, more bleeding-edge style.
The differences between Compiz and Compiz-Quinn were just too big so they have started a new project.
2. David doesn't like hacky code. He says he wants high quality code. There really are some improvements, but David is more focused on the core rather than plugins.

BTW, you're typing "beryl" lowercase and "Compiz" uppercase :D

[stalynx]

Rewriting the compiz core achieves nothing and in the end will be the death of them. It'll be like Hitler's invasion of Russia but more bloody.

[iznogood]

This as far as I know is not true and they are funded by a commercial company.

Be more specific please

[mikedee]

This as far as I know is not true and they are funded by a commercial company.

Be more specific please

Listen to the LUG radio interview that quinn gave, if you can bear to listen for the whole hour then you will hear the name of the company that now 'sponsors' them (I have a feeling the same company is funding trips to California for Quinn). I assume that the company mainly pays for hosting etc.

I looked it up at the time and it was basically a company that sold small mini mac like computers with Linux and I assume beryl too. Just a small company, but they were certainly commercial. Cant remember the name, sorry

Seems like they keep it fairly quiet since I assume it would ruin their whole argument that Compiz is run by a company with only thoughts for 'company things', whereas they are written by the community for the community etc etc

[RAOF]

I'd just like to add, having just written an app-switcher patch for switcher.c, that the plugin code is really easy to read. I've never done anything anywhere near as close-to-the-metal as compiz, and it was really easy and clear what was going on, and where the flow of control was.

I haven't looked at the core code, or the actual drawing code of switcher.c, but I didn't need to, which is kinda the point :).

[mikedee]

Well mike, chill out ;) if they dont have right, and only pretend to have _right_ they will fail sooner or later. Look at David, he is over all that stuff. He know what to do and he is doing that. This is his job ;-) I thnik he know that sooner or later _branched_ compiz (a.k. beryl;) will fail, and even if not ? who cares ?

I think it is important, my main problem is that because Compiz is the new face of Linux people will run beryl and it will be unstable/lack features/crash etc and they will think it is representative of Linux. I see it all the time where new users think berl == Linux.

I also think its wrong for people to fix bugs but not submit patches for them back to the main project. They are using some of those fixes as ammunition against Compiz and that is not right.

If they fail then they will take any bug fixes and user testing with it, it will be a total waste of time and we will ALL suffer. In fact I do not think we could ever get the exact fixes out because none of the changes are documented properly or available as individual patches.

If they succeed then they will have stolen a project from someone else and will be taking credit which they do not deserve. They cannot maintain the code and it will give Linux a bad name.

[mikedee]

Finally some good news, look at this quote from the main site of beryl-project:

Beryl is a fork of the Compiz project, started by David Reveman of Novell. We continue to port new changes from compiz, and considier them essentially our upstream. Beryl could not have existed were it not for the heavy lifting done both server side by David and in creating compiz, which is the base on which all of our code is built, and which still comprises probably 90% or more of our code, though this number is likely to change as the 0.3.0/0.4.0 release cycle gets started.


I notice that all the new credit is now gone from the site. They have obviously changed their mind about the direction again.

Or maybe the new changes were lost in some sort of hard drive crash

[nzjrs]

*sigh*

http://dev.beryl-project.org/~kristian/beryl/7/the-future-of-beryl/

Compiz is developed as a one-man project, and that could be a good thing, but it would require that it is done because of an attitude. Not because it is convenient. If the code was clean and smooth, I would surely have no problem understanding why someone would want complete control over every part of it. They would want to make sure it remains clean. However, this is not the case with the compiz code, nor is it the case with the Beryl code.

At least he manages to not make any sense

[wfarr]

What an idiot.

What I want to do with Beryl is clean it up. Make it better. I want to use the good ideas David had, and implement them properly. Make the code sustainable over time.

That is a complete 180 from their views in the past.

What it amounts to is a lot of smoke and trying to NOW do what David and the other developers have been practicing for ages upon ages and in the end, I think they lack the brain finesse to pull it off if they stay true to their word and divert from using Compiz code.

[maniac]

While I do not share all of Kristian's opinions, I have to defend him in one point:


Personally I think that the handleEvent is fair enough. I don't think it would be too much more readable if each event type had its own function (it would certainly be slower). To be honest, nobody needs to go there.

So you're saying "David has written this, don't touch it"? That's not a very good attitude - a constructive critizism is helpful to both sides.
At this particular point, I share Kristian's opinion: Extremely huge functions are very hard to read, so it indeed makes sense to split them into smaller pieces.
The speed arguement does not count: I'm very sure David knows the __inline__ directive.


If you want to change something in that function and you don't understand it because its too long, then you shouldn't be messing with such core functions (Compiz basically runs around this function, changing it could severely break things).

Long functions != complicated functions. I'm pretty sure Kristian understands what handleEvent() does - it's not that hard to understand. It's just uncomfortable to read.

[iznogood]

While I do not share all of Kristian's opinions, I have to defend him in one point:


Personally I think that the handleEvent is fair enough. I don't think it would be too much more readable if each event type had its own function (it would certainly be slower). To be honest, nobody needs to go there.

So you're saying "David has written this, don't touch it"? That's not a very good attitude - a constructive critizism is helpful to both sides.
At this particular point, I share Kristian's opinion: Extremely huge functions are very hard to read, so it indeed makes sense to split them into smaller pieces.
The speed arguement does not count: I'm very sure David knows the __inline__ directive.


If you want to change something in that function and you don't understand it because its too long, then you shouldn't be messing with such core functions (Compiz basically runs around this function, changing it could severely break things).

Long functions != complicated functions. I'm pretty sure Kristian understands what handleEvent() does - it's not that hard to understand. It's just uncomfortable to read.

You do not know what you are saying. As i have posted before this is standard style used for event handlers even in DOS days when people hooked on BIOS interrupts to see which key was pressed on the keyboard.
It is also used in Windows from the very start. Everyone who has written a piece of code on old windows API knows that this is the common style. This particular function does not matter how large it can get because the main purpose it serves is to dispatch the events - notifications to all listeners so it does very few things. This is why it should not be touched. You should never need to change anything there, if you want to do something and you look there to place your code then you should stop coding now because you do NOT know want you are doing. The only reason someone might need to change anything there is if an event is not supported and only then. He should hook there, do some small things and dispatch the event elsewhere is needed.

I do not know this Kristian guy but if he wants to change something he is looking at the wrong place. This function is written like that ON PURPOSE, if someone can not understand that then its his problem

Lets not discuss this anymore because its pointless. If people still believe that david could not break that function it 100 smaller one's but can code on X and openGL like he does then maybe they need to see a doctor or perhaps do some serious coding them selves.

Read my previous post. If you are a framework programmer you need to maintain a common style not only in your application - library but with others to so that people can easyly understand what you try to do.If someone has problems undestanding such coding style it only means that he has no experience with such frameworks

That's all folks
Hopefully you understand

[maniac]

You do not know what you are saying. As i have posted before this is standard style used for event handlers even in DOS days when people hooked on BIOS interrupts to see which key was pressed on the keyboard.
It is also used in Windows from the very start. Everyone who has written a piece of code on old windows API knows that this is the common style. This particular function does not matter how large it can get because the main purpose it serves is to dispatch the events - notifications to all listeners so it does very few things. This is why it should not be touched. You should never need to change anything there, if you want to do something and you look there to place your code then you should stop coding now because you do NOT know want you are doing. The only reason someone might need to change anything there is if an event is not supported and only then. He should hook there, do some small things and dispatch the event elsewhere is needed.

Believe me - I know very good what I'm saying. I'm also used to Windows programming, so you don't need to explain that to me.
Of course the function will get large because there is a big bunch of different messages. But where's the problem in splitting out the handling of single messages (ClientMessage comes to my mind, or the default case) in separate functions? What I'm saying is: If it actually DID only dispatch the events, everything would be fine. But the handling of the events is the largest part, not the dispatching.

BTW: You're basically telling me "if you believe that only a single line of this function is wrong, you don't know what you're doing" ... nice argument, indeed. Looks like we have finally found the perfect code (TM) :roll:

And another BTW: NOONE wants to place his own code there ... please read the discussion again.


Lets not discuss this anymore because its pointless. If people still believe that david could not break that function it 100 smaller one's but can code on X and openGL like he does then maybe they need to see a doctor or perhaps do some serious coding them selves.

No comment... :?


Read my previous post. If you are a framework programmer you need to maintain a common style not only in your application - library but with others to so that people can easyly understand what you try to do.If someone has problems undestanding such coding style it only means that he has no experience with such frameworks

Are you sure it's common sense in X that functions should be >1000 lines (handleEvent has 1017)? :shock:

[iznogood]

From my experience people usually let this function bloat because they handle important stuff elsewhere and want to be able to find something fast. If you think that this is not the case here then you should point that out

Anyway my bad attitute was not directed at you, i hope you understand, but we can not judge a piece of code just by one or two functions. Also i found unnecessary the following :

"So you're saying "David has written this, don't touch it"? That's not a very good attitude - a constructive critizism is helpful to both sides."

which is not true of course. It was an old argument from kristian

[mikedee]

It looks to me like every member of the beryl team has their own idea about what beryl will be. I dont think anyone has the same opinion. What seems certain though, is the beryl devs think that they need some sort of new goal since the last one went horribly wrong. They are all just reacting to public opinion and are desperate to find some new reason to exist.

http://dev.beryl-project.org/~cyberorg/beryl/14/beryl-where-do-we-want-to-go/

I dont think that Kristians opinion carries much weight with them. They all just have their own opinions but none of them have any power (I think quinn and ixce have the real say).

[PaK]

my $0.02 about HandleEvent, i thnik even if somone will do it using functions he will break base rule of structural coding, because functions/procedures in structural programing shuould be used only when some part of code is used in more than 1 place, if not there is no reason to doing it in that way. This should be obvious!

[gnumdk]

The speed arguement does not count: I'm very sure David knows the __inline__ directive.

-inline isn't portable!
-inline just tell to the compiler: try to inline the function...

[gnumdk]

It's just uncomfortable to read.

Why? You know handleEvent() is one big switch...

What the difference between searching for CirculateNotify: or for circulateNotify() ?

I don't see any reason to separate events code in functions as switch{} already do this separation...

[iznogood]

It's just uncomfortable to read.

Why? You know handleEvent() is one big switch...

What the difference between searching for CirculateNotify: or for circulateNotify() ?

I don't see any reason to separate events code in functions as switch{} already do this separation...

I agree totally

[RAOF]

It looks to me like every member of the beryl team has their own idea about what beryl will be. I dont think anyone has the same opinion. What seems certain though, is the beryl devs think that they need some sort of new goal since the last one went horribly wrong. They are all just reacting to public opinion and are desperate to find some new reason to exist.

http://dev.beryl-project.org/~cyberorg/beryl/14/beryl-where-do-we-want-to-go/

I dont think that Kristians opinion carries much weight with them. They all just have their own opinions but none of them have any power (I think quinn and ixce have the real say).

Yay. You should really link that up a bit more, mikedee :). If they were to actually take that approach, I'd applaud. Everyone wins!

[FunkyM]

It looks to me like every member of the beryl team has their own idea about what beryl will be. I dont think anyone has the same opinion. What seems certain though, is the beryl devs think that they need some sort of new goal since the last one went horribly wrong. They are all just reacting to public opinion and are desperate to find some new reason to exist.

http://dev.beryl-project.org/~cyberorg/beryl/14/beryl-where-do-we-want-to-go/

I dont think that Kristians opinion carries much weight with them. They all just have their own opinions but none of them have any power (I think quinn and ixce have the real say).

Yay. You should really link that up a bit more, mikedee :). If they were to actually take that approach, I'd applaud. Everyone wins!

Well, it will still take a long time for beryl guys to realize what beryl (or Compiz-Quinn) should actually had been, a list of well maintained patches and experimental plugins ontop of a compiz checkout.

Regarding clean code, both projects have parts with code without optimal layout which is normal for such a "new tech" project (and if you know better, feel free to post patches).

The main difference is that compiz is nevertheless attempting to enforce clean code as much as possible whereas beryl often totally disregards clean code in order to just get the "nice eyecandy" in.

[maniac]

-inline isn't portable!

Please have a look at the C99 standard, chapter 6.7.4 (page 112) if you have access to it. inline is mentioned there.


-inline just tell to the compiler: try to inline the function...
inline tells the compiler that calls to this function should be as fast as possible. The compiler decides how to exactly achieve that.


thnik even if somone will do it using functions he will break base rule of structural coding, because functions/procedures in structural programing shuould be used only when some part of code is used in more than 1 place, if not there is no reason to doing it in that way. This should be obvious!

Well, this is obvious if documentation and readability don't count at all. Maybe this is the case for you - at least it isn't for me. I also think that code readability is more important than paradigms.


I dont think that Kristians opinion carries much weight with them. They all just have their own opinions but none of them have any power (I think quinn and ixce have the real say).

Do you disagree that each dev having his own opinion is a good thing? At least I think that this is a good thing.
And responsibilities are clearly stated out here (http://www.beryl-project.org/team.php).

[PaK]

thnik even if somone will do it using functions he will break base rule of structural coding, because functions/procedures in structural programing shuould be used only when some part of code is used in more than 1 place, if not there is no reason to doing it in that way. This should be obvious!

Well, this is obvious if documentation and readability don't count at all. Maybe this is the case for you - at least it isn't for me. I also think that code readability is more important than paradigms.

IMHO you should start coding in c# or java ;) c isn't alway obviously readable, it shouldn't be because if it would be why someone would invented c++ java na then c# if c is so perfect ?

I didnt check but imho you will find many smimilar places where some part of code takes about 1k lines of code (30% will be \n and { } :) and it is 1 function. Offcours i dont have on mind any drivers because sometimes they are really bad implemented and bugged.


OMG is that handleEvent so important ? What doing it in other way will change ? Do you really need to do sth with that part of code in evry release ? (when you adding some special future ?) And is that really hard to use switch/case ? I dont see any benefits of doing it in other way, any "pratcical benefits!!" all discussion is similar to "what coding style we should use" ? camel or hungarian ? what will be the difference in each other ? for compiler ? :) for processor ? for user experience ? for documentation ?

I thnik paradigms are very important in c. This is very old language and its probably imposible to invent new paradigms that will be better and will break old.

Sorry for bad english :P

[lowfi]

This discussion is pointless. There's absolutly nothing to gain from splitting up this function. The only thing that might happen is that it introduces new bugs or affects performance. It's a wasted effort. How many beryl devs do actually have to work with this function? 10? 1? It was already mentioned that the only thing one would do there is to add a new event (and i guess there are not many left).

What's the point in documenting the event system? People who want to understand what's going on there should read the X documentation, not the beryl docs. And the length argument, well...
http://svn.gnome.org/viewcvs/gtk%2B/trunk/gdk/x11/gdkevents-x11.c?rev=17044&view=markup
take a look at gdk_event_translate ()

[RYX]

I fully agree with lowfi ... pointless discussion here. It is totally useless (and definetly a decrease in performance) to split the event-handling function into multiple functions. If they want to do it - they shall go on and do it. To me, enforcing OOP-coding style and rules in C-programming makes no sense.

It can NOT be faster than it currently is (and that's one of the main goals - being fast) ... and it is absolutely easy enough to read .... If people are not able to read/understand the handleEvent, they should keep their hands off that function, grab a book about C and start reading and learning!!! If they think compiz is sooo badly coded, they should stop taking any newly written line of code from compiz ...

(Most ironic is that splitting handleEvent into multiple functions would increase the length of the code even more ... isn't that what they want to reduce?)

:)

[maniac]

I fully agree with lowfi ... pointless discussion here. It is totally useless (and definetly a decrease in performance) to split the event-handling function into multiple functions. If they want to do it - they shall go on and do it. To me, enforcing OOP-coding style and rules in C-programming makes no sense.

RYX, while I agree that this discussion is a bit pointless, I just can't stand if wrong statements are presented as facts. A compiler which is not completely dumb (and I doubt gcc is) will always arrange functions called only one time as if they were written at the location they are called. There will be no performance influence at all. And what does splitting a large function into smaller ones have to do with OOP? Sorry, I don't get it.


It can NOT be faster than it currently is (and that's one of the main goals - being fast) ...

Yes, it won't be faster, but also it won't be slower (see above).


and it is absolutely easy enough to read .... If people are not able to read/understand the handleEvent, they should keep their hands off that function, grab a book about C and start reading and learning!!! If they think compiz is sooo badly coded, they should stop taking any newly written line of code from compiz ...

Sorry, do you want to start a flamewar? I'm trying to do this discussion on a technical level, but you (read: iznogood, PaK, ... ) attack me/us on a personal level. I don't think that's a nice style - could you please stop that?
BTW, I do not agree with Kristian that Compiz' code is bad. But there are some points where it could be improved, and handleEvent() is an example for that. You read right, it's only an example. And I don't want to attack you, David or anyone else. I'm only saying that there are some points where his code indeed can be improved.


(Most ironic is that splitting handleEvent into multiple functions would increase the length of the code even more ... isn't that what they want to reduce?)

That's misinterpretation - a better code readability sometimes also can be achieved by more LOC if it's splitted properly. I really don't think reducing code size to the max is a good thing - and the end, it results in things like that (http://www.de.ioccc.org/main.html) ;)

[mikedee]

BTW, I do not agree with Kristian that Compiz' code is bad. But there are some points where it could be improved, and handleEvent() is an example for that. You read right, it's only an example. And I don't want to attack you, David or anyone else. I'm only saying that there are some points where his code indeed can be improved.

Well it IS open source, if you find there is something which can be approved, prepare the patches and send them to the mailing list. The patches will then be reviewed by your peers (some of whom are experts in xlib, opengl and programming in general). If they are good then they will be accepted, if they are not then people will point out how they are not good.

Sometimes I think that it is peer review that you do not really like. Peer review at beryl is basically showing other people your code and then being told that its great. I think beryl has always taken peer review badly, they just brand anyone a troll who says their code is less than optimal. The really funny thing is normally you merge the correct changes from Compiz (ie. multihead changes) but do not make a big fuss over it.

As other people have said - this is a pointless discussion. I am not deluded that all of the Compiz code is perfect and could never be improved on, but splitting up a function is not going to help at all. If you are going to show us all how its done, then please show us your code - stop telling non-technical people that Compiz is not written well and you are single handedly going to rewrite it. It is wishful thinking at best and plain deception at worst (Kristian I am looking at you).

[PaK]

I didnt want to attack you at personal level.

I thnik everything about this case was said before. If you think you can improve some parts code just do it. I am not proper person to judge about that your paths(that will be based on your ideas) will be or will not be included into compi. I can only judge your ideas, giving some technical (i hope they are technical;) arguments. This are the rules of open disccussion right ? I dont want to attack you, i dont want to create flamewar(i hate taht word :().

[RYX]

I surely don't want to start any flamewar, we apparently have a very different idea of this stuff (which is absolutely ok, I think). To me it makes no difference at all - 50 small functions or one large function ... event-handling is event-handing. It is less trouble to have all in one function. The code is not reused anywhere so there is no need to make multiple functions of it ...

Sorry, I just don't see any improvement in splitting a function that is untouched most of the time, only to cause much more chaos (by having tens of small _additional_ functions).

And what does splitting a large function into smaller ones have to do with OOP? Sorry, I don't get it. I think in OOP it is quite uncommon to have functions with 1000+ lines. It is considered good practice to split things into rather small functions and create a good class-layout to improve reusability and readability ... but I doubt that these rules apply to event-handling functions in traditional C-applications.

[Not talking to anyone special here:] If someone wants the code to be better he should start commenting it instead of complaining about its complexity (or try creating compiz without complex code - oh, I forgot ... beryl is exactly that, right? keyword: "incremental rewrite" ...) ...

:)

[iznogood]

Generally speaking its nice to split a large function in smaller ones.

The event handler is not one of these functions. As for the why, it is pretty obvious if you look at older code. A few years ago function calls (especially if they passed many params) where extremely expensive and paint functions or main loops could not be written like that. It just would not run...

Now this is not the same as before (although a program like compiz or X should always look for wasted cpu cycles, kernel programmers optimize even register params) but its common style for such routines. Its pretty obvious and very readable you just have to use it. Large switch statements are not bad if they do not contain a lot of code in each case statement or a lot of code before and after the switch.
Its just another way to split it in many functions.

I apologize for been rude but i find it very obvious.

I have another question for anyone caring to take this conversation away from that stupid (yes it is) function: Personally i would have preferred compiz written in C++. What do you think?? I believe that if people do not use the strange C++ syntax and keep things simple, object oriented programming produces more readable, easier and faster code.

What do you think people??

EDIT: I agree with RYX

[PaK]

Before i will agree i must ask you one quiestion.
X, OpenGL, and gdk, gtk (used in first window decorator in compiz) this are libraries that compiz is using mostly right? X got Xlib, in tuture it will be xcb so compiz will probably switch into xcb sooner or later, opengl give us low-level 3d api mostly in c, gdk and gtk are written in c too and almost all programs written in c are in c (with some exceptions taht are using gtk/gtk with c++ gtk/gdk binginds :) so my question is using all this stuff with c++ would be so obvious and easy in c++ versoion of compiz from begining ? This is probably silly question so dont spank me for that :)

[iznogood]

easier

[lupine]

Mmm. Interesting stuff. Maybe?

TBH, I couldn't care less about whether handleEvent() is one huge case statement or lots of functions - but I think if you re-read KristianLy's blog post, you'll see it was mentioned as part of a wider overhaul of the event system, and core in general. This "let's grab hold of a single point one disagrees with and beat it to death" style of, ahem, debate is... silly. Really.

Although if I was (re?)writing it, I'd be doing it as a C++ class, with all the attendent fun and games :p. Or maybe in Object Pascal, just to put the fear of BEGIN into all the rabid C-lovers :p

Actually regarding C++, it's funny but one of the options that's been debated for beryl has been re-implementing in C++ :p. I like the idea in theory, but in practice I'd probably vote against it - I'm a pessimist. Even though it'd be so incredibly, amazingly elegant in it's implementation.

Compiz was probably written in C because davidr likes C, not because of any particular thought given to architectural advantages of an OOP core (there are many with a plugin structure). Again, fair enough. But can you see even the thought of re-implementing compiz in C++ hitting the compiz ML, much less being considered seriously? I can't :p. RE: libraries... C++ has bindings for everything that C does, and more besides. Maybe we're just seeing another fragment of the GNOME/KDE debate, here. Who knows.

Personally, I think that beryl-as-a-patchset-of-compiz is a bad idea, because it doesn't give "us" (i.e. beryl) the power to do what is /needed/ to core, and it's a sloppy way to organise a project anyway. As a bunch of plugins developers, we'd never be able to give our users what they wanted... Copy rendering is a good example of why the different development methodologies of beryl & compiz require separate cores, IMO. davidr didn't want to implement copy, and fair does to him for that - it is, after all, inferior in many ways to TFP. But many users (esp. ones not using Xgl :p) appreciated copy because it helps with the black windows bug, among other things.

I'm not saying davidr's way of doing stuff is bad, and I'm not saying he's a selfish git who wants everyone to use Xgl because it's his baby (no, really, I'm not. This whole copy-mode thing is just an example, anyway. The concept that's important is the power to be able to do what our users want, to the core - whether that's good or bad in architectural terms is irrelevent here. Really). IMO, this is FOSS, and users should have as much choice as possible.

What else...

How beryl's "leadership" is structured is laid out on the wiki, rather nicely if you ask me. Quick hint: the 1337ist developer doesn't /have/ to be leader. Honestly.

libberylsettings > dbus and a settings plugin, we've established that before.

GPL code can't be passed to a MIT project, we've also established that. In addition, we've established that the philosophical differences between the two positions are fairly intractable, and mostly ideological, so there's no point discussing it or using it as ammunition, really.

RE: sponsorship/shadow funding for hosting - that's a good one. Unless iXce has us all fooled and really /is/ running beryl from behind the scenes (and what an amazingly cool story that'd be), it's just not happening. Sorry. I'd love some funding too, but I donate all my bandwidth for free, as does everyone else. We've asked for donations exactly once, for a new server, and as far as I'm aware that's the only funding beryl has ever received.

Compiz isn't owned by Novell, it's given away freely to all and sundry. Remember? I don't hate compiz because it smells of novell (in fact, I don't hate compiz at all); I just hate Novell. That one of their employees happens to write compiz is no reason for a boycott. I think I'm the one who brought that issue up, actually, and it was almost completely a joke. But no worries.

xF,

...Nick

[RYX]

I can remember the "rewrite compiz in C++"-discussion from the old compiz-quinnstorm forums. I think rewriting compiz in C++ would be not very smart and a bad approach to solve the problem. The OOP-argument isn't very strong because the compiz architecture already follows a true object-oriented design - CompScreen, CompWindow, CompDisplay, ... - all these structs are the C-version of OOP. If these are properly mapped to another language through the pluginloader-system in compiz, we have a fast and clean core in C and great extensibility through plugins written in a higher-level language.

The compiz-sharp (or better: compiz-mono) plugin would allow exactly that - writing plugins in ANY CIL-compliant language (C#, boo, nemerle, ...) ... so also managed C++ (but I don't know really much about that). Everyone who wants to create plugins in other languages than C should focus on that plugin ...

(once again only my pov)

:)

[RAOF]

...

I have another question for anyone caring to take this conversation away from that stupid (yes it is) function: Personally i would have preferred compiz written in C++. What do you think?? I believe that if people do not use the strange C++ syntax and keep things simple, object oriented programming produces more readable, easier and faster code.

...

It's actually just as possible to write object-oriented code in C as C++ (Glib is, among other things, a library of tools to do this in C). C++ also has the disadvantage that it's difficult to write C code which links to it, but it's easy to write C++ code that links to C.

As RYX said, maintain compiz-cil and you can write plugins in whatever CIL language you want.

[nzjrs]

Jesus Francis Peter H Christ

This sounds like one of those embarising arguments you overhear when you walk through a computer science department at University.

"Erlang is awesome" "No I like Object Pascal" "C++ is the one true way forward" "C is pure man" "I code in assembly"

In this situation I chuckle to myself and walk away, knowing that in all likelyhood 76% of these people will end up being Java code cutters as a cog in a larger enterprise software cubicle machine.

But I digress,
*language arguments are lame
*OOP/AOP/MVC/ArchitecturalOrDesignBuzzwordOfTheMonth is/are methodologies not languages
*In the real world people care SFA bout what language something is written it

If you design and architect it well you will be productive. If you code it well they will come.

A plugin system is well architected. You can write the plugins in $LANGUAGE_OF_THE_WEEK. A core in C has few negatives especially now it has reached the critial mass of being productive for the coders working on it. Re: suggestions to rewrite Compiz in shell script - See my point about good design, get it to that critical mass, pretty much code it and they will come.....

The rest of this is noise.

[RYX]

I think I don't really get your point ...

The end-user obviously doesn't care what language is used to write a plugin - it would attract more developers to work on compiz-plugins if we had a fully working version of compiz-cil (or compiz-sharp as it is accidently called) ... and that's what compiz really needs - more plugin-coders.

The compiz-architecture is optimal for its purpose and there is not much that needs to be changed, instead things should be simplified in form of a higher-level language interface. A plugin that allows to load cil-based plugins is by far the most flexible and elegant solution. The end-user wouldn't even notice a difference when using a cil-based plugin, it behaves similar to all other plugins (thanks to David's pluginloader)

[nzjrs]

I think I don't really get your point ...

I was mostly being facetious.

We are in agreement. We have a good system. All this language argument stuff is just noise.

The core has reached a point where it is stable and efficient to develop in C. The plugin system is decoupled sufficiently that plugines will eventually (if not now) be able to be done in $LANGUAGE_OF_THE_WEEK

[RYX]

Nothing to add ... :D

[Kristian]

One word:

Cache.

or another:

Cache misses.

Inline doesn't even come into the picture. If you read any good book on C, or rather the unix philosophy, you will know that a function is supposed to do one thing and do it well.

And a few questions:

How often do you really change your desktop geometry?
How often do you you receive atom changes for brightness, opacity and saturation?
How often do you trigger a window move/resize client message?
How often do you change the state of a window?
How often do you change the number of desktops?
How often does windows move across _desktops_?
How often do you change desktop for that matter?
How often do you jump in and out of show desktop ?
How often do you create new windows?
How often do you destroy old windows?
How often do you map and unmap windows?
How often do you reparent your windows?
How often does a window change windowtype?
How often do you move your mouse in and out of windows?

Sorry to say this guys, but anyone who doesn't realise that splitting the event code is a good thing doesn't know the code very well.

[RYX]

If you read any good book on C, or rather the unix philosophy, you will know that a function is supposed to do one thing and do it well.
Only one question - where do you see the problem in the event-handling then? It does one thing very well: handling events ... nothing more, nothing less.

I don't see any benefit in having a big bunch of functions with 10 lines each that are too specialized to reuse them anyway. It seems like a quite stupid approach to "simplify" chaos with more chaos ...

:)

[Kristian]

If you read any good book on C, or rather the unix philosophy, you will know that a function is supposed to do one thing and do it well.
Only one question - where do you see the problem in the event-handling then? It does one thing very well: handling events ... nothing more, nothing less.

I don't see any benefit in having a big bunch of functions with 10 lines each that are too specialized to reuse them anyway. It seems like a quite stupid approach to "simplify" chaos with more chaos ...

:)

Read my post.
Read the code.
Learn how cache works.
THEN Post.

In that order.

No reason for me to argue this further since you obviously only read parts of what I'm saying and you post without understanding what I'm saying. And notice my complete lack of polite behavior since I've allready been called several less than nice things by the compiz-community. I apologise to those who take offence at this, but really, this behavior has to stop.

If you want a real discussion, I'm all ears. Really, I want to hear what people mean, but so far I've not really read many unbiased posts in this thread. It's mostly people just having too much pride (for what? You didn't write compiz...) and refusing to listen to logic.

If you actually read the code, you will know that parts of it CAN be reused, parts of it needs better error reporting to track down strange behavior, and the vast majority of it does not benefit from beeing inline. And none of it benefits from beeing inlined on the source level.

Arguments about inline portability is irrelevant since you won't be running an X server with the composite extension and not have a C99 capable compiler. Arguments about speed is irrelevant too since inline takes care of that, and since reducing the cache usage actually improves the speed (Read any good document discussing inlining). Arguments about "it has allways been done that way!" are just, in lack of a better word, retarded. So what if everyone has insisted on doing something stupid? It's not even true. Arguments about big functions beeing easily read are just ways of putting your hands over your ears and singing "lalalalala!" very loud.

Again, if you have any arguments based on real knoweledge, not just something you just read about on wikipedia 20 minutes ago or something someone else said, I'm all ears, but so far you haven't exactly shown much sign of that...

[RYX]

Indeed you're missing politeness. I think there are forums where you get kicked if you talk to a Moderator like that (consider this a first and final warning) ... I read all you were saying but you are not saying much at all, instead you're asking silly rethorical questions.

If you want to get heard - stick to the beryl mailing-list ... or have you lost your buddies' attention?

:P

[RYX]

OK, so after you've edited your post ... maybe we start the other way round. Why don't you just briefly describe again where you see the benefits of splitting the event-handling code? As I said - rethorical questions have no big informational value ...

[stalynx]

Breaking down the event handling code into smaller functions and then inline'ing those functions might give you a slight performance boost. However whether or not this is noticeable is another thing. Also it might create some unexpected behavior.

It'll be interesting to see some benchmarks.

[Kristian]

RYX: Cache. Readability. Maintainability. Documentation.

stalynx: It's not the inlining that will make it faster, the code is allready inline, that's the problem. I doubt it will be a noticeable performance boost, but it will boost the performance in that there will be fewer cache misses. Unexpected behavior is what you allready have. When you have a function that does one thing, there shouldn't be any unexpected behavior. I.E: You put something into it and it does the same thing. Regardless of what's inside the function. If you have one huge function that does several things, you don't have the same level of predictability.

The reason you think leaving it as-is will avoid unexpected behavior is, I assume (and correct me if I am wrong), because you think there will be a lot of rewriting. Rewriting of this part of the code isn't really necesarry, it's just about moving code around, so what the code does won't change, only where it is done. The actual handeling code is fairly simple and there's not much reason to rewrite it. Since the majority of the code is hardly ever executed, makeing sure it doesn't take up valuable cache space is a good thing. And the code that executes very frequently will be just as fast because of inlining. Since there is no speed loss due to the above mentioned reasons, and the change is trivial, I don't see a reason why NOT to do it, as it improves the code for new developers, and it also makes sure we don't have code duplication. It also makes it easier to maintain.

[stalynx]

stalynx: It's not the inlining that will make it faster, the code is allready inline, that's the problem.

Hrm I don't see any inline declarations anywhere. Are we looking at event.c or something else?

[RYX]

@Kristian: If you want to change things in the core you should write a mail to the compiz-mailinglist and see what happens. David is always open for good ideas and quality patches.

Edit: You edited, I edited - peace?

[racarr]

If you read any good book on C, or rather the unix philosophy, you will know that a function is supposed to do one thing and do it well.
Only one question - where do you see the problem in the event-handling then? It does one thing very well: handling events ... nothing more, nothing less.

:)


/* compositeWindowManager(void)
* Function has one purpose:
* Implement composite window manager.
* Arguments: None
*/
static void compositeWindowManager(void);

On a more serious note, why does any of this matter?

Really it's functionally the same and it's a matter of personal preference. It happens to be that a larger portion of coders would consider it split in to multiple functions a more ideal setup. Some people disagree.

To me this seems like arguing about

if (a) {
}

vs.

if (a)
{
}

[RYX]

Concerning the handleEvent-function I'd say it is a clear matter of taste. If someone creates a patch and it is such a great improvement, I am sure David will accept it into the core. If it is sooo important for some of you you should give it a try ..

Edit: And I agree with racarr - keyword: personal preference ...

Edit2: But if you consider the size and dimensions of compiz and take the overall lines of code into account, the handleEvent-function suddenly becomes really tiny and it is not uncommon to leave it like that ... I think most people here never wrote a sourcecode of such a size.

[RAOF]

...
stalynx: It's not the inlining that will make it faster, the code is allready inline, that's the problem. I doubt it will be a noticeable performance boost, but it will boost the performance in that there will be fewer cache misses. Unexpected behavior is what you allready have. When you have a function that does one thing, there shouldn't be any unexpected behavior. I.E: You put something into it and it does the same thing. Regardless of what's inside the function. If you have one huge function that does several things, you don't have the same level of predictability.
...

How do you get fewer cache misses by breaking the function up and then making all the subfunctions inline? If the compiler respects your inline declaration, you've just changed the source code, not the resultant object code - the inline functions get inlined back into one :).

You might get fewer cache misses with non-inlined functions, but then you've got the whole function-call overhead.

[Kristian]

RYX: Peace.

stalynx: It's inlined at the source level (if that's even a real term), putting it in functions and then declearing it inline should produce more or less the exact same result when compiled, though it's up to the compiler, not the author, so the machine code could deviate slightly.

What I mean is that performance-wise it's allready inlined. So splitting the diffrent parts out in functions, and declearing those inline, won't change the speed (again, this depends slightly on the compiler). However, splitting out the parts that are less-used and NOT declearing them inline, will keep them out of the CPU cache. And splitting them into seperate functions will also make it easier to spot code duplication and re-use the code (I've allready spotted a situation like this though not fixed it yet atm in Beryl). But the main reason to do this is makeing it easier to maintain and read.

The event code is also just an example, I've gone over other parts of the code too. Most of the ideas David has used are good, but I feel the last finish is missing. This doesn't really mean it's bugged or strictly BAD per se, just not as perfect as it could be. Beeing a purist, this bothers me. I don't really think that splitting the event code will make Beryl a lot better over night, but I do belive that general type of work will help a lot in the long run. It's boring work, it's tedious work, it's work that noone really sees, but I feel it's the right thing to do. Maybe I am wrong and that my work will just be in vain, but don't think of it as a way of saying that this is far better than how the original code looked. Then you're reading me entierly wrong.

In the example of the event handling code in event.c, it's fairly clean but it was just too huge to be really clean. The eventLoop() was slightly messier, and this was maybe more important because it used an excessive amount of variables (A CPU, after all, has a very limited amount of registers), and it was fairly complicated for beeing a single function. It's also probably the most important function of Compiz and Beryl for obvious reasons (of course that can be debated, there's no definite answer there).

So do I think David can't code for .... ? Well, I know I've stated so in the past, but that was a rather colorfull exageration as I hope people remember. I think David knows what he's doing quite well. I also belive he has a solid amount of programming experience. But that doesn't make his code automatically clean. I would consider him highly productive (Look at his recent work on X for instance), maybe too much so for a guy like me. I've seen projects gone bad because things happened too fast in the past. I don't think that's happening with Compiz, but I see some similarities. But unclean code does NOT equal bad code, keep that in mind.

I don't belive this "code quality" is why the fork happened, nor do I hope any of you belive that's our reasoning, because we haven't done a whole lot better in that general area yet ourself to be honest. So don't take my comments about the code quality the wrong way, please :)

[stalynx]

Just because the source code is "inline" does not mean the resultant object code is itself inline. With GNU C you need to inline a function with the inline declaration.

[Kristian]

How do you get fewer cache misses by breaking the function up and then making all the subfunctions inline? If the compiler respects your inline declaration, you've just changed the source code, not the resultant object code - the inline functions get inlined back into one :).

You might get fewer cache misses with non-inlined functions, but then you've got the whole function-call overhead.

You obviously get just as many cache-missess if we just split the code into functions then inline it back again, you're absoloutly right about this, that's why there's not much of a point discussing the performance of inlined functions one way or another. Performance-wise they're just the same as if they were in the original function.

However, most of the code can be non-linlined without any real performance issue. This is why you get fewer cache misses.

Keep in mind that the function in question does everything from handle motionnotify events (granted, that handeling is empty, but MotionNotify events still go through that function), to handle changes of the number of desktops. You obviously move your mouse quite a bit more often than you change the number of desktops you have. Thus splitting the code would reduce the cache usage because the rarely used code will not be cached. And it wouldn't decrease performance because the often-used functions would be inlined.

[RAOF]

That makes sense. Why don't you write a patch that breaks it up in that way? Plus, you could comment it at the same time! :wink:

[nzjrs]

RYX: Peace.

stalynx: It's inlined at the source level (if that's even a real term), putting it in functions and then declearing it inline should produce more or less the exact same result when compiled, though it's up to the compiler, not the author, so the machine code could deviate slightly.

What I mean is that performance-wise it's allready inlined. So splitting the diffrent parts out in functions, and declearing those inline, won't change the speed (again, this depends slightly on the compiler). However, splitting out the parts that are less-used and NOT declearing them inline, will keep them out of the CPU cache. And splitting them into seperate functions will also make it easier to spot code duplication and re-use the code (I've allready spotted a situation like this though not fixed it yet atm in Beryl). But the main reason to do this is makeing it easier to maintain and read.

The event code is also just an example, I've gone over other parts of the code too. Most of the ideas David has used are good, but I feel the last finish is missing. This doesn't really mean it's bugged or strictly BAD per se, just not as perfect as it could be. Beeing a purist, this bothers me. I don't really think that splitting the event code will make Beryl a lot better over night, but I do belive that general type of work will help a lot in the long run. It's boring work, it's tedious work, it's work that noone really sees, but I feel it's the right thing to do. Maybe I am wrong and that my work will just be in vain, but don't think of it as a way of saying that this is far better than how the original code looked. Then you're reading me entierly wrong.

In the example of the event handling code in event.c, it's fairly clean but it was just too huge to be really clean. The eventLoop() was slightly messier, and this was maybe more important because it used an excessive amount of variables (A CPU, after all, has a very limited amount of registers), and it was fairly complicated for beeing a single function. It's also probably the most important function of Compiz and Beryl for obvious reasons (of course that can be debated, there's no definite answer there).

So do I think David can't code for .... ? Well, I know I've stated so in the past, but that was a rather colorfull exageration as I hope people remember. I think David knows what he's doing quite well. I also belive he has a solid amount of programming experience. But that doesn't make his code automatically clean. I would consider him highly productive (Look at his recent work on X for instance), maybe too much so for a guy like me. I've seen projects gone bad because things happened too fast in the past. I don't think that's happening with Compiz, but I see some similarities. But unclean code does NOT equal bad code, keep that in mind.

I don't belive this "code quality" is why the fork happened, nor do I hope any of you belive that's our reasoning, because we haven't done a whole lot better in that general area yet ourself to be honest. So don't take my comments about the code quality the wrong way, please :)

OK no problem. Remember to submit your patches up stream. Being a good open source citizen helps the whole FOSS ecosystem.

David has been discussing cleaning up parts of the core of late (are you subscribed to the compiz ML?). It would be good to hear your thoughts as it appears you are the defacto beryl project leader these days - we have not seen a post on the compiz ML from quinn for 4 months.

Remember increasing the delta between the two projects helps no one. Thats one of the core things us compiz-guys keep arguing about.