[Sugar-devel] [PATCH] Add-clone-to-view-source-submenu

[Sascha Silbe]

Excerpts from Walter Bender's message of Fri Aug 19 00:11:53 +0200 2011:

[src/jarabe/view/customizebundle.py]
[...]
> +import logging
> +_logger = logging.getLogger('ViewSource')

Please add two empty lines between the imports and the global variable
to make this easier to read (might also be in PEP-8).

> +CUSTOMICONSUBPATH = 'emblems/emblem-view-source.svg'
> +BADGETRANSFORM = '  <g transform="matrix(0.45,0,0,0.45,32,32)">\n'

Again for readability, please separate words using underscores ('_').

> +def generate_unique_id():
> +    ''' Generate an id based on the user's nick name and their public key
> +    (Based on schema used by IRC activity). '''

For consistency with other parts of the code, please use """ instead of
'''.

> +    nick = sugar.profile.get_nick_name()
> +    pubkey = sugar.profile.get_pubkey()
> +    m = hashlib.sha1()
> +    m.update(pubkey)
> +    hexhash = m.hexdigest()

Side note: We should probably add sugar.profile.get_pubkey_hash().

> +    # Get the alphabetic bits of the nick
> +    nick_letters = "".join([x for x in nick if x.isalpha()])

Again for consistency, please use ' instead of " for strings.

> +    # If that came up with nothing, make it 'XO'.  Also, shorten it if
> +    # it's more than 11 characters (as we need 5 for - and the hash).
> +    if len(nick_letters) == 0:

PEP-8: if not nick_letters:

> +        nick_letters = 'XO'
> +    elif len(nick_letters) > 11:
> +        nick_letters = nick_letters[0:11]

[:11] would be more intuitive to me, but maybe not to others. We can get
rid of the conditional, though, since the slice will work even if there
are fewer letters.


> +    # Finally, generate an id by using those letters plus the first
> +    # four hash bits of the user's public key.
> +    return(nick_letters + '_' + hexhash[0:4])

Please add a space and remove the parentheses. Return is a statement,
not a function.


> +def generate_bundle(nick, activity_name, home_activities, new_bundle_name):
> +    ''' Generate a new .xo bundle for the activity and copy it into the
> +    Journal. '''
> +
> +    # First remove any existing bundles in dist/
> +    if os.path.exists(os.path.join(home_activities, new_bundle_name, 'dist')):
> +        command_line = ['rm', os.path.join(home_activities, new_bundle_name,
> +                                           'dist', '*.xo')]
> +        _logger.debug(subprocess.call(command_line))
> +        command_line = ['rm', os.path.join(home_activities, new_bundle_name,
> +                                           'dist', '*.bz2')]
> +        _logger.debug(subprocess.call(command_line))

Sounds like a job for os.remove() and glob.glob().

> +    # Then create a new bundle.
> +    config = bundlebuilder.Config(source_dir=os.path.join(
> +            home_activities, new_bundle_name),
> +            dist_name='%s_%s-1.xo' % (nick, activity_name))
> +    bundlebuilder.cmd_fix_manifest(config, None)

We don't have MANIFEST anymore.

> +    bundlebuilder.cmd_dist_xo(config, None)

I'd prefer not to use bundlebuilder in new code as we should replace it
with distutils [1].

> +    # Finally copy the new bundle to the Journal.
> +    dsobject = datastore.create()
> +    dsobject.metadata['title'] = '%s_%s-1.xo' % (nick, activity_name)

I wonder whether we should really use something that looks like a file
name for the title. I know we do this for files downloaded with Browse,
but while we could (and should) do better in some cases we don't always
have a choice. However when cloning an activity, we have all information
at hand. So how about '%(nick)ss clone of %(activity_name)s'?


> +def customize_activity_info(nick, home_activities, new_bundle_name):
> +    ''' Modify bundle_id in new activity.info file: (1) change the
> +    bundle_id to bundle_id_[NICKNAME]; (2) change the activity_icon
> +    [NICKNAME]-activity-icon.svg; (3) set activity_version to 1.
> +    Also, modify the activity icon by applying a customize overlay.
> +    '''

The list would be more readable if each item started on a separate line.

> +    activity_name = ''
> +
> +    info_old = open(os.path.join(home_activities, new_bundle_name,
> +                                 'activity', 'activity.info'), 'r')
> +    info_new = open(os.path.join(home_activities, new_bundle_name,
> +                                 'activity', 'new_activity.info'), 'w')
> +    for line in info_old:
> +        tokens = line.split('=')

This should probably read line.split('=', 1) to prevent us from breaking
if the value part contains equal signs. We can also give them tokens
better names:

        name, value = line.split('=', 1)


> +        if tokens[0].rstrip() == 'bundle_id':

This isn't performance critical, but reading tokens[0].rstrip() eight
times in a row hurts my eyes. ;)

How about:

        name, value = [token.strip() for token in line.split('=', 1)]


> +            new_bundle_id = '%s_%s_clone' % (tokens[1].strip(), nick)
> +            info_new.write('%s = %s\n' % (tokens[0].rstrip(), new_bundle_id))
> +        elif tokens[0].rstrip() == 'activity_version':
> +            info_new.write('%s = 1\n' % (tokens[0].rstrip()))
> +        elif tokens[0].rstrip() == 'icon':
> +            old_icon_name = tokens[1].strip()
> +            new_icon_name = '%s_%s' % (nick, old_icon_name)
> +            info_new.write('%s = %s\n' % (tokens[0].rstrip(), new_icon_name))
> +        elif tokens[0].rstrip() == 'name':
> +            info_new.write('%s = %s_%s\n' % (tokens[0].rstrip(), nick,
> +                                           tokens[1].strip()))
> +            activity_name = tokens[1].strip()
> +        else:
> +            info_new.write(line)

With a slightly different structure we could get rid of some duplicated
formatting:

    for line in info_old:
        name, value = [token.strip() for token in line.split('=', 1)]
        if name == 'bundle_id':
            new_value = '%s_%s_clone' % (value, nick)
        elif name == 'activity_version':
            new_value = '1'
        elif name in ['icon', name]:
            new_value = '%s_%s' % (nick, value)
        else:
            info_new.write(line)
            continue

        info_new.write('%s = %s\n', name, new_value)


> +    info_old.close
> +    info_new.close

The parentheses are missing, turning these into no-ops (caught by
pylint).

> +    command_line = ['mv', os.path.join(home_activities, new_bundle_name,
> +                                       'activity', 'new_activity.info'),
> +                    os.path.join(home_activities, new_bundle_name,
> +                                 'activity', 'activity.info')]
> +    _logger.debug(subprocess.call(command_line))

This can be more easily achieved using os.rename().

> +    _custom_icon(home_activities, new_bundle_name, old_icon_name,
> +                 new_icon_name)

When reading this, I wondered what that function does and needed to look
it up. How about _create_custom_icon() instead?


[...]
> +def _custom_icon(home_activities, new_bundle_name, old_icon_name,
> +                 new_icon_name):
> +    ''' Modify new activity icon by overlaying CUSTOMICON. '''
> +
> +    # First, find CUSTOMICON, which will be used as an overlay.
> +    icon_path = None
> +    for path in gtk.icon_theme_get_default().get_search_path():
> +        if os.path.exists(os.path.join(path, 'sugar', 'scalable',
> +                                       CUSTOMICONSUBPATH)):
> +            icon_path = path
> +            break
> +
> +    if icon_path is None:
> +        # If we cannot find the overlay, just copy the old icon
> +        _logger.debug('%s not found', CUSTOMICON)

You probably meant CUSTOMICONSUBPATH (caught by pylint).

> +        command_line = ['cp', os.path.join(home_activities, new_bundle_name,
> +                                           'activity', old_icon_name + '.svg'),
> +                        os.path.join(home_activities, new_bundle_name,
> +                                     'activity', new_icon_name + '.svg')]
> +        _logger.debug(subprocess.call(command_line))
> +        return

shutil.copy() is a better way to achieve this.


> +    # Extract the 'payload' from CUSTOMICON.
> +    fd_custom = open(os.path.join(icon_path, 'sugar', 'scalable',
> +                                CUSTOMICONSUBPATH), 'r')
[...]

I didn't quite grok the code and will leave it for the next iteration of
review. It does look to be rather complicated however, making me wonder
whether there's an easier way to achieve at least some of this (e.g.
parsing the overlay icon).

[...]
> +    icon_old.close
> +    icon_new.close

Missing parentheses again.


[src/jarabe/view/viewsource.py]
> +    def __copy_to_home_cb(self, menu_item):
> +        ''' Make a local copy of the activity bundle in
> +        $HOME/Activities as [NICK]- '''
> +
> +        # TODO: Check to see if a copy of activity already exisits
> +        # If so, alert the user before overwriting.
> +        home_activities = os.path.join(os.environ['HOME'], 'Activities')

Please use sugar.env.get_user_activities_path() instead.

> +        nick = generate_unique_id()

This is correct from a purely functional point of view, but could do
with some better names. At first glance setting the nick name to an
apparently automatically created unique identifier (which is usually
random or a number sequence) doesn't look like what we'd want.

> +        new_bundle_name = '%s_%s' % (nick, self._document_path.split('/')[-1])

os.path.basename() would be more appropriate here.

> +        command_line = ['cp', '-r', self._document_path,
> +                        os.path.join(home_activities, new_bundle_name)]
> +        _logger.debug(subprocess.call(command_line))

shutil.copytree() with symlinks=True is a good candidate for replacing
the above code.

Sascha

[1] https://bugs.sugarlabs.org/ticket/2947
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20110822/b177c484/attachment.pgp>