Thanks a lot for the review Sascha, I will improve these things.<br><br><div class="gmail_quote">On Tue, Aug 21, 2012 at 9:38 AM, Sascha Silbe <span dir="ltr"><<a href="mailto:silbe@activitycentral.com" target="_blank">silbe@activitycentral.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Martin Abente Lahaye <<a href="mailto:martin.abente.lahaye@gmail.com">martin.abente.lahaye@gmail.com</a>> writes:<br>

<br>
[src/jarabe/journal/volumestoolbar.py]<br>
[_set_up_activities_examples()]<br>
[...]<br>
<div class="im">> +        activities = []<br>
> +        for home_activity in home_model._activities:<br>
> +            if home_activity.is_journal():<br>
> +                continue<br>
> +<br>
> +            activity_name = home_activity.get_activity_name()<br>
> +            if activity_name not in activities:<br>
> +                self._add_activity_example(home_activity)<br>
> +                activities.append(activity_name)<br>
<br>
</div>I wonder if using a dictionary for deduplication would be better<br>
here. E.g.:<br>
<br>
    activities = dict([(home_activity.get_activity_name(), home_activity)<br>
                       for home_activity in home_model])<br>
    for home_activity in activities.values():<br>
        self._add_activity_examples(home_activity)<br>
<br>
<br>
Alternatively, you could just iterate over all entries and let<br>
_add_activity_examples() take care of deduplication (as it has to do<br>
anyway), at the risk of making it O(n²).<br>
<br>
<br>
> +    def _proper_occurrence(self, home_activity):<br>
<br>
What's a "proper" occurrence?<br>
<div class="im"><br>
> +        if home_activity.is_journal():<br>
> +            return False<br>
> +<br>
> +        home_model = shell.get_model()<br>
> +        act_name = home_activity.get_activity_name()<br>
> +        activities = filter(lambda act: act_name == act.get_activity_name(),<br>
> +                        home_model._activities)<br>
> +<br>
> +        return len(activities) <= 1<br>
<br>
</div>I assume this is for deduplication again. The easiest and fastest way<br>
overall (O(1) per iteration [1], O(n) in total) is probably a<br>
ref-counting scheme:<br>
<br>
    def __init__(self, ...):<br>
        # ...the usual stuff...<br>
<br>
        self._activity_counts = {}<br>
        self._example_buttons = {}<br>
        for home_activity in home_model:<br>
            self._add_activity(home_activity)<br>
<br>
    def _add_activity(self, home_activity):<br>
        """Keep track of an active activity session<br>
<br>
        Keep track of an active activity session and add the examples<br>
        folder if available and not already shown.<br>
        """<br>
        name = home_activity.get_activity_name()<br>
        old_count = self._activity_counts.get(name, 0)<br>
        self._activity_counts[name] = old_count + 1<br>
        if old_count:<br>
            return<br>
<br>
        # ...create button...<br>
<br>
        self._example_buttons[name] = example_button<br>
<br>
    def _remove_activity(self, home_activity):<br>
        """Stop keeping track of an active activity session<br>
<br>
        Remove all internal references to this session. Remove the<br>
        corresponding examples folder if it's the last instance of<br>
        this activity.<br>
        """<br>
        name = home_activity.get_activity_name()<br>
        old_count = self._activity_counts[name]<br>
        self._activity_counts[name] = old_count - 1<br>
        if old_count > 1:<br>
            return<br>
<br>
        example_button = self._example_buttons.pop(name)<br>
<br>
        # ...remove button...<br>
<div class="im"><br>
<br>
<br>
> +    def _add_activity_example(self, home_activity):<br>
> +        examples_path = os.path.join(home_activity.get_bundle_path(),<br>
> +                                    'examples')<br>
> +<br>
> +        if not os.path.exists(examples_path):<br>
> +            return<br>
<br>
</div>If you add a guard against home_activity.get_bundle_path() returning<br>
None, you can drop the special-casing for the Journal activity.<br>
<br>
<br>
Sascha<br>
<br>
[1] <a href="http://wiki.python.org/moin/TimeComplexity#dict" target="_blank">http://wiki.python.org/moin/TimeComplexity#dict</a><br>
<span class="HOEnZb"><font color="#888888">--<br>
<a href="http://sascha.silbe.org/" target="_blank">http://sascha.silbe.org/</a><br>
<a href="http://www.infra-silbe.de/" target="_blank">http://www.infra-silbe.de/</a><br>
</font></span></blockquote></div><br>