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

[Walter Bender]

Thanks for all of this great feedback...

On Mon, Aug 22, 2011 at 5:18 PM, Sascha Silbe <silbe at activitycentral.com> wrote:
> 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].

Do we have a good example of using distutils to create a Sugar bundle?

>
>> +    # 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.

I ave struggled with this. I wanted a name that could be used for the
bundle id and incorporated into any posix paths, but to play it safe,
I stuck to alphanumeric characters... probably too strict. I also
wanted to be able to distinguish between clones made by two different
users with the same nick, hence the "unique" factor.

Open to suggestions here.

>
>> +        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/
>


-walter
-- 
Walter Bender
Sugar Labs
http://www.sugarlabs.org