Rendering pluggable UIs in lightbox

Review Request #3526 — Created Nov. 17, 2012 and discarded

aam1r
Review Board
master
reviewboard
Review UIs can now be shown inline in a lightbox (i.e. modalBox for now) window rather than having the user render a new page every time. 

To display a review UI as inline, "allow_inline" needs to be set to True in the Review UI subclass (see markdownui.py in the diff for an example). Custom URL patterns can also be defined by a Review UI subclass that can be used in the inline pluggable UI to allow additional custom functionality (such as fetching data through the server using AJAX).
Inline enabled for Markdown Review UI:
- Uploaded a Markdown file with no caption. Clicked "Review". Modal box popped up with filename as title. Displayed the rendered contents of the attachment.
- Uploaded a Markdown file with a caption. Clicked "Review". Modal box popped up with the attachment caption as title. Displayed the rendered contents of the attachment.
- Uploaded a Markdown file with no content. Clicked "Review". Modal box popped up. Displayed empty modal box (since attachment was originally empty).

Inline disabled for Markdown Review UI:
- Uploaded a Markdown file. Clicked "Review". Got redirected to the review UI page; no modal box was shown.

Custom URL pattern initialized in Markdown Review UI:
- Set a custom URL pattern in Markdown Review UI class. Accessing the URL manually (using curl and my browser) loaded the view of that URL pattern, as expected.
Loading file attachments...

Description From Last Updated

Probably should be the attachment's display name, or filename if that's not there.

chipx86chipx86

I probably screwed up, but there should be two blank lines.

chipx86chipx86

continuation line over-indented for hanging indent

JS jsintal

Since this is a function, it should be prefixed with get_() I think actually, just get_urlpatterns, since the context should ...

chipx86chipx86

When comparing against not, use "is" or "is not." In this case, though, don't do that comparison, just "if not ...

chipx86chipx86

We already have this in mimetypes.py, so let's not duplicate it here.

chipx86chipx86

Separate these. Anything imported from reviewboard.* is in the "current project imports" group. "django" is in the "3rd party imports" ...

chipx86chipx86

URLs should always end with "/". "fetch" is a weird URL. It's not likely what we want. Maybe "rendered."

chipx86chipx86

line too long

JS jsintal

line too long

JS jsintal

Should be aligned with "urlpatterns"

chipx86chipx86

Should be two blank lines.

chipx86chipx86

Alignment looks off, but no need to have one per line. Do you mean for this function to still be ...

chipx86chipx86

Where did the original files go? Merging issue?

chipx86chipx86

This is global. This should all be handled by the ReviewUI subclass. The click handler should only call something on ...

chipx86chipx86

The title should include the attachment display name or filename.

chipx86chipx86

Why are you limiting this to 500px?

daviddavid

We just hit some problems with dynamic URL handling for extensions, and came up with a nifty thing called DynamicURLResolver. ...

chipx86chipx86

Blank line between these. Third-party imports and current project imports must be in their own import groups.

chipx86chipx86

It'd be better to do ".... import get_urlpatterns as get_review_ui_urlpatterns", to make it clear what it is we're accessing.

chipx86chipx86

I was confused at first about which 'this' we were using. Maybe instead of passing the link, pass the URL.

chipx86chipx86

Use 1-space indentation, and make sure parameters align.

chipx86chipx86

Since mimeparse is 3rd party, it should be with the 2nd group of imports, right under: from djblets.util.urlresolvers import DynamicURLResolver

SL slchen

Exceeds max line length: from reviewboard.reviews.ui.base import get_urlpatterns \ as get_review_ui_urlpatterns

SL slchen
AA
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 1)
     
     
     
     
    Is there a more Pythonic way of doing this?
  3. 
      
