Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4276 closed defect (fixed)

Journal: .xo file in Documents or pendrive show a wrong icon

Reported by: godiard Owned by: erikos
Priority: High Milestone:
Component: Sugar Version: Unspecified
Severity: Unspecified Keywords: regression, r+, olpc-test-passed
Cc: ajay_garg, humitos, manuq Distribution/OS: Fedora
Bug Status: Unconfirmed

Description

TestCase

Copy a .xo file in the Documents directory

Go to the Journal and show the directory

The icon show is not the real icon, but one from other activity.

Tested in os15, xo-4

Attachments (3)

test-wrong-icon.png (36.5 KB) - added by manuq 8 years ago.
shell-wrong-icon.log (46.7 KB) - added by manuq 8 years ago.
Lots of guessing but no one provided a log :)
0001-ActivityBundle-don-t-wrap-the-temporal-icon-path-in-.patch (1.7 KB) - added by manuq 8 years ago.
Candidate patch.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by erikos

  • Priority changed from Unspecified by Maintainer to High

I copied edit-8.xo from the Journal into the Documents folder using the copy to option in the Palette. I had been downloading the activity before. There was already Write-83.1.xo in the Documents folder.

Now the Documents folder view shows:

  • an entry "edit-8" with the edit icon (in the Journal the entry is named "edit-8.xo")
  • another entry "Write-83.1.xo" with the edit icon

If I go to the details view of "Write-83.1.xo" the icon is the one from the Write activity. If I go to the details view of "edit-8" the edit activity icon is shown.

Copying other entries from the Journal to the Documents folder do show the correct icon.

comment:2 Changed 8 years ago by erikos

Furthermore:

  • the right click Palette of the "Write-83.1.xo" does show the correct icon.
  • copying another activity I downloaded, the view stays the same, only the "Write-83.1.xo" entry is wrong.

comment:3 follow-up: Changed 8 years ago by erikos

I downgraded the DS for #4276: http://arm.koji.fedoraproject.org/koji/buildinfo?buildID=96531

I think the issue is gone with this.

comment:4 in reply to: ↑ 3 Changed 8 years ago by manuq

Replying to erikos:

I downgraded the DS for #4276: http://arm.koji.fedoraproject.org/koji/buildinfo?buildID=96531

I think the issue is gone with this.

I downgraded the DS too, for an .xo file in a stick I can still see the document-like icon instead of the activity icon (which does display in the details view and palette menu).

Changed 8 years ago by manuq

comment:5 Changed 8 years ago by ajay_garg

  • Cc ajay_garg added
  • Component changed from sugar to sugar-toolkit-gtk3
  • Distribution/OS changed from Unspecified to Fedora

comment:6 Changed 8 years ago by ajay_garg

An accompanying sugar-patch has been floated at http://lists.sugarlabs.org/archive/sugar-devel/2012-December/041181.html

This patch removes the icon-files directory on every sugar-reboot, to prevent accumulation of any spurious files.

Thanks to James Cameron <quozl@…>, for the patch-idea.

Changed 8 years ago by manuq

Lots of guessing but no one provided a log :)

comment:8 Changed 8 years ago by manuq

  • log line 285 and 286, we have a warning from pygi - unable to set property file-name' of type gchararray' from value of type `PyObject'

Changed 8 years ago by manuq

Candidate patch.

comment:9 follow-up: Changed 8 years ago by manuq

  • Keywords r? added

So the pygi warning in the log here is because the journal model for 'file-name', which should be a 'str' with the path of the icon, is getting a sugar3.util.TempFilePath instead. That class actually inherits 'str', but seems that pygobject doesn't like it.

For a quick workaround I casted the result of get_icon as 'str':

--- a/src/jarabe/journal/misc.py
+++ b/src/jarabe/journal/misc.py
@@ -74,7 +74,7 @@ def get_icon_name(metadata):
         if file_path is not None and os.path.exists(file_path):
             try:
                 bundle = ActivityBundle(file_path)
-                file_name = bundle.get_icon()
+                file_name = str(bundle.get_icon())
             except Exception:
                 logging.exception('Could not read bundle')

And then I come to the next issue: as Ajay said, the temporal file is deleted before the CellRendererIcon reads it.

1355603345.296226 DEBUG root: TempFilePath created '/tmp/activity-webu590GL.svg'
1355603345.300510 DEBUG root: TempFilePath deleted '/tmp/activity-webu590GL.svg'
1355603345.301241 DEBUG root: MANUQ get_icon_name got '/tmp/activity-webu590GL.svg' of type <type 'str'>
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/sugar3/graphics/icon.py", line 952, in do_render
    surface = self._buffer.get_surface()
  File "/usr/lib/python2.7/site-packages/sugar3/graphics/icon.py", line 261, in get_surface
    handle = self._load_svg(icon_info.file_name)
  File "/usr/lib/python2.7/site-packages/sugar3/graphics/icon.py", line 121, in _load_svg
    return self._loader.load(file_name, entities, self.cache)
  File "/usr/lib/python2.7/site-packages/sugar3/graphics/icon.py", line 51, in load
    icon_file = open(file_name, 'r')
IOError: [Errno 2] No existe el fichero o el directorio: '/tmp/activity-webu590GL.svg'

What happens is as we don't maintain any reference to the TempFilePath, its del method deletes the temporal file.

As a comment in the get_icon method of ActivityBundle say, ideally we should return the icon data. But that implies a major change in the Journal model and views, and in the CellRendererIcon class.

So I propose returning the temporal file path directly, instead of wrapping it in a TempFilePath. This has one disadventage, the temporal files are left in /tmp and that directory can get crowded. But that is not harmful

The attached patch does so.

comment:10 in reply to: ↑ 9 Changed 8 years ago by manuq

Replying to manuq:

the temporal files are left in /tmp and that directory can get crowded. But that is not harmful

Because /tmp is mounted as a tmpfs, so it is cleaned by the system after each reboot or when needed.

comment:11 Changed 8 years ago by humitos

  • Cc humitos manuq added
  • Keywords r+ added; r? removed

The patch looks good as a quick fix. But as you said, we have to improve this in the future. I tested it on XO-4 13.1.0 build 19 and it worked.

Check #1175 for some history on a similar issue.

comment:12 Changed 8 years ago by manuq

  • Keywords olpc-test-pending added
  • Resolution set to fixed
  • Status changed from new to closed

Pushed b6152b02 .

comment:13 Changed 8 years ago by greenfeld

  • Keywords olpc-test-passed added; olpc-test-pending removed

I see the correct icon for several xo files on a USB stick in 13.1.0 os24/Sugar 0.98.3.

comment:14 Changed 8 years ago by dnarvaez

  • Component changed from sugar-toolkit-gtk3 to Sugar

comment:15 Changed 8 years ago by dnarvaez

  • Milestone 0.98 deleted

Milestone 0.98 deleted

Note: See TracTickets for help on using tickets.