Hi Alan,
Thanks for your review.
Alan Coopersmith wrote:
> Erwann Chenede wrote:
>> Hi All,
>>
>> This fix adds 2 new scripts to the SUNWcompiz package. See
>> http://www.opensolaris.org/jive/thread.jspa?messageID=209065𳂩
>> for the architecture (ARC) details.
>> One script (compiz-configure) is a pyhton GUI that checks the
>> system capabilities wrt to running compiz. The second one is a perl
>> script
>> that modify the Xserver configuration to run compiz on the system.
>>
>> The source can be found here :
>> http://www.gnome.org/~erwannc/compiz/gnome-integration-0.6.2.v1.tar.bz2
>
> I've only looked at the modify-xorg-conf script in there, and I've got
> some
> questions.
>
> - Unless I'm misreading the {} nesting, you're calling
> /usr/bin/nvidia-xconfig in configure_xorg_conf_intel - is that
> really the right thing to do?
No, the right thing would be to have a fully blown xorg.conf parser for
intel.
I'm just short in time for integration.
I used this trick in my install bundle to be able to generate a xorg.conf
without having to use Xorg -configure. I've left it like this as it is
working :)
My plan is to get the drivers to enable these features by default in the
next releases.
And dropping these scripts when it's done....
>
> - Checking prtconf for the string "nvidia" seems like you risk hitting
> false positives when other nvidia devices are present, like nvidia
> ethernet chips. I'd check /usr/X11/bin/constype output for NVDAnvda.
> (That's how the OpenGL library version selector & the Xorg server
> determine if the nvidia driver is in use.)
I've added this check for the nvidia drivers.
I did it that way originally because /usr/X11/bin/constype didn't always
return something
meaningful on all systems (blades, sparc, etc.)
> - The hardcoded list of pci id's for Intel & ATI chipsets also seems
> like
> it will be a challenge to keep accurate and up-to-date.
Yes, I agree, but this is the only way to figure out which chipset is
supported
for Intel and ATI (at lease it is what I've been told by the drivers
developers). If there
is a nicer way, I'll gladly change to it.
>
> - While not required, bonus points if your perl script follows the
> guidelines
> in http://muskoka.eng/~sch/perl/style.html (I don't know if those are
> published externally yet) - at the least, expect a bug report filed by
> Install QA on any package that includes a perl script and doesn't have
> perl listed in the package dependencies or doesn't use
> /usr/perl5/bin/perl
> instead of /usr/bin/perl to ensure it uses the OS-bundled perl
> instead of
> another version the sysadmin changed the /usr/bin/perl link to
> point to.
I've made all the recommended changes.
>
> - Stylistically, there's a lot more `` commands than I'd use in a
> perl script
> `date` could be replaced by POSIX::strftime, `mv` by rename(), etc.
I've fixed all these too :)
The revised version is available here :
http://www.gnome.org/~erwannc/compiz/gnome-integration-0.6.2.v1.tar.bz2
Thanks,
Erwann
--
Erwann Ch?ned?,
Desktop Group, Sun Microsystems, Grenoble
Phone : +33 476 188 358 ext: 38358
[ I speak for myself, not for my employer ]
