Only some newly installed system-wide activities are favorites

[quozl]

Sugar has a list of activities that are automatically added to favourites.

• sugar:data/activities.defaults

Downstream projects such as OLPC replace this file during image build, so that certain activities included in the build are marked as favourites.

But in other downstream projects such as Debian, Ubuntu, and possibly some others, the independent user experience is somewhat unpredictable: when an activity is installed from package repositories, it may or may not appear in favourites, depending on the defaults file.

Sugar should always add newly installed activities to favourites, with the exception of Terminal and Log; e.g. using a file that describes activities that should not be added to favourites on install.

[quozl]

On a fresh install of Ubuntu Trusty 14.04.2, the following activities were not marked as favourite: Abacus, Clock, Develop, Finance, FotoToon, FractionBounce, Jukebox, Labyrinth, Letters, Log, Portfolio, StopWatch, and Terminal. Of these only Log and Terminal should be so marked.

[tch]

I did some tests just to double check the current behavior. Basically there are two scenarios:

a) Activities are installed when sugar IS running: in this case it will always include it in the favorites list, regardless of how it was installed (sugar bundle, system package, etc).

b) Activities are installed when sugar is NOT running: in this case one activity will be favorited, during sugar startup, if:

b.1) the bundle id is in /usr/share/sugar/data/activities.defaults file, OR

b.2) if it was previously favorited and later removed but its bundle id is still in ~/.sugar/default/favorite_activities.

So, what you are suggesting is to change the behavior described above and _always_ favorite activities under scenarios a) and b), and instead, provide some sort of black list to exclude activities only under scenario b)?

[godiard]

In my opinion, change to a black list have sense.

We need check also the behavior of the sugar activity updater.

[tch]

I spent some time looking at the defaults favorites and general favorites code. Lets say, in the scenario b), that we want to favorite everything except for what its declared in the blacklist.

At first glance it seems like a simple change but, once you understand how favorites currently works, it suggest that it will actually require to change not only the defaults favorites code but also the general favorites code. Here is why:

Assume we remove activities.defaults and a new activities.blacklist.defaults file, then during the first Sugar startup it will favorite every activity that is installed at that point except for those in the blacklist.

Then, these favorite activities will then be stored in ~/.sugar/default/favorite_activities along with the general (declared by the user in runtime) favorites. There is no more distinction between defaults and general favorites.

Now, lets say the user removes one favorite that is not in activities.blacklist.defaults, Sugar will remove that entry from ~/.sugar/default/favorite_activities. Now imagine when Sugar restarts, since that activity is not in the blacklist, it will be favorited again (by definition), annoying the user and making the "favorite" concept even more inconsistent.

If we try to workaround this case by checking at ~/.sugar/default/favorite_activities during startup (ie., to make sure is not there to avoid making it a favorite), we will be back to square one, because activities installed when sugar was not running are not included in ~/.sugar/default/favorite_activities anyway, therefore won't be favorited during sugar startup.

So to make this truly consistent, we will need to change the general code for favorites, to think in terms of blacklisted_activities and say that everything is a favorite unless the user say otherwise in runtime. E.g, maintaining a ~/.sugar/default/blacklist_activities instead of ~/.sugar/default/favorite_activities.

Also, the blacklist.defeaults entries will require a double check during startup to make sure these are also in ~/.sugar/default/blacklist_activities, otherwise it would prevent users from favorite Log or Terminal if they want to.

I would appreciate if you guys can double check if this logic is correct, but if it is, it means this change requires some redesign not simply a fix.

[quozl]

Thanks for the redesign work. I agree that preserving the user choice, to either add as favourite or remove as favourite, would achieve best usability outcome.

Not sure I agree with a per-user blacklist file. Instead, since favorite_activities is a JSON structure, and each bundle has either null or a dictionary for recording position in position capable views, two further keys might be added:

• a key uf to represent one of either:
  • added as favourite by user,
  • removed as favourite by user, or;
  • no choice made by user (e.g. key missing),