AA
AA
AA
chipx86
  1. 
      
  2. Probably should be the attachment's display name, or filename if that's not there.
  3. reviewboard/reviews/ui/__init__.py (Diff revision 2)
     
     
     
     
    I probably screwed up, but there should be two blank lines.
  4. reviewboard/reviews/ui/base.py (Diff revision 2)
     
     
    Since this is a function, it should be prefixed with get_()
    
    I think actually, just get_urlpatterns, since the context should be clear from the file. Importers can alias it if needed.
  5. reviewboard/reviews/ui/base.py (Diff revision 2)
     
     
    When comparing against not, use "is" or "is not." In this case, though, don't do that comparison, just "if not review_ui.urlpatterns".
    
    That way, urlpatterns can be [] and it'll do the correct thing.
  6. reviewboard/reviews/ui/base.py (Diff revision 2)
     
     
     
     
    We already have this in mimetypes.py, so let's not duplicate it here.
  7. reviewboard/reviews/ui/markdownui.py (Diff revision 2)
     
     
     
    Separate these.
    
    Anything imported from reviewboard.* is in the "current project imports" group. "django" is in the "3rd party imports" group.
  8. reviewboard/reviews/ui/markdownui.py (Diff revision 2)
     
     
    URLs should always end with "/".
    
    "fetch" is a weird URL. It's not likely what we want. Maybe "rendered."
  9. reviewboard/reviews/ui/markdownui.py (Diff revision 2)
     
     
    Should be aligned with "urlpatterns"
  10. reviewboard/reviews/urls.py (Diff revision 2)
     
     
     
     
    Should be two blank lines.
  11. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
     
     
    Alignment looks off, but no need to have one per line.
    
    Do you mean for this function to still be here, though? This should only exist in the given ReviewUIs that need to do such things, and should not be common.
    1. I tried moving this function to the MarkdownReviewUI class and made it a class method. I also changed the URL pattern to be:
      
      (r'^(?P<review_request_id>[0-9]+)/file/(?P<file_attachment_id>[0-9]+)/rendered/$', 'reviewboard.reviews.ui.markdownui.rendered_attachment')
      
      but that didn't work. I get the following error: "Could not import reviewboard.reviews.ui.markdownui.rendered_attachment. View does not exist in module reviewboard.reviews.ui.markdownui." (http://i.imgur.com/2j9CL.png)
      
      Moving the function outside of that class works though but I think it should ideally be part of the ReviewUI subclass. Any ideas on how I can make a classmethod work?
  12. reviewboard/settings.py (Diff revision 2)
     
     
     
     
    Where did the original files go? Merging issue?
  13. reviewboard/static/rb/js/reviews.js (Diff revision 2)
     
     
    This is global. This should all be handled by the ReviewUI subclass. The click handler should only call something on ReviewUI.
    
    Remember not not all ReviewUIs need to fetch data to display on the server.
    
    The only part that should be common is the modalBox.
    1. Moved this to the "showReviewUI()" function defined in the same file but that still needs some work to be done. Instead of hardcoding it to be "rendered", I need to make it so that it first checks whether the ReviewUI does support lightbox displaying. Once that has been verified, it needs to get the URL for to get the HTML data to render.
  14. The title should include the attachment display name or filename.
  15. 
      
JS
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 2)
     
     
    continuation line over-indented for hanging indent
  3. reviewboard/reviews/ui/markdownui.py (Diff revision 2)
     
     
    line too long
  4. reviewboard/reviews/ui/markdownui.py (Diff revision 2)
     
     
    line too long
    1. Not sure how I can make it any shorter. Several lines in reviews/urls.py also have the same issue.
      
      Any ideas?
    2. Don't worry about the line length in urls.py. There's no good way to wrap the regexps.
  5. 
      
AA
AA
david
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 4)
     
     
     
     
     
     
     
     
    If you wanted to make this cuter, you could use a list comprehension.
    1. How would that list comprehension look?
      
      I am trying: 
          urls = [ui.urlpatterns for ui in _file_attachment_review_uis if ui.urlpatterns]
      
      But that gives a different result than using a loop:
      
      (Pdb) urlpatterns
      [<RegexURLPattern None ^(?P<review_request_id>[0-9]+)/file/(?P<file_attachment_id>[0-9]+)/rendered/$>]
      (Pdb) urls
      [[<RegexURLPattern None ^(?P<review_request_id>[0-9]+)/file/(?P<file_attachment_id>[0-9]+)/rendered/$>]]
    2. It's making a list of lists, so you can flatten it like this:
      itertools.chain(*urls)
      
      That said, maybe it's better to be clearer than cuter :)
  3. reviewboard/static/rb/js/reviews.js (Diff revision 4)
     
     
    Why are you limiting this to 500px?
  4. 
      
