Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
On Sun, Jun 8, 2008 at 3:51 AM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: Sorry, Marco, but here is yet another version of the patch. I decided to refactor the Config class out of bundlebuilder - all it was carrying in the common start() case was bundle_name; and as for other cases, it is simpler to call Builder(...params...) than Builder(Config(...params...)). I'm using a config object because it's easier to carry around in the other Xer. I don't think having to create the Config is a big deal, calling it directly is not the primary use case anyway. But i you *really* feel that we need this bit of convenience, we can add a create_builder() which setups the config for you. Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
On Sun, Jun 8, 2008 at 3:51 AM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: Sorry, Marco, but here is yet another version of the patch. I decided to refactor the Config class out of bundlebuilder - all it was carrying in the common start() case was bundle_name; and as for other cases, it is simpler to call Builder(...params...) than Builder(Config(...params...)). I'm not convinced that's a good idea. But anyway, let's not increase the scope of the patch, as I told you I already had problems to review it in his current form. I'll review the old versions of the patch and when that's done we can consider this change. Thanks, Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
-import subprocess
+from subprocess import Popen, PIPE
import re
import gettext
from optparse import OptionParser
+import warnings
+import subprocess
warnings seem unused, doesn't pylint warn you about it?
Since you are importing subprocess just use that for Popen and PIPE.
+from sugar.bundle.activitybundle import ActivityBundle, MANIFEST, list_files
It doesn't make sense for activitybundle to expose something a generic
as list_files, especially since you are just using it to walk files
inside that module.
I think the MANIFEST definition is overkill and you are using it
inconsistently anyway. Just use the string.
+def __init__(self, bundle_name, source_dir=None, dist_dir = None,
+ dist_name = None):
As I explained in my previous mail, I want to keep passing in the
config here. Please remove the extra params.
+def fix_manifest(self):
+allfiles = list_files(self.config.source_dir,
+ ignore_dirs=['dist', '.git'],
+ ignore_files=['.gitignore', 'MANIFEST',
+'*.pyc', '*~', '*.bak'])
+for afile in allfiles:
+if afile not in self.config.bundle.manifest:
+self.config.bundle.manifest.append(afile)
+manifestfile = open(os.path.join(self.config.source_dir,
+ MANIFEST),
+wb)
+for line in self.config.bundle.manifest:
+manifestfile.write(line + \n)
This is all really hard to read. Some suggestions:
* Split the ignore lists out of the list_files call.
* We are never using aX for list iteration in the code. Just use f or
filename there.
* Do a manifest = self.config.bundle.manifest, you are repeating it 3 times
* s/manifestfile/manifest so that it feet in one line and you don't
need the crazy split up.
* Add a \n after manifestfile = open...
def get_files(self):
-return list_files(self.config.source_dir,
- ignore_dirs=['locale', 'dist', '.git'],
- ignore_files=['.gitignore'])
+git_ls = Popen('git-ls-files', stdout=PIPE, cwd=self.config.source_dir)
+if git_ls.wait():
+#non-0 return code - failed
+return BuildPackager.get_files(self)
+f = git_ls.stdout
+files = []
+for line in f.readlines():
+filename = line.strip()
+if not filename.startswith('.'):
+files.append(filename)
+f.close()
+return files
Please separate groups of code with \n to make it more readable. I
don't think this comment is necessary #non-0 return code - failed,
it's obvious if you ever used Popen.
+from sugar import activity
+from sugar import env
Unless you have a concrete need for this please drop it from the
patch. To fix it properly I think we need to really cleanup the
dependencies. But that's another patch.
+def _is_dir(self, filename):
+def _is_file(self, filename):
As you did in your last patch remove the _
+def install(self):
I don't think installation should be expose by ActivityBundle, we need
to cleanup the dependencies. Let's remove it from this patch and we
can discuss how to do it properly
+try:
+f = self._get_file(MANIFEST)
+except IOError:
+f = None
+if not f:
+logging.warning(Activity directory lacks a MANIFEST file.)
+return []
All the other code which uses _get_file, assumes it will just return
None if there is an IOError. Seems easier to just change the method to
really do so (if it doesn't already).
+#strip trailing \n and other whitespace
I think this comment is unnecessary, it's implicit in the definition
of the strip function.
I have some more comments for the read_manifest implementation (as a
start some \n will help readability), but I have to run out now. We
can address those in the next iteration.
Thanks!
Marco
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
[sugar] [PATCH] Browse - copy Image to clipboard under rainbow
Hi,
tomeu can you verify that patch?
Use case: Copying an image using the palette under rainbow.
We loose the data which exist in the filename, should we try to store this
somehow?
Best,
Simon
diff --git a/palettes.py b/palettes.py
index d62fb5e..254201f 100644
--- a/palettes.py
+++ b/palettes.py
@@ -15,8 +15,8 @@
# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
import os
-import tempfile
import urlparse
+import time
from gettext import gettext as _
import gtk
@@ -27,6 +27,7 @@ from sugar.graphics.palette import Palette, Invoker
from sugar.graphics.menuitem import MenuItem
from sugar.graphics.icon import Icon
from sugar import profile
+from sugar.activity import activity
class ContentInvoker(Invoker):
_com_interfaces_ = interfaces.nsIDOMEventListener
@@ -157,8 +158,10 @@ class ImagePalette(Palette):
extension = None
if '.' in file_name:
extension = file_name.split('.')[1]
-fd, self._temp_file = tempfile.mkstemp(suffix='.' + extension)
-os.close(fd)
+
+temp_path = os.path.join(activity.get_activity_root(), 'instance')
+self._temp_file = os.path.join(temp_path, '%i%s' %
+ (time.time(), extension))
cls = components.classes['@mozilla.org/network/io-service;1']
io_service = cls.getService(interfaces.nsIIOService)
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Some quick responses, before I dig into this...
On Sun, Jun 8, 2008 at 4:08 AM, Marco Pesenti Gritti [EMAIL PROTECTED]
wrote:
-import subprocess
+from subprocess import Popen, PIPE
import re
import gettext
from optparse import OptionParser
+import warnings
+import subprocess
warnings seem unused, doesn't pylint warn you about it?
Brain fart. I read pylint output, said OK, there are warnings about unused
imports, but which unused imports? :) Actually, this is fixed in later
patches I sent. Will fix.
Since you are importing subprocess just use that for Popen and PIPE.
+from sugar.bundle.activitybundle import ActivityBundle, MANIFEST,
list_files
It doesn't make sense for activitybundle to expose something a generic
as list_files, especially since you are just using it to walk files
inside that module.
So I should duplicate the code of list_files? (This is my biggest question
with your response)
I think the MANIFEST definition is overkill and you are using it
inconsistently anyway. Just use the string.
As the number of metadata files in the activity format grows, it seems
likely that eventually we'll give them their own directory. Making this a
global now is the first step to being able to change it later. But if you
prefer, I'll use a string for now.
+def __init__(self, bundle_name, source_dir=None, dist_dir = None,
+ dist_name = None):
As I explained in my previous mail, I want to keep passing in the
config here. Please remove the extra params.
disagree a bit, but will do.
+def fix_manifest(self):
+allfiles = list_files(self.config.source_dir,
+ ignore_dirs=['dist', '.git'],
+ ignore_files=['.gitignore', 'MANIFEST',
+'*.pyc', '*~', '*.bak'])
+for afile in allfiles:
+if afile not in self.config.bundle.manifest:
+self.config.bundle.manifest.append(afile)
+manifestfile = open(os.path.join(self.config.source_dir,
+ MANIFEST),
+wb)
+for line in self.config.bundle.manifest:
+manifestfile.write(line + \n)
This is all really hard to read. Some suggestions:
* Split the ignore lists out of the list_files call.
* We are never using aX for list iteration in the code. Just use f or
filename there.
* Do a manifest = self.config.bundle.manifest, you are repeating it 3 times
* s/manifestfile/manifest so that it feet in one line and you don't
need the crazy split up.
* Add a \n after manifestfile = open...
will do
def get_files(self):
-return list_files(self.config.source_dir,
- ignore_dirs=['locale', 'dist', '.git'],
- ignore_files=['.gitignore'])
+git_ls = Popen('git-ls-files', stdout=PIPE,
cwd=self.config.source_dir)
+if git_ls.wait():
+#non-0 return code - failed
+return BuildPackager.get_files(self)
+f = git_ls.stdout
+files = []
+for line in f.readlines():
+filename = line.strip()
+if not filename.startswith('.'):
+files.append(filename)
+f.close()
+return files
Please separate groups of code with \n to make it more readable. I
don't think this comment is necessary #non-0 return code - failed,
it's obvious if you ever used Popen.
will do
+from sugar import activity
+from sugar import env
Unless you have a concrete need for this please drop it from the
patch. To fix it properly I think we need to really cleanup the
dependencies. But that's another patch.
will do
+def _is_dir(self, filename):
+def _is_file(self, filename):
As you did in your last patch remove the _
will do
+def install(self):
I don't think installation should be expose by ActivityBundle, we need
to cleanup the dependencies. Let's remove it from this patch and we
can discuss how to do it properly
install was already exposed, I just refactored unpack out of it (and thus
also exposed unpack). This only showed up in the patch because I put unpack
first, and the body of unpack matched with that part of the body of install.
I'm pretty sure this function is used by Journal, we can't just drop it.
+try:
+f = self._get_file(MANIFEST)
+except IOError:
+f = None
+if not f:
+logging.warning(Activity directory lacks a MANIFEST file.)
+return []
All the other code which uses _get_file, assumes it will just return
None if there is an IOError. Seems easier to just change the method to
really do so (if it doesn't already).
Will do.
+#strip trailing \n and other whitespace
I think this comment is unnecessary, it's implicit in the definition
of the strip function.
Will do.
I
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
On Sun, Jun 8, 2008 at 1:59 PM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: It doesn't make sense for activitybundle to expose something a generic as list_files, especially since you are just using it to walk files inside that module. So I should duplicate the code of list_files? (This is my biggest question with your response) Unless I'm missing something you won't really duplicate much of it, since you are not using the ignore_* which are the big part of the code and the actual purpose of that function. You will basically just need the for... I think the MANIFEST definition is overkill and you are using it inconsistently anyway. Just use the string. As the number of metadata files in the activity format grows, it seems likely that eventually we'll give them their own directory. Making this a global now is the first step to being able to change it later. But if you prefer, I'll use a string for now. If you move it to a directory you will better construct to the path with os.path.join, so it would probably be a method of Bundle. Anyway I don't think it's worth to worry about that yet, it's easy enough to refactor if/when necessary. +def __init__(self, bundle_name, source_dir=None, dist_dir = None, + dist_name = None): As I explained in my previous mail, I want to keep passing in the config here. Please remove the extra params. disagree a bit, but will do. Just look at all the ugly None checks you need to do to support it... :) +def install(self): I don't think installation should be expose by ActivityBundle, we need to cleanup the dependencies. Let's remove it from this patch and we can discuss how to do it properly install was already exposed, I just refactored unpack out of it (and thus also exposed unpack). This only showed up in the patch because I put unpack first, and the body of unpack matched with that part of the body of install. I'm pretty sure this function is used by Journal, we can't just drop it. You are right, That's ok then. Thanks, Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Here is the revised patch. It has your suggested changes, plus a couple more: - Check for existence of po directory in Builder - Config.__init__() is cleaned up. Now gets bundle name from activity.info. start() no longer needs a bundle name, and has deprecation warning. Also I put things in a more logical order. On Sun, Jun 8, 2008 at 7:11 AM, Marco Pesenti Gritti [EMAIL PROTECTED] wrote: On Sun, Jun 8, 2008 at 1:59 PM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: It doesn't make sense for activitybundle to expose something a generic as list_files, especially since you are just using it to walk files inside that module. So I should duplicate the code of list_files? (This is my biggest question with your response) Unless I'm missing something you won't really duplicate much of it, since you are not using the ignore_* which are the big part of the code and the actual purpose of that function. You will basically just need the for... I think the MANIFEST definition is overkill and you are using it inconsistently anyway. Just use the string. As the number of metadata files in the activity format grows, it seems likely that eventually we'll give them their own directory. Making this a global now is the first step to being able to change it later. But if you prefer, I'll use a string for now. If you move it to a directory you will better construct to the path with os.path.join, so it would probably be a method of Bundle. Anyway I don't think it's worth to worry about that yet, it's easy enough to refactor if/when necessary. +def __init__(self, bundle_name, source_dir=None, dist_dir = None, + dist_name = None): As I explained in my previous mail, I want to keep passing in the config here. Please remove the extra params. disagree a bit, but will do. Just look at all the ugly None checks you need to do to support it... :) +def install(self): I don't think installation should be expose by ActivityBundle, we need to cleanup the dependencies. Let's remove it from this patch and we can discuss how to do it properly install was already exposed, I just refactored unpack out of it (and thus also exposed unpack). This only showed up in the patch because I put unpack first, and the body of unpack matched with that part of the body of install. I'm pretty sure this function is used by Journal, we can't just drop it. You are right, That's ok then. Thanks, Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar diff --git a/src/sugar/activity/bundlebuilder.py b/src/sugar/activity/bundlebuilder.py index 1063f72..c16845d 100644 --- a/src/sugar/activity/bundlebuilder.py +++ b/src/sugar/activity/bundlebuilder.py @@ -19,53 +19,52 @@ import os import zipfile import tarfile import shutil -import subprocess import re import gettext from optparse import OptionParser +import subprocess +import logging from sugar import env from sugar.bundle.activitybundle import ActivityBundle +IGNORE_DIRS=['dist', '.git'], +IGNORE_FILES=['.gitignore', 'MANIFEST', '*.pyc', '*~', '*.bak'] + def list_files(base_dir, ignore_dirs=None, ignore_files=None): result = [] - + for root, dirs, files in os.walk(base_dir): for f in files: -if ignore_files and f not in ignore_files: +if not (ignore_files and +[True for pat in ignore_files if fnmatch(f,pat)]): +#not (result matches a pattern in ignore_files, ignore it) rel_path = root[len(base_dir) + 1:] result.append(os.path.join(rel_path, f)) if ignore_dirs and root == base_dir: for ignore in ignore_dirs: if ignore in dirs: dirs.remove(ignore) - return result class Config(object): -def __init__(self, bundle_name, source_dir=None): -if source_dir: -self.source_dir = source_dir -else: -self.source_dir = os.getcwd() +def __init__(self, source_dir=None, dist_path = ): +self.source_dir = source_dir or os.getcwd() - -bundle = ActivityBundle(self.source_dir) -version = bundle.get_activity_version() - -self.bundle_name = bundle_name -self.xo_name = '%s-%d.xo' % (self.bundle_name, version) -self.tarball_name = '%s-%d.tar.bz2' % (self.bundle_name, version) +self.bundle = bundle = ActivityBundle(self.source_dir) +self.version = bundle.get_activity_version() +self.activity_name = bundle.get_name() self.bundle_id = bundle.get_bundle_id() +self.bundle_name = reduce(lambda x, y:x+y, self.activity_name.split()) + +dist_dir, dist_name = os.path.split(dist_path) +self.dist_dir = dist_dir or os.path.join(self.source_dir, 'dist') +self.xo_name = dist_name or '%s-%d.xo' %
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
On Sun, Jun 8, 2008 at 9:26 PM, Jameson Chema Quinn [EMAIL PROTECTED] wrote: Here is the revised patch. It has your suggested changes, plus a couple more: - Check for existence of po directory in Builder - Config.__init__() is cleaned up. Now gets bundle name from activity.info. start() no longer needs a bundle name, and has deprecation warning. Also I put things in a more logical order. Jameson, *please* do *not* make any additional changes when submitting a new patch. Limit yourself to the changes requested by the reviewer. Additional fixes/improvements should go in a separate patch. I'll review it as is this time, but next time I'm going to ask you to take out the additional changes. Thanks, Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] Features for Sugar 0.82
Sounds great Marco. I'd really like to get a build that Charlie and Joe can start testing asap. Will this come from the OLPC-3 branch? We will work with Michael on exactly where to get the build and release notes about what should be working. Thanks! Kim On Sat, Jun 7, 2008 at 8:39 AM, Marco Pesenti Gritti [EMAIL PROTECTED] wrote: Hello, as you can see from the roadmap, 0.81.3 is going to be release on the 20 June and it's the feature, ABI, string freeze for 0.82. http://wiki.sugarlabs.org/go/ReleaseTeam/Roadmap In the last few days we landed: * Control panel (erikos) * Session management (marco) Here is what is left on the list (based on the feedback to my last mail about feature freeze). Note that the owners of several of these are tentative. * Free form layout of the homepage and ability to move icons around (tomeu) My understanding is that Tomeu is waiting for Eben to finalize the mockup * New activity startup notification (marco) I just sent a patch for review. It will need some bugfixing likely but should be good enough for the feature freeze. * Web activity: autocompletion of bookmarks and history (marco) I'm pretty much done with my features (earlier than I expected), so I can take this one. I'm going to implement what Eben suggested unless someone speak up to request something different soon: Well, we can introduce this in a relatively straightforward manner that has almost no additional UI up front. Anytime a bookmark is created in a session (by anyone in it, not just me), we would need to log this into a database stored in the shared space for all Browse instances. We should also log all pages visited (the history) here as well. The only additional UI in the short term would be a special palette of sorts attached to the URL entry which provides autocompletion suggestions in all Browse instances. * Web activity: find text in page (erikos) Simon, can you take this please? * Web activity: custom certificates support (tomeu) Tomeu, I think you did some work/investigation on this. If you don't feel home page work will give you enough time to tackle this one please let me know. * Object chooser [http://wiki.laptop.org/go/Designs/Object_Chooser improvements] (tomeu) Tomeu sent a patch for this, should land soon. * Chat with non-sugar Jabber clients (morgs) Morgs can you give an update on the status of this one? Are we on the right track to get it in time? * Presence scalability when using Jabber by using gadget (gdesmott, daf) gdesmott, daf, can you update on the status? Any estimate for when the backend for this will be ready? I should have time to work on the UI the week of the 16. All in all I'm really happy of how things are proceeding, we are on the right track! Thanks! Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Bundlebuilder use manifest, fix_manifest function
Also, you left this in. +print path Marco ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