• a key sf to represent one of either:
  • the system defaults listed the bundle as favourite,
  • the system defaults listed the bundle as not to be a favourite, or;
  • the system defaults made no choice (e.g. key missing).

So to determine whether to show the bundle is a straightforward set of priorities; user choice first, or if no choice made then use system default, or if no system default made then show the bundle.

The system defaults would be "do not show" for Read, Log, and Terminal, and "do show" for Browse. For example, where Terminal has been marked favourite by user, and a new activity installed, the JSON may contain:

  "org.laptop.sugar.ReadActivity 99": {              'sf': False },
  "org.laptop.Log 29":                {              'sf': False },
  "org.laptop.Terminal 39":           { 'uf': True,  'sf': False },
  "NewActivity 1":                    {                          },
  "org.laptop.WebActivity 157.1":     {              'sf': True  },

Fedora on XO laptop deployments will need a patch to olpc-os-builder. Deployments of other operating systems will gain a fix.

(I don't think we are at a stage of assessing the impact and suitability for merging; that can come after design and coding is complete. When that happens relative to release schedule is unknown. The bug severity is minor. If a fix becomes a feature only because of complexity, it's hard to see where the threshold is. I prefer to think of this as a fix rather than a feature, because all the user gains is properly working software.)

[godiard]

I would prefer the following (in my view) simpler solution:

• Add a file /etc/sugar/activities_hidden.default. This file have a simple list with

the activities we don't want by default in the favorite view as Log, Terminal and a few more.

• User selection is done on .sugar/default/favorite_activities as usual. The only change is that file is created based on a blacklist instead of in a whitelist.

Once the favorite_activities was created, the activities_hiden.default file is not needed anymore.

This proposal solve the issue in multi-user systems, is easier to implement, and do not have the problem described by tch on comment 4.

Comments?

[tch]

I have been experimenting with an approach that is similar to James approach, but it is simpler and does not break the current format (which is still necessary to store icon positions for favorites).

This approach follows the "The user has the last word, unless he hasn't said anything" guideline. At the implementation level, the "favorite_activities" JSON is extended with only one more key called "favorite" aside with the current "position" key. This new key stores the favorite state (True or False). The "activities.defaults" file is completely removed.

So how does it work? When Sugar starts up it loads the "favorite_activities" JSON file to the "self._favorite_bundles" list as usual. Then instead of merging the "activities.defaults" (which is now gone) is will be able to detect new activities and favorite them. This behavior is the exact same as it happens when the activities are installed when sugar is running, so it is consistent.

How does it know is a new activity? Because the bundle id is not yet in "favorites_activities" JSON file, which now knows which activities are favorites and which are not (similar to James approach).

How it handles hidden? A new "activities.hidden" file is added. Sugar reads this file during start up and simply does not favorite, unless the user has said something directly (which should be reflected in the JSON file).

An initial working implementation can be found here [1]. The important changes are here [2,3], the rest of the commits are just for removing traces of "activities.defaults".

Please give it a try!

[1] ​https://github.com/tchx84/sugar/commits/enhance-favorites-try1
[2] ​https://github.com/tchx84/sugar/commit/a4e65086d3c8fd18012b4012e6d692ce452ab15a
[3] ​https://github.com/tchx84/sugar/commit/c1886be8c0bd7fceb04735900178aa1247284a2b

[quozl]

Replying to tch:

I have been experimenting with an approach that is similar to James approach, but it is simpler and does not break the current format (which is still necessary to store icon positions for favorites).

I did not propose to break the current format; as far as I could see it could work fine after an upgrade. I do not think we support downgrade.

An initial working implementation can be found here [1]. The important changes are here [2,3], the rest of the commits are just for removing traces of "activities.defaults".

Apart from the removal of "activities.defaults", there doesn't seem to be any API change, do you agree?

[tch]

Replying to quozl:

