[Sugar-devel] [PATCH sugar] Control Panel: making about my computer section hardware independent, OLPC #11232

[Simon Schampijer]

On 10/08/2011 09:57 PM, Sascha Silbe wrote:
> Excerpts from Simon Schampijer's message of 2011-10-07 11:13:15 +0200:
>> '/ofw' is not used anymore on the XO, '/proc/device-tree' is used now
>> instead. Instead of just adjusting for that change we took the
>> chance to make the section hardware independent. As the firmware
>> version we display the bios version if available on non XO
>> hardware. As ethtool has become a depedency of Sugar we can now
>                                      ^ dependency
>
>> display the wireless firmware as well on all hardwares.
>
> There's no plural form of hardware. Maybe "any hardware"?
> Not that important, though.
>
>> The serial number is often only readable by root on non-XO
>> hardware. If the serial number can not be found on an XO we hide
>> that part of the section.
>>
>> The patch has been tested on XO 1, 1.5, 1.75 and on a Thinkpad T61.
>
> I like the general approach. Thanks for considering (and even testing
> on) non-XO hardware!
>
>
> [extensions/cpsection/aboutcomputer/model.py]
>> @@ -17,7 +17,6 @@
>>
>>   import os
>>   import logging
>> -import re
>>   import subprocess
>>   from gettext import gettext as _
>>   import errno
>> @@ -35,6 +34,7 @@ _NM_DEVICE_TYPE_WIFI = 2
>>
>>   _OFW_TREE = '/ofw'
>>   _PROC_TREE = '/proc/device-tree'
>> +_SYS_TREE = '/sys/class/dmi/id'
>
> Both /ofw and /proc/device/tree are mount points for the OFW device
> tree. /sys/class/dmi/id OTOH contains different entries. So
> _DMI_DIRECTORY might be a better name.

Is fine with me, done.

>> @@ -58,6 +58,8 @@ def get_serial_number():
>>           serial_no = _read_file(os.path.join(_OFW_TREE, _SN))
>>       elif os.path.exists(os.path.join(_PROC_TREE, _SN)):
>>           serial_no = _read_file(os.path.join(_PROC_TREE, _SN))
>> +    else:
>> +        return None
>>       if serial_no is None:
>>           serial_no = _not_available
>>       return serial_no
>
> We are hiding the serial number completely if the kernel doesn't expose
> device tree information, but show "Not available" if the file couldn't
> be read or parsed. That doesn't really match the patch description.
>
> How about:
>
> def get_serial_number():
>      if os.path.exists(os.path.join(_OFW_TREE, _SN)):
>          return _read_file(os.path.join(_OFW_TREE, _SN))
>      elif os.path.exists(os.path.join(_PROC_TREE, _SN)):
>          return _read_file(os.path.join(_PROC_TREE, _SN))
>      elif os.path.exists(os.path.join(_DMI_DIRECTORY, 'board_serial'):
>          return _read_file(os.path.join(_DMI_DIRECTORY, 'board_serial'))
>      return None

Ahh, maybe not as clear from the description. The board_serial is only 
readable by root. Knowing that it does not really make sense to try to 
read it. That is why I special cased displaying the serial number. The 
behavior has not changed to previous code, so I think it is good like 
that, espacially as a bug fix patch.

>> @@ -100,14 +102,14 @@ def get_firmware_number():
>>       firmware_no = None
>>       if os.path.exists(os.path.join(_OFW_TREE, _MODEL)):
>>           firmware_no = _read_file(os.path.join(_OFW_TREE, _MODEL))
>> +        firmware_no = firmware_no[6:13]
>>       elif os.path.exists(os.path.join(_PROC_TREE, _MODEL)):
>>           firmware_no = _read_file(os.path.join(_PROC_TREE, _MODEL))
>> +        firmware_no = firmware_no[6:13]
>> +    elif os.path.exists(os.path.join(_SYS_TREE, 'bios_version')):
>> +        firmware_no = _read_file(os.path.join(_SYS_TREE, 'bios_version'))
>
> The above assumes OLPC format (e.g. "CL2   Q4B11  Q4B") for
> .../openprom/model and will break on other machines with device tree
> (e.g. PPC Macs). The old code at least only did the cutting if there
> actually were three space-separated parts:

Ok, will revert to old format.

>>       if firmware_no is None:
>>           firmware_no = _not_available
>> -    else:
>> -        firmware_no = re.split(' +', firmware_no)
>> -        if len(firmware_no) == 3:
>> -            firmware_no = firmware_no[1]
>>       return firmware_no
>
>
> [extensions/cpsection/aboutcomputer/view.py]
>> @@ -127,37 +128,36 @@ class AboutComputer(SectionView):
>>           box_software.pack_start(box_sugar, expand=False)
>>           box_sugar.show()
>>
>> -        if os.path.exists('/ofw'):
>
> I wonder if we should hide the Firmware version if it's unavailable,
> like we now do for the serial number. Is it useful to know that Sugar
> would display it, but can't access it? Could it confuse users if it
> doesn't get mentioned at all?
>
> Sascha
>

In general, I prefer the approach to show the same items all the time 
and disable them or mark them as not available like we do in the rest of 
the UI a lot (e.g. with buttons).

For the future I would as well always show the identity section, with 
the model and the serial number marked as unreadable/not available.

Regards,
    Simon