AA
AA
AA
AA
AA
AA
AA
chipx86
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 7)
     
     
     
     
     
     
     
     
    We just hit some problems with dynamic URL handling for extensions, and came up with a nifty thing called DynamicURLResolver. We should use that here so that we can make it work when extensions are enabled/disabled.
    
    You can import it from djblets.util.urlresolvers.
    
    You should then do:
    
    _dynamic_urls = DynamicURLResolver()
    
    def get_urlpatterns():
        urlpatterns = patterns('', _dynamic_urls)
    
        for blah:
            _dynamic_urls.add_patterns(review_ui.urlpatterns)
    
    return urlpatterns
    
    
    Registration should then do an add_patterns on the reviewui's urlpatterns, and unregistration should do remove_paterns.
  3. reviewboard/reviews/urls.py (Diff revision 7)
     
     
     
    Blank line between these. Third-party imports and current project imports must be in their own import groups.
  4. reviewboard/reviews/urls.py (Diff revision 7)
     
     
    It'd be better to do ".... import get_urlpatterns as get_review_ui_urlpatterns", to make it clear what it is we're accessing.
  5. reviewboard/static/rb/js/reviews.js (Diff revision 7)
     
     
    I was confused at first about which 'this' we were using. Maybe instead of passing the link, pass the URL.
  6. reviewboard/templates/reviews/review_request_box.html (Diff revision 7)
     
     
     
     
     
     
     
    Use 1-space indentation, and make sure parameters align.
  7. 
      
AA
AA
AA
SL
  1. 
      
  2. reviewboard/reviews/ui/base.py (Diff revision 10)
     
     
    Since mimeparse is 3rd party, it should be with the 2nd group of imports, right under:
    
    from djblets.util.urlresolvers import DynamicURLResolver
    1. Hmm, but isn't djblets also considered 3rd party? 
      
      I guess even then, alphabetically it makes sense to put "import mimeparse" after "from * import *". I will fix this. Thanks!
    2. Yes, from reviewboard's perspective, it's third party.
  3. 
      
TI
  1. Looks great Aamir! Looking forward to use this with the inline file attachments UI.
  2. reviewboard/static/rb/js/reviews.js (Diff revision 10)
     
     
    I'm wondering what will happen when there's unsaved data in the dlg when you try to close it. Will it trigger the check for unsaved actions when you destroy the dlg?
    1. Tina, could you emphasize a bit more? Not sure what unsaved data and dialog you're referring to. showReviewUI gets called when a user clicks "Review" for a file attachment (see screenshots). 
    2. oops, sorry let me try this again. Please correct me if I'm wrong, but I thought the different review UI will be embedded in the body of the showReviewUI dialog? From interacting with the existing UI, as a user I would expect if there is unsaved comment in any review UI I would get a prompt before I leave the page reminding me that there is unsaved work, are you sure you want to leave this page? I was wondering if the review UIs have the same function? and if so when they are embedded in the showReviewUI dialog are they behaving the same way? I was worry that the close button would just remove the dialog html from <body> and you won't get a reminder. Does that make a bit more sense?
    3. On that note, if you have a comment dialog open and close the editor, I imagine the dialog remains? We'll need to solve these issues.
  3. reviewboard/static/rb/js/reviews.js (Diff revision 10)
     
     
    I'm curious, what happens if the title (eg. caption) is longer than the width of the dlg?
    1. Good question. I tried it and it just continues over to the next line. I've added a screenshot.
    2. Thanks for checking! I think that looks okay.
  4. I personally think it would be useful if the title also show the MIME type and/or filename of the file being reviewed, even if there is a caption. What do you think?
    1. I think it would be pretty neat actually to show the filename as well as the caption. If "README.md" had the caption "our updated README file", I could make it so that it's "README.md: our updated README file". It gives more context when they're viewing the UI and fills up that empty space in the title bar.
      
      I am not sure though if that is consistent with the rest of the UI. 
      
      Mentors: any thoughts?
    2. Maybe just in parens. "My image (foo.png)"
      
      Of course, if there's no caption, it should just show the filename still.
      
      Another thing to consider is draft captions. If there's a draft object in the context and there's a file.draft_caption, that should be used. I think David just recently committed a change similar to that.
  5. 
      
AA
AA
mike_conley
  1. I'm quite happy with this. Thank you Aamir.
  2. 
      
mike_conley
  1. Grr - keep forgetting to hit "Ship it".
  2. 
      
mike_conley
  1. Grr - keep forgetting to hit "Ship it".
  2. 
      
SL
  1. 
      
  2. reviewboard/reviews/urls.py (Diff revision 11)
     
     
    Exceeds max line length:
    
    from reviewboard.reviews.ui.base import get_urlpatterns \
        as get_review_ui_urlpatterns
    1. Not sure how I feel about moving it to the next line since it's only 4 extra characters.
      
      Mentors: any thoughts on whether it's a good idea to move this to the next line or is this okay?
    2. You could just rename this get_review_ui_urls and get the best of both worlds.
    3. I like that better.
      
      But in general, yes, it should be wrapped, even on imports.
  3. 
      
AA
AA
Review request changed

Status: Discarded

Loading...