Apart from the removal of "activities.defaults", there doesn't seem to be any API change, do you agree?

Agreed, and activities.hidden can basically be considered a functionality in the API. In terms of Sugar internals the API remains the same.

[tch]

Replying to quozl:

Apart from the removal of "activities.defaults", there doesn't seem to be any API change, do you agree?

Agreed, and activities.hidden can basically be considered a new functionality in the API. In terms of Sugar internals the API remains the same.

Did you get to try it?

[godiard]

I tested the patches and work, but I found the following issues:

• Add more descriptive header to the commits.
• In particular, I would merge the first and last patches to make easier understand the change.
• _scan_new_favorites() does not have in account the case of multiple home views (see load_favorites).
• In particular this part is over-complicated:

          for bundle in self:
              bundle_id = bundle.get_bundle_id()
              key = self._get_favorite_key(
                  bundle_id, bundle.get_activity_version())
              if key not in self._favorite_bundles[_DEFAULT_VIEW] and \
                      bundle_id in hidden_activities:
                  self._favorite_bundles[_DEFAULT_VIEW][key] = \
                      {'favorite': False}
              elif not self._favorite_bundles[_DEFAULT_VIEW].get(key, None):
                  self._favorite_bundles[_DEFAULT_VIEW][key] = \
                      {'favorite': True}
              elif 'favorite' not in self._favorite_bundles[_DEFAULT_VIEW][key]:
                  self._favorite_bundles[_DEFAULT_VIEW][key]['favorite'] = True
  

I think this should be enough

        for bundle in self:
            bundle_id = bundle.get_bundle_id()
            key = self._get_favorite_key(
                bundle_id, bundle.get_activity_version())

            if key not in self._favorite_bundles[_DEFAULT_VIEW]:
                self._favorite_bundles[_DEFAULT_VIEW][key] = {
                    'favorite': bundle_id not in hidden_activities}

• In "with open(path) as hidden_file:", hidden_file is a confusing name, hidden_activities_files should be better.

Is clear we don't allow change the hidden_activities file after the user setup is done and hide activities already set as favorites by the user, these are activities hidden by default.

[tch]

Replying to godiard:

I tested the patches and work, but I found the following issues:

• Add more descriptive header to the commits.

DONE.

• In particular, I would merge the first and last patches to make easier understand the change.

If I do that, I will also have to merge the removal and addition of .defaults and .hidden files in the same commit, making it too big. This because the commits should respect the transition when the files are needed. Also, I think is easier to understand now.

• _scan_new_favorites() does not have in account the case of multiple home views (see load_favorites).

DONE.

I made the conversion of old JSON file more explicit now and covers all home views.

Note that activities made favorite by the shell always go to the main home view and never to the others. This is also true when activities are installed when the shell is running, so is better to keep the consistency.

• In particular this part is over-complicated:

          for bundle in self:
              bundle_id = bundle.get_bundle_id()
              key = self._get_favorite_key(
                  bundle_id, bundle.get_activity_version())
              if key not in self._favorite_bundles[_DEFAULT_VIEW] and \
                      bundle_id in hidden_activities:
                  self._favorite_bundles[_DEFAULT_VIEW][key] = \
                      {'favorite': False}
              elif not self._favorite_bundles[_DEFAULT_VIEW].get(key, None):
                  self._favorite_bundles[_DEFAULT_VIEW][key] = \
                      {'favorite': True}
              elif 'favorite' not in self._favorite_bundles[_DEFAULT_VIEW][key]:
                  self._favorite_bundles[_DEFAULT_VIEW][key]['favorite'] = True
  

I think this should be enough

        for bundle in self:
            bundle_id = bundle.get_bundle_id()
            key = self._get_favorite_key(
                bundle_id, bundle.get_activity_version())

            if key not in self._favorite_bundles[_DEFAULT_VIEW]:
                self._favorite_bundles[_DEFAULT_VIEW][key] = {
                    'favorite': bundle_id not in hidden_activities}

