Markdown Review UI

Review Request #3434 — Created Oct. 21, 2012 and discarded

Information

Review Board
master

Reviewers

Adds functionality to render Markdown file attachments in Review Board.

The rendering is handled by the "markdown" Python package which, given a file, returns the HTML code for it. This HTML code is then passed to the Backbone view for the Markdown Pluggable UI through the template and rendered in the browser.

Note: This code depends on this review request: http://reviews.reviewboard.org/r/3613/
"Valid" Case:
1) Created a review request and uploaded multiple random files to it
2) Then, uploaded a valid Markdown file (with the extension .md) to that review request
3) Clicked the "Review" link for the Markdown file
4) The new page loaded should render the Markdown file

"Invalid" Case:
1) Created a review request and uploaded multiple random files to it
2) Then, uploaded an image to that review request by changed the file extension to be ".md". For example, wallpaper.png -> wallpaper.md
3) Clicked the "Review" link for the invalid Markdown file
4) The new page loaded did not render anything (i.e. it displayed the title and caption but nothing was displayed in the section where the HTML would have rendered)

Description From Last Updated

My latest change (up for review) for image review UI has registration support. You should be able to look into …

chipx86chipx86

Place these before the screenshot ones.

chipx86chipx86

Same.

chipx86chipx86

The new branch I mentioned has a FileAttachmentReviewable, which you'll want to look at. It should provide everything this currently …

chipx86chipx86

Remove the trailing comma.

chipx86chipx86

No need for this.

chipx86chipx86

Make sure not to have trailing commas, or things will break in some browsers.

chipx86chipx86

Trailing commas bad.

chipx86chipx86

Sync master and you'll be able to take advantage of registration. Look at reviewboard/reviews/ui/__init__.py.

chipx86chipx86

I might be very wrong here, but I think you need to be inheriting from FileAttachmentReviewUI, otherwise your MarkdownReviewUI won't …

KA Karl-L

This is a bit expensive. You should use the file attachment provided. You'll need to subclass FileAttachmentReviewUI for that.

chipx86chipx86

I might (again) be very wrong here, but your MarkdownReviewUI instance should have been given the FileAttachment object to render …

KA Karl-L

I don't think you need to do this. It's more expensive to process this than to just let the browser …

chipx86chipx86

Unless you have additional work to do here, don't bother overriding it.

chipx86chipx86

You shouldn't define a new type of render function. What you really should be doing is: this.$el.html(this.model.get('rendered')); in renderContent.

chipx86chipx86

No need to return anything.

chipx86chipx86

You want {{markdown.caption|escapejs}}

chipx86chipx86

Same.

chipx86chipx86

This should append view.$el.

chipx86chipx86

Should be included alphabetically, and have a trailing comma.

chipx86chipx86

I wonder if it'd make more sense to just call it reviewboard.reviews.ui.markdown, as opposed to reviewboard.reviews.ui.markdownui. The ui in markdownui …

mike_conleymike_conley

It would be nice to simply prioritize the mimetype when looking for a best handler. So, instead of checking the …

KA Karl-L

This would be more visible at the top of the file.

KA Karl-L

Maybe I'm missing something here, but why are you changing these? Seems only tangentially related to the rest of the …

mike_conleymike_conley

We tend to keep the trailing commas in Python.

mike_conleymike_conley

Again, I have to ask - why are these being added? I don't see these files being added in this …

mike_conleymike_conley

Should inherit from FileAttachmentReviewable, and then you won't need 'caption' or 'attachmentID'.

chipx86chipx86

Nit: alphabetical

mike_conleymike_conley

Nit: remove this newline.

mike_conleymike_conley

I think this is indented too much.

mike_conleymike_conley

StringIO is part of the standard library, so your imports should read like this: import StringIO import markdown from reviewboard.reviews.ui.base …

daviddavid

Should be: try: from cStringIO import StringIO except ImportError: from StringIO import StringIO That'll use cStringIO when available, which is …

chipx86chipx86
chipx86
  1. I haven't read through the code yet, but you should keep to the same naming conventions for your JavaScript files. markdown*.js, not markdown/*.js.
    1. Er, could have sworn the email showed markdown/reviewableModel.js. Nevermind.
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     
     
     
    Show all issues
    My latest change (up for review) for image review UI has registration support. You should be able to look into that and use it. I recommend applying it to a branch for now, then rebasing your code on top of it, until we get it in (soon).
  3. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
    Place these before the screenshot ones.
  4. reviewboard/settings.py (Diff revision 1)
     
     
    Show all issues
    Same.
  5. Show all issues
    The new branch I mentioned has a FileAttachmentReviewable, which you'll want to look at. It should provide everything this currently does.
  6. Show all issues
    Remove the trailing comma.
  7. Show all issues
    No need for this.
  8. Show all issues
    Make sure not to have trailing commas, or things will break in some browsers.
  9. Show all issues
    Trailing commas bad.
  10. 
      
