Framework for handling file attachments by mimetype

Review Request #2967 — Created March 18, 2012 and submitted

Review Board
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.
Loading file attachments...

  • 0
  • 0
  • 21
  • 1
  • 22
Description From Last Updated
  1. Looks like a fantastic start. Some style stuff to fix, and a couple ideas regarding mimetype matching.
    What about the renderer for the .txt file you show in the screenshot? Seems like a good one to include too.
  2. reviewboard/attachments/ (Diff revision 1)
    No blank line. Djblets is third-party in Review Board's perspective.
  3. reviewboard/attachments/ (Diff revision 1)
    First line, immediately after the """, must be a one-line summary. Then a blank line, then a description of the class.
  4. reviewboard/attachments/ (Diff revision 1)
    It'd be nice to document these functions.
  5. reviewboard/attachments/ (Diff revision 1)
    Blank line between these.
  6. reviewboard/attachments/ (Diff revision 1)
    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.
  7. reviewboard/attachments/ (Diff revision 1)
    Blank line between these.
  8. reviewboard/attachments/ (Diff revision 1)
    Blank line.
    (I prefer blank lines between blocks (if statements, loops, etc.) and other code on the same level.)
  9. reviewboard/attachments/ (Diff revision 1)
    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.
  10. reviewboard/attachments/ (Diff revision 1)
    Same comment here about one-line summary.
  11. reviewboard/attachments/ (Diff revision 1)
    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.
  12. reviewboard/attachments/ (Diff revision 1)
    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.
  13. reviewboard/attachments/ (Diff revision 1)
    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 _.
  14. reviewboard/attachments/ (Diff revision 1)
    Yeah, should be fine. Make this a @property though.
  15. reviewboard/static/rb/css/reviews.less (Diff revision 1)
    It's nice to keep these roughly alphabetical.
  2. reviewboard/static/rb/css/reviews.less (Diff revision 2)
    I believe border, margin, and padding can be "0" instead of "0px".
  1. This is looking great. The remaining things are very minor, but this will be going in soon :)
  2. reviewboard/attachments/ (Diff revision 3)
    This is a classmethod, so 'self' is wrong. It should be 'cls'.
  3. reviewboard/attachments/ (Diff revision 3)
    Same about cls vs. self.
  4. reviewboard/attachments/ (Diff revision 3)
    The % should be aligned with the '<img
  5. reviewboard/attachments/ (Diff revision 3)
    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.
  6. reviewboard/attachments/ (Diff revision 3)
    with: is only in Python 2.6. Just use standard:
    f = open(...)
  7. reviewboard/attachments/ (Diff revision 3)
    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]).
    1. I subtract one from the height because I set preview to be escape(f.readline()) before the loop.
  8. reviewboard/attachments/ (Diff revision 3)
    The % preview should be aligned with the parameters.
  9. reviewboard/attachments/ (Diff revision 3)
    Weird convention. Shouldn't use _, and should spell out Mimetype.
    Same with all other classes here.
  1. I know this was forever ago now, but we're finally back in feature mode for the release. This is awesome, and I'm just about to commit it. Thanks for all your work on this!
Review request changed

Status: Closed (submitted)