DONE.

Now that I separated the old JSON file conversion from the scanning, it is simpler now.

• In "with open(path) as hidden_file:", hidden_file is a confusing name, hidden_activities_files should be better.

DONE.

Is clear we don't allow change the hidden_activities file after the user setup is done and hide activities already set as favorites by the user, these are activities hidden by default.

I also tested it with multiple views so is more robust now. I added other e.g., having Log and Terminal in the activities.hidden as defaults.

Please give it a try [1]!

[1] ​https://github.com/tchx84/sugar/tree/enhance-favorites-try3

[quozl]

Thanks!

Tested, with removing and reinstalling Terminal using apt; Terminal was shown in the activity ring when it should not have been. shell.log contained a traceback:

Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/jarabe/model/bundleregistry.py", line 136, in __file_monitor_changed_cb
    self.add_bundle(one_file.get_path(), set_favorite=True)
  File "/usr/lib/python2.7/dist-packages/jarabe/model/bundleregistry.py", line 281, in add_bundle
    bundle_id = bundle.get_bundle_id()
AttributeError: 'NoneType' object has no attribute 'get_bundle_id'

Further detail sent by private mail.

[tch]

Replying to quozl:

Thanks!

Thank you for testing it.

Tested, with removing and reinstalling Terminal using apt; Terminal was shown in the activity ring when it should not have been.

I have addressed the comments from your email.

The issue was that I only covered (by design) the equivalent scenario for processing defaults, so the hidden list was only applied during shell start up. The shell already favorite all the activities when they are installed, my patches did not modify that.

Now [1], I also covered that other scenario for when activities are copied directly to the activities directories or via journal at run-time, so the hidden list is also taken into account then.

Please a have a look [2].

[1] ​https://github.com/tchx84/sugar/commits/enhance-favorites-try5
[2] ​https://github.com/tchx84/sugar/commit/28261e47e65e6fdd8c62b55ec6714c3378a1d3f8

shell.log contained a traceback:

Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/jarabe/model/bundleregistry.py", line 136, in __file_monitor_changed_cb
    self.add_bundle(one_file.get_path(), set_favorite=True)
  File "/usr/lib/python2.7/dist-packages/jarabe/model/bundleregistry.py", line 281, in add_bundle
    bundle_id = bundle.get_bundle_id()
AttributeError: 'NoneType' object has no attribute 'get_bundle_id'

I have tried to reproduce this but couldn't (using Fedora 21 and sugar-build).

The weird thing is that the error appears in a execution path that is not related to the changes of version you tried. The hidden file processing happened only during shell start up nothing in "add_bundle" should be affected by that, specially not the parsing of the bundle files.

There is definitely something wrong (as the logs shows), but would be good to confirm this is unrelated to this changes. Have you tried reproducing it without the changes?

I would not be surprised there are other issues. I solved another while I was improving my previous changes [3], which I think it happens to be what German mentioned the other day.

[3] ​https://github.com/sugarlabs/sugar/commit/a489eae29fce1c9d29515211c02a62da352f31e0

Further detail sent by private mail.

[quozl]

Thanks, ​28261e4 on 'try5 seems to work better, but one remaining problem which may have already been present. See test case #4 below.

---

Test case #1, with the Terminal activity, not a favourite:

• observe Sugar home view,
• remove the package for the activity,
• observe Sugar home view,
• install the package for the activity,
• observe Sugar home view,

Expected result: no change to activities shown. Pass.

---

Test case #2, with the Speak activity, a favourite:

• observe Sugar home view,
• remove the package for the activity,
• observe Sugar home view,
• reinstall the package for the activity,
• observe Sugar home view,

Expected result: the activity disappears from the home view when the package is removed, and reappears when the package is reinstalled. Pass.

---

Test case #3, with the Sugar home view visible, SSH in from another system, then remove all activity packages:

