search

sugarlabs / sugar-toolkit-gtk3

Sugar Learning Environment, Activity Toolkit, GTK 3.
GNU Lesser General Public License v2.1
21 stars 80 forks source link

Remove Rainbow, avoid race in creating directories #306

Closed quozl closed 8 years ago

quozl commented 8 years ago

Sugar depended on Rainbow for clearing the activity instance/ and tmp/ directories. But Rainbow is no longer used downstream.

samdroid-apps commented 8 years ago

This patch is a NAK from my perspective.

As @godiard said on the mailing list, what if the user has 2 activity instances open? When you close the first instance, it will clear it's instance dir and leave it in a bad state. Please clear it at a different time to address that issue.

Otherwise it looks like a good patch.

quozl commented 8 years ago

Any reason why not say this on mailing list where the discussion is?

quozl commented 8 years ago

Ping? Substantial change to patch, please review.

samdroid-apps commented 8 years ago

From Gonzalo's reply to your initial mailing list post:

A few issues: ...

  • Temporary directories are shared between instances, if we remove temporary directories at activity start or stop, we need check if there are other instances of the activity running.

Please address this. Currently, your patch description says it will delete files from under the activity's eyes if a 2nd instance is launched.

Also, where is the code that actually removes the directories?

quozl commented 8 years ago

Currently, your patch description says it will delete files

No, it doesn't. Look at the commit carefully?

samdroid-apps commented 8 years ago

Ah, I see. I just edited the initial pull request comment to reflect the new commit message.

This looks fine to me. I'll just test 'n' merge.