- Screenshots:
-
File Attachments Thumbnails
Framework for handling file attachments by mimetype
Review Request #2967 — Created March 18, 2012 and submitted
This is meant as an overview of the design for a framework handling file attachments according to different mimetypes (e.g., generating a thumbnail, determining the icon url, etc.). The FileAttachment class will have some notion of a handler, which will be called at the appropriate points (a la strategy patter). For mimetype-specific parsing, I chose mimeparse, although I suspect the spec may be amenable to simple string/regular-expression matching.
I was thinking of creating a MimetypeHandler class that will have a factory classmethod that will allow it to iterate over all the subclasses and find the appropriate handler. A handler indicates this capability through a matches classmethod. Ideally, each subclass will operate on a subset of the mimetypes that the parent class operates on. I'm not sure if using subclasses is the best way to do this, as the class will be registered automatically once you install the extension. I've worked around it as follows, but I'm not sure if it is a good way to do it:
def matches(self, mimetype):
ext_manager = get_extension_manager()
for e in ext_manager.get_enabled_extensions():
if e.__class__ == TextMimetypeHandler:
return mimetype[0] == 'text'
return False
I've also included a working extension for text mimetypes.
This works on a LinuxMint 10 (linux 2.6.35-22) virtual machine with Review board 1.7a on Firefox 3.6.16.
Screenshots
| Description | From | Last Updated |
|---|---|---|
|
No blank line. Djblets is third-party in Review Board's perspective. |
 chipx86
|
|
|
First line, immediately after the """, must be a one-line summary. Then a blank line, then a description of the … |
chipx86
|
|
|
It'd be nice to document these functions. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Blank line. (I prefer blank lines between blocks (if statements, loops, etc.) and other code on the same level.) |
chipx86
|
|
|
This should be using the static() function. It's a templatetag, but you can import it and use it like a … |
chipx86
|
|
|
Same comment here about one-line summary. |
chipx86
|
|
|
Is this the right default? I'm not sure we want to default every MimetypeHandler to matching. Maybe keep this False … |
chipx86
|
|
|
Always have a period after sentences in docs. These will eventually end up on webpages. When you only have one … |
chipx86
|
|
|
What's the point of the function? Can this roll into get_thumbnail below? Or is it for subclasses? If it's for … |
chipx86
|
|
|
Yeah, should be fine. Make this a @property though. |
chipx86
|
|
|
It's nice to keep these roughly alphabetical. |
chipx86
|
|
|
I believe border, margin, and padding can be "0" instead of "0px". |
ME medanat | |
|
This is a classmethod, so 'self' is wrong. It should be 'cls'. |
chipx86
|
|
|
Same about cls vs. self. |
chipx86
|
|
|
The % should be aligned with the '<img |
chipx86
|
|
|
Parameters need to align. And, hmm, I think you could get the full path from the FileField. Not sure you … |
chipx86
|
|
|
with: is only in Python 2.6. Just use standard: f = open(...) ... f.close() |
chipx86
|
|
|
Are you sure height - 1 is correct? range is inclusive (so a height of 3 will produce [0, 1, … |
chipx86
|
|
|
The % preview should be aligned with the parameters. |
chipx86
|
|
|
Weird convention. Shouldn't use _, and should spell out Mimetype. Same with all other classes here. |
chipx86
|
AM
-
-
-
First line, immediately after the """, must be a one-line summary. Then a blank line, then a description of the class.
-
-
-
Interesting. Never knew about __subclasses__. This will, I assume, only return direct subclasses and not subclasses of subclasses, but I guess in this case we're likely to recurse? My one concern is how we deal with priorities. If two things claim to match, one should win, but which? Some might be more specific. For example, a handler specifically for text/* and one for text/html should prefer the second for HTML files. We can figure this out by having a way of scoring the match by assigning one point for each of the following: * Mimetype matches in some form (exactly, or through wildcards, or by stripping out the +foo, or by being a subset of the mimetype -- text/plain vs text/html) * Left-hand side of the "/" matches exactly. * Right-hand side matches (possibly by stripping out the stuff after "+", if it exists, or being a mimetype that is a subset of another) * Right-hand side matches exactly (including the "+foo") So.. something like that. The points are only returned if the mimetype is compatible. Through the scoring, we can pick one handler over another based on how confident that handler is that it knows better what the mimetype is. By consolidating this logic in MimetypeHandler, the subclasses could by default just have a list of mimetypes they support instead of overriding handles(). Of course, they could always override that, but it keeps the basic case simple.
-
-
Blank line. (I prefer blank lines between blocks (if statements, loops, etc.) and other code on the same level.)
-
This should be using the static() function. It's a templatetag, but you can import it and use it like a function. If you grep around the codebase, you'll see examples. Also, you can get rid of MEDIA_SERIAL now.
-
-
Is this the right default? I'm not sure we want to default every MimetypeHandler to matching. Maybe keep this False here, and have a GenericMimetypeHandler subclass that sets this to True.
-
Always have a period after sentences in docs. These will eventually end up on webpages. When you only have one line, the """ should be right up against both sides of the text.
-
What's the point of the function? Can this roll into get_thumbnail below? Or is it for subclasses? If it's for subclasses, it shouldn't have a _.
-
-
AM
- Change Summary:
-
I had to restrict the <br /> tag that was being targeted. Additionally, I'm not sure what the best way for the handler to discern between enabled and disabled extensions.
- Diff:
-
Revision 2 (+267 -21)
AM
- Change Summary:
-
I cleaned up some of the comments/variable names, and tweaked the logic slightly so that all supported mimetypes are considered instead of just the first better match.
- Diff:
-
Revision 3 (+263 -21)
-
This is looking great. The remaining things are very minor, but this will be going in soon :)
-
-
-
-
Parameters need to align. And, hmm, I think you could get the full path from the FileField. Not sure you need to os.path.join. There's a method injected into the model, I believe.
-
-
Are you sure height - 1 is correct? range is inclusive (so a height of 3 will produce [0, 1, 2], or in your case, [0, 1]).
-
-