apt remove \
$(dpkg-query -W|grep sugar|grep activity|cut -f1 -d'        ')

Expected result: activities are slowly removed from the view, leaving only activities that are installed in $HOME (if any). Pass.

---

Test case #4, with the Sugar home view visible, SSH in from another system, then install all activity packages;

apt install olpc-activities

Expected result: activities are slowly added to the view, with all activities shown once the install completes. Fail.

Observed result: while many activities do appear, not all of them do.

Whether an activity appears depends on some random element;

• first try: Abacus, Calculate, Chart, Clock, Distance, FotoToon, FractionBounce, Gears, Image Viewer, Implode, Jukebox, Letters, Maze, Memorize, Moon, Physics, Poll, Portfolio, Read, Record, StopWatch, and Write.
• second try: Abacus, Calculate, Chart, Clock, Distance, FotoToon, FractionBounce, Gears, Image Viewer, Jukebox, Letters, Maze, Measure, Memorize, Paint, Poll, Read, Record, StopWatch, Terminal, and Write.

All activities are shown after a restart of Sugar.

My guess is that there is a ​race condition.

[tch]

Regarding your Test case # 4, the BundleRegistry misses some activities in favorites/list because GFileMonitor emits events when there are still no activity/activity.info file in the activity directory, thus the BundleRegistry skips it and the bundle is never added.

This happens randomly (more or less, 3 out of 38), but from what I checked it seems to happen more often on activities packages with bigger sizes e.g, sugar-help-activity.

The problem with this is that the BundleRegistry only interacts with the package manager through the file system events, therefore we can't have full control of what is happening with these directories. From the different things I tried, the simplest option [1] was to add a delay to the processing of the paths in the GFileMonitor callback. This is only an imperfect workaround and can't guarantee that it will work with bigger size packages.

I posted the logs of a couple tests, with [2] and without [3] the workaround. I added custom logging entries to make sense of the different stages.

For [2] I even temporarily changed the GFileMonitor event to G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT instead of G_FILE_MONITOR_EVENT_CREATED because it is supposed to be emitted at the end, but made no difference. It shows how the activity/activity.info file is still no accessible ("bundle_from_dir exists False") even when GFileMonitor emitted the CREATED signal, and that it only happens with a few cases while it does work for the most.

On the other hand, by simply adding the delay [3], all the activity contents can be accessed ("bundle_from_dir exists True").

Alternatives I discarded:

• inotify not detecting all events. I tested with inotifywatch, and verified that it detected every single change in the /usr/share/sugar/activities/.
• GFileMonitor missing events. All events are properly informed to BundleRegistry, not a single event got missed, BUT, I detected a separate problem though: when uninstalling all the activities (see my testing steps below), the /usr/share/sugar/activities/ directory was also removed which makes the GFileMonitor miss some events.
• GFileMonitor rate limits. Changes to the rate limit made no difference at (tried with 0, 1 and 10000).
• Bundle processing workload. The bundle processing workload made no difference, I even tried completely removing the processing and made no difference.

Steps I used to reproduce (using Ubuntu 14.04):

1. sudo dpkg --remove --force-depends sugar-abacus-activity sugar-browse-activity sugar-calculate-activity sugar-chart-activity sugar-chat-activity sugar-clock-activity sugar-develop-activity sugar-distance-activity sugar-finance-activity sugar-fototoon-activity sugar-fractionbounce-activity sugar-gears-activity sugar-get-books-activity sugar-help-activity sugar-image-viewer-activity sugar-implode-activity sugar-jukebox-activity sugar-labyrinth-activity sugar-letters-activity sugar-log-activity sugar-maze-activity sugar-measure-activity sugar-memorize-activity sugar-moon-activity sugar-paint-activity sugar-physics-activity sugar-pippy-activity sugar-poll-activity sugar-portfolio-activity sugar-read-activity sugar-record-activity sugar-speak-activity sugar-stopwatch-activity sugar-story-activity sugar-terminal-activity sugar-turtleart-activity sugar-words-activity sugar-write-activity

