I really hate to nit-pick, but it's still not correct :/ _backlight = XInternAtom (_ecore_x_disp, RANDR_PROPERTY_BACKLIGHT, True);
should be: _backlight = XInternAtom(_ecore_x_disp, RANDR_PROPERTY_BACKLIGHT, True); IE: We don't use spaces between function and function parameters. This is done in a few places in this patch, and yet others in this patch are ok: XRRPropertyInfo *info = XRRQueryOutputProperty(_ecore_x_disp, output, _backlight); So we have a 'mixed' bit of formatting here :/ Also: if (actual_type != XA_INTEGER || nitems != 1 || actual_format != 32) Should be done like: if ((actual_type != XA_INTEGER) || (nitems != 1) || (actual_format != 32)) And again here: if (info->range && info->num_values == 2) Should be: if ((info->range) && (info->num_values == 2)) Bad: if(!_ecore_x_randr_output_validate(root, output)) Good: if (!_ecore_x_randr_output_validate(root, output)) Please declare your variables at the beginning of the function .. NOT inline like these: + /* get the ressources */ + XRRScreenResources *resources = _ecore_x_randr_get_screen_resources (_ecore_x_disp, root); + int o; + for (o = 0; o < resources->noutput; o++) Functionally, the patch looks sane. I have not tested it tho. If you could address the above issues, I'd be more than happy to apply locally and test it out. Cheers, devilhorns On 02/09/2011 02:16 PM, Tom Hacohen wrote: > Thanks a lot, sorry for bugging you, but it makes life easier for everyone. > Sorry I can't commit, but I honestly don't know ecore well enough. > > Thanks for your quick adjustments, > Tom. > > On Wed, Feb 9, 2011 at 9:10 PM,<mathieu.taillefum...@free.fr> wrote: > >> enclosed is the split version of the patch. >> >> The first one remove trailing whitespaces. The second one implement >> backlight support in Ecore. The ChangeLog is modified accordingly. >> >> Best >> >> Mathieu >> >> ----- Mail Original ----- >> De: "Tom Hacohen"<t...@stosb.com> >> À: "mathieu taillefumier"<mathieu.taillefum...@free.fr> >> Cc: "Tom Hacohen"<tom.haco...@partner.samsung.com>, "enlightenment-devel" >> <enlightenment-devel@lists.sourceforge.net> >> Envoyé: Mercredi 9 Février 2011 19h10:17 GMT +01:00 Amsterdam / Berlin / >> Berne / Rome / Stockholm / Vienne >> Objet: Re: [E-devel] [PATCH][Ecore] second version of the backlight >> functions >> >> Regarding formatting and logic: I mean actual code changes and spaces >> changes. >> For example you changed those empty lines at the start but also changed >> features, >> yes, your patch is short and easy to understand as is, but since I don't >> work on ecore >> I can't really tell if it's good or not so I won't commit it anyway. >> >> What I was able to say, is that the patch was missing the changelog >> (although you did >> add it) and had mixed formatting and logic changes. :) >> >> Regarding your question: >> Yes, I think it's generally good to split to two patches. >> >> Just got Lucas's mail, he's right :) >> >> On Wed, Feb 9, 2011 at 7:50 PM,<mathieu.taillefum...@free.fr> wrote: >> >>> Hey Tom, >>> >>> I don't know ecore enough to commit it myself, but I still have a couple >>> of comments: >>> 1. Your patch includes both logic and formatting changes, please split >>> to two different patches. >>> >>> Two patches for three functions of fifteen lines each ? The formatting is >>> changed because I had to add a single line on the top of the file and my >>> emacs still does not want to format things the right way. Something is >>> furiously wrong in my .emacs file. >>> >>> I do not know what do you mean about logic. Could you be more precise. >> The >>> only thing I find too rigid is to separate the patch into two patches. It >> is >>> very short, so I did not feel like to separate it into two parts >>> >>> 2. Please also include the ChangeLog change in the same patch. >>> >>> I Will do it. >>> >>> Also, if possible please also include a nice svn log message, for >>> example: >>> "Ecore ecore-x: Implemented the stub backlight support functions." >>> >>> fine by me. >>> >>> Mathieu. >>> >>> >>> >> ------------------------------------------------------------------------------ >>> The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE: >>> Pinpoint memory and threading errors before they happen. >>> Find and fix more than 250 security defects in the development cycle. >>> Locate bottlenecks in serial and parallel code that limit performance. >>> http://p.sf.net/sfu/intel-dev2devfeb >>> _______________________________________________ >>> enlightenment-devel mailing list >>> enlightenment-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel >>> >> >> >> >> -- >> Tom. >> ------------------------------------------------------------------------------ The ultimate all-in-one performance toolkit: Intel(R) Parallel Studio XE: Pinpoint memory and threading errors before they happen. Find and fix more than 250 security defects in the development cycle. Locate bottlenecks in serial and parallel code that limit performance. http://p.sf.net/sfu/intel-dev2devfeb _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