AA
AA
AA
AA
AA
  1. 
      
  2. reviewboard/reviews/ui/markdownui.py (Diff revision 4)
     
     
     
    Instead of replacing new line characters over here, I can also do {{ review_ui.render|linebreaksbr }} (https://docs.djangoproject.com/en/dev/ref/templates/builtins/?from=olddocs#linebreaksbr)
    
    Is one method better than/preferred over another?
  3. 
      
AA
  1. 
      
  2. reviewboard/reviews/ui/markdownui.py (Diff revision 4)
     
     
    Is there a more elegant / more-Pythonic way to get the FileAttachment object of the Markdown file? 
    
    I don't think this will work if there is a review request with multiple attachments or when the first attachment is not a markdown file
  3. 
      
AA
KA
  1. Pretty cool to see markdown being rendered in RB! I couldn't find where to leave comments on the screenshot, but maybe adding a light border and some extra margin (well...padding) around the rendering would help make it clear that we're looking at the file and not some other content, sort of like a page-within-a-page type deal? Just a thought.
    1. To add a comment, just click-and-drag.
  2. reviewboard/reviews/ui/base.py (Diff revision 4)
     
     
    I know this is temporary, but just curious what your plans are for handling markdown files. I don't think (?) the mimetype is to be trusted (it might just be text/plain), so will you be augmenting this method to look at file extensions as well?
  3. reviewboard/reviews/ui/markdownui.py (Diff revision 4)
     
     
    Show all issues
    I might be very wrong here, but I think you need to be inheriting from FileAttachmentReviewUI, otherwise your MarkdownReviewUI won't get picked as the best handler. You'll still need to fix FileAttachmentReviewUI.get_best_handler to support markdown, of course.
    
    See the FileAttachment.review_ui property in attachments/models.py
  4. reviewboard/reviews/ui/markdownui.py (Diff revision 4)
     
     
    Show all issues
    I might (again) be very wrong here, but your MarkdownReviewUI instance should have been given the FileAttachment object to render when it got instantiated (as `self.obj`). So, instead of getting the file attachments for the review request and picking one from the list, the file to be rendered should already be available as `self.obj.file`. 
    
    See FileAttachmentReviewUI in ui/base.py.
  5. reviewboard/reviews/ui/markdownui.py (Diff revision 4)
     
     
     
    I think the template filter is the better option -- might as well keep that part of the presentation separate.
    . It cleans up the python code a little and leaves the door open to getting the regular markdown output if we want.
    
    Out of curiosity, why remove the newlines? Does it affect the rendered HTML?
    1. New lines in the rendered HTML were causing things to break when included in the template. To work around that, I replaced "\r\n", "\r", and "\n" as a quick fix to get the HTML to render in the browser.
      
      I've removed that code in my most recent patch and instead used the "escapejs" filter in the Django template (as suggested by Christian) instead. It pretty much does the same thing without having me reinventing the wheel: https://docs.djangoproject.com/en/dev/ref/templates/builtins/?from=olddocs#escapejs
  6. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 4)
     
     
     
     
    Show all issues
    Sync master and you'll be able to take advantage of registration. Look at reviewboard/reviews/ui/__init__.py.
  3. reviewboard/reviews/ui/markdownui.py (Diff revision 4)
     
     
    Show all issues
    This is a bit expensive.
    
    You should use the file attachment provided. You'll need to subclass FileAttachmentReviewUI for that.
  4. reviewboard/reviews/ui/markdownui.py (Diff revision 4)
     
     
     
    Show all issues
    I don't think you need to do this. It's more expensive to process this than to just let the browser handle it.
    
    Also, if we were going to, a regex would be faster. One loop through and not 3.
  5. Show all issues
    Unless you have additional work to do here, don't bother overriding it.
  6. Show all issues
    You shouldn't define a new type of render function. What you really should be doing is:
    
        this.$el.html(this.model.get('rendered'));
    
    in renderContent.
  7. Show all issues
    No need to return anything.
  8. Show all issues
    You want {{markdown.caption|escapejs}}
  9. Show all issues
    Same.
  10. Show all issues
    This should append view.$el.
  11. setup.py (Diff revision 4)
     
     
    Show all issues
    Should be included alphabetically, and have a trailing comma.
  12. 
      
AA
AA
  1. 
      
  2. reviewboard/reviews/ui/markdownui.py (Diff revision 5)
     
     
     
    There is no standardized mimetype for Markdown yet. I tested with a few Markdown files and were plain text, some were "text/x-markdown" and some were "application/octet-stream".
    
    I think we should do as follows:
    - support_mimetypes should be ['text/x-markdown', 'text/x-web-markdown'] (until something is standardized)
    - Additionally, there should be a property to match against the file extension. For example: extension_pattern = "*.md"
    
    Instead of relying on mimetypes, we can use the file extension to match an attachment to a pluggable UI.
  3. 
      