2. sudo mkdir /usr/share/sugar/activities/

3. (optional, from a different terminal) tailf ~/.sugar/default/logs/shell.log
4. (optional, from a different terminal) inotifywatch --verbose /usr/share/sugar/activities/

5. sudo apt-get install sugar-abacus-activity sugar-browse-activity sugar-calculate-activity sugar-chart-activity sugar-chat-activity sugar-clock-activity sugar-develop-activity sugar-distance-activity sugar-finance-activity sugar-fototoon-activity sugar-fractionbounce-activity sugar-gears-activity sugar-get-books-activity sugar-help-activity sugar-image-viewer-activity sugar-implode-activity sugar-jukebox-activity sugar-labyrinth-activity sugar-letters-activity sugar-log-activity sugar-maze-activity sugar-measure-activity sugar-memorize-activity sugar-moon-activity sugar-paint-activity sugar-physics-activity sugar-pippy-activity sugar-poll-activity sugar-portfolio-activity sugar-read-activity sugar-record-activity sugar-speak-activity sugar-stopwatch-activity sugar-story-activity sugar-terminal-activity sugar-turtleart-activity sugar-words-activity sugar-write-activity

EDIT: forgot to mention this is present in master, so is not related to the changes made by the enhancement patches.

[1] ​https://github.com/tchx84/sugar/commit/2b5c81e0d1954b70878b0fa5fc9d55ae6734e7ed
[2] ​http://fpaste.org/236346/84689143/
[3] ​http://fpaste.org/236348/85323143/

[quozl]

Thanks for working on this.

---

The scenario is a well known class of problem that has been solved in other software components by adding a notification to the post-install script of the package.

You can see examples by searching for interprocess communication methods in /var/lib/dpkg/info/*.postinst. Methods include calling /usr/bin/touch to create a file in /var/run which is then deleted by a daemon, or sending D-Bus messages to the system bus using /usr/bin/dbus-send, or sending a SIGHUP signal using /bin/kill or the shell built-in. The method must be harmless if Sugar is not running.

A Debian package postinst is guaranteed to run when all the files are in place. A similar mechanism is available for Red Hat derived distributions.

---

The delay you propose is a good start, but it would be better to set the delay to 100ms and continue to repeat the callback (return True) if the directory is not yet ready. See ​revised patch, untested. However, this will be noisy, because add_bundle presumes to log an exception every time, and an invalid bundle will cause a performance hit. An internal method to look for the activity.info file may be better. A give-up countdown may be better.

---

I don't think it is necessary to handle the removal of the /usr/share/sugar/activities/ directory, as even with an upgrade of every activity the packages are processed one by one. For the moment, remove one activity from the remove everything test.

[tch]

I got a revised version for the last patch to handle the delay between the directory creation and the time to actually access its contents (e.g. actvity.info file).

The idea is basically just to do a best-effort retry approach, this means it will retry with inscreasing delay times until it reaches a limit.

The patch [1] as it is, will basically retry 3 times, after 100ms, 1000ms and 10000ms, which seems reasoable in terms of workload. These times work fine for all our current packages.

To test I used:

1. sudo apt-get remove $(dpkg-query -W|grep sugar|grep activity|cut -f1)
2. sudo mkdir /usr/share/sugar/activities # very important, because the directory is removed in the previous step.
3. sudo apt-get install olpc-activities

[1] ​https://github.com/tchx84/sugar/commit/7743b90b380970751cf32eee544ee84f135c203a

[quozl]

@tch, now that we have a fix for activity list inconsistency bundled in feature ​Trigger Bundle Add, can we get the enhanced favourites branch merged too?

[quozl]

Most functional changes were committed and are in 0.108.

A small change for the hidden activity list is in
​https://github.com/sugarlabs/sugar/pull/705

After that, we can probably close this ticket.

[quozl]

Was merged, closing.