AA
AA
AA
KA
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues
    It would be nice to simply prioritize the mimetype when looking for a best handler. So, instead of checking the file extension here, let get_best_handler run its course. If it couldn't find a handler using the mimetype, then check the file extension.  This way, if, say a jpeg is uploaded with the right mimetype, but a file extension in the MIMETYPE_EXTENSIONS, it will still pick the right handler.
    
    Also, checking the extension separately should make it a little easier to have many file extensions for the same file. You could add a class property of supported_file_extensions, and fill it with a list of file extensions. I know, for me, I've used '.md', '.mdown' and '.markdown' for Markdown files.
    
    Any thoughts?
    1. Good point. I took this chunk of code from r/3454 (which hasn't been merged yet). It was discussed here: http://reviews.reviewboard.org/r/3454/#comment7155
      
      I personally think though that it makes sense to have this code before "get_best_handler". One case I can think of is when a filename has an invalid extension (let's say, someone renamed "document.pdf" to "document.doc"). This code would run first and set the mimetype to be of a Word document. However, "get_best_handler" would then run and pick the appropriate mimetype. Doing it the other way around would cause unexpected behaviour in this scenario. 
  3. reviewboard/reviews/ui/base.py (Diff revision 6)
     
     
    Show all issues
    This would be more visible at the top of the file.
    1. Removed this from the file since it already exists in mimetypes.py (in r/3454). Instead, I am now importing MIMETYPE_EXTENSIONS from mimetypes.py to avoid code duplication.
  4. Other than the comments I've left, this is looking pretty good to me.
mike_conley
  1. 
      
  2. reviewboard/reviews/ui/__init__.py (Diff revision 6)
     
     
    Show all issues
    I wonder if it'd make more sense to just call it reviewboard.reviews.ui.markdown, as opposed to reviewboard.reviews.ui.markdownui. The ui in markdownui is a bit redundant.
    
    Just a nit.
    1. A naming conflict occurs when I rename the file as "markdown.py". I am using a Python package named "markdown" to render Markdown as HTML. However, when I do "import markdown" it imports "reviewboard.reviews.ui.markdown" instead of the Python package "markdown".
  3. reviewboard/settings.py (Diff revision 6)
     
     
     
     
     
    Show all issues
    Maybe I'm missing something here, but why are you changing these? Seems only tangentially related to the rest of the patch.
  4. setup.py (Diff revision 6)
     
     
    Show all issues
    We tend to keep the trailing commas in Python.
  5. 
      
AA
mike_conley
  1. Hey - so the code looks pretty good, and I tried the patch and was able to render some Markdown, which was awesome.
    
    I'm curious though - am I supposed to be able to attach comments to this Markdown? Did I miss something? Where does that come in?
  2. reviewboard/settings.py (Diff revision 7)
     
     
     
    Show all issues
    Again, I have to ask - why are these being added? I don't see these files being added in this patch...
    1. Mike, thanks for pointing those JS files out. I added them by mistake; I thought I had accidentally removed them but it turns out they were never there in the first place.
      
      As for the commenting functionality, I wasn't aware that that had to be implemented until you mentioned it. I will get the specifications and start working on that ASAP.
    2. I think that'll be the big blocker for now. The rest is great, and I wouldn't mind committing it to some branch, but ship-wise, we'll need to solve that.
  3. 
      
AA
chipx86
  1. What's needed before this can go in? Anything, or is it standalone at this point?
  2. reviewboard/static/rb/js/models/markdownReviewableModel.js (Diff revision 8)
     
     
     
     
     
     
     
    Show all issues
    Should inherit from FileAttachmentReviewable, and then you won't need 'caption' or 'attachmentID'.
    1. Christian, r/3613/ needs to be merged before. This should actually inherit from TextBasedReviewable (which inherits from FileAttachmentReviewable) so that this review UI can get the commenting functionality. 
      
      Once r/3613 is merged, I can fix the inheritance so it can be ready-to-go.
  3. 
      
AA
AA
AA
mike_conley
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 10)
     
     
    Show all issues
    Nit: alphabetical
  3. reviewboard/reviews/ui/markdownui.py (Diff revision 10)
     
     
    Show all issues
    Nit: remove this newline.
  4. reviewboard/reviews/ui/markdownui.py (Diff revision 10)
     
     
    Show all issues
    I think this is indented too much.
  5. 
      
AA
david
  1. 
      
  2. reviewboard/reviews/ui/markdownui.py (Diff revision 11)
     
     
     
    Show all issues
    StringIO is part of the standard library, so your imports should read like this:
    
    import StringIO
    
    import markdown
    
    from reviewboard.reviews.ui.base import FileAttachmentReviewUI
  3. 
      
chipx86
  1. It looks like this is including the text change as well. Can you make sure you're using --parent?
  2. reviewboard/reviews/ui/markdownui.py (Diff revision 11)
     
     
    Show all issues
    Should be:
    
    try:
        from cStringIO import StringIO
    except ImportError:
        from StringIO import StringIO
    
    
    That'll use cStringIO when available, which is faster.
  3. 
      
AA
AA
Review request changed
Status:
Discarded
Change Summary:
Updated version of this change is under review at https://reviews.reviewboard.org/r/4414/