the-paperless-project / paperless

Scan, index, and archive all of your paper documents
GNU General Public License v3.0
7.86k stars 499 forks source link

Ability to both open documents inline as well as download them #241

Open ddddavidmartin opened 7 years ago

ddddavidmartin commented 7 years ago

Good day,

I was thinking that it would be nice to have the option to both view documents inline as well as download them. Currently, when you click a thumbnail in the overview it will always trigger a download. Often I just want to look at something though and avoid the download. Switching this to inline viewing can be done with the content-disposition as below:

diff --git a/src/documents/views.py b/src/documents/views.py
index b57a0a5..b749973 100644
--- a/src/documents/views.py
+++ b/src/documents/views.py
@@ -68,7 +68,7 @@ class FetchView(SessionOrBasicAuthMixin, DetailView):
             GnuPG.decrypted(self.object.source_file),
             content_type=content_types[self.object.file_type]
         )
-        response["Content-Disposition"] = 'attachment; filename="{}"'.format(
+        response["Content-Disposition"] = 'inline; filename="{}"'.format(
             self.object.file_name)

         return response

This in itself does not seem like a great solution though as the code overall refers to this as the download url and I think it makes sense to be a download url.

One option that came to my mind was on mouse-over on a thumbnail to have two buttons, one for download and one for viewing. Similar to what gmail does with attachments [0].

I'm happy to do a bit of tinkering around but I'm not quite sure where the actual layout of the document list is defined.

Any thoughts? :)

[0] screen shot 2017-06-24 at 1 01 35 pm screen shot 2017-06-24 at 12 58 08 pm

MasterofJOKers commented 7 years ago

If I'm not mistaken, https://github.com/danielquinn/paperless/blob/master/src/documents/templates/admin/documents/document/change_list_results.html#L153 is the place to look at for tinkering with that idea.

danielquinn commented 7 years ago

This is a great idea. If you're interested in doing the legwork, I'm happy to review & merge a PR.

Here's a few places to start:

The simplest way to handle attachment vs. inline downloads is probably just to tweak the FetchView to accept a GET parameter. Then set Content-Disposition accordingly:

disposition = self.request.GET.get("disposition", "attachment")
response["Content-Disposition"] = '{}; filename="{}"'.format(
    disposition,
    self.object.file_name
)

As for how to handle the popup, Django's admin provides a copy of Bootstrap 3 in there, so you should be able to do what you're looking for with a popover.

As for @MasterofJOKers's comment, that's absolutely correct, but if you go digging in there understand that this entire page is a Terrible Hack on top of Django's default change_list admin view, and the markup generated here is most definitely terrible. I overrode the default table-based view to use divs in a grid, but if you view the source, you'll see that remnants of the table bits are still there and may not be easy to remove. Your best bet might be just to target .image a rather than trying to place a class on the <a> tags themselves.

ddddavidmartin commented 7 years ago

Thanks guys! I don't quite understand yet what is required but you are giving me some good pointers on what to look into. I'll do a bit of reading and see how it goes :)

ddddavidmartin commented 7 years ago

I started having a look at this and made some initial changes on my inline_document_viewing branch. Instead of having the disposition as a GET parameter I have added it as a separate URL. Documents can be shown under /view/doc/<pk> which seems a bit more explicit. I'm happy to be convinced either way there though.

I have 2 questions: 1) How do I run the tests locally? 2) @danielquinn:

Your best bet might be just to target .image a rather than trying to place a class on the tags themselves.

Would you mind elaborating on this a little bit? I fear I'm not quite following. Thanks!

danielquinn commented 7 years ago

Hi @ddddavidmartin, I'll answer both of those for you.

  1. I typically use pytest from the src root (same directory as manage.py). That's good when you're developing, but before you issue a PR, you should run tox from the same directory as that will attempt to run the tests in different Python versions (if you have them available) as well as run pep8 against the code.
  2. By now you've probably looked at the HTML that is generated for the listing page and seen that each box in which we render a pdf looks like this:
    <div class="box">
      <div class="result">
        <div class="header">
          <div class="checkbox"><td class="action-checkbox"><input class="action-select" name="_selected_action" type="checkbox" value="285" /></td></div>
          <div class="info">
            <td class="field-correspondent nowrap">-</td><br />
            <th class="field-title"><a href="/admin/documents/document/285/change/">Some Document</a></th>
          </div>
          <div style="clear: both;"></div>
        </div>
        <div class="tags"><td class="field-tags_">&nbsp;</td></div>
        <div class="date"><td class="field-created nowrap">March 31, 2017, 8:03 p.m.</td></div>
        <div style="clear: both;"></div>
        <div class="image"><td class="field-thumbnail"><a href="/fetch/doc/285"><img src="/fetch/thumb/285" title="20170331200313-some-document.pdf" alt="Thumbnail of 20170331200313-some-document.pdf" width="180"/></a></td></div>
      </div>
    </div>

You'll note that there's a few (definitely incorrect) uses of <td> in there -- I can't do anything about those, as I haven't yet been able to hack around the way Django forces page generation. Unfortunately, this means that the HTML generated is invalid and while most browsers will compensate for it when rendering it to others, I have no idea what it might do to your Javascript when tying to attach methods do a DOM element.

So, when I suggested that you "target .image a I meant that when using Javascript to do something, you should take care not to do something like:

$(".box .result .image .a").on("click", function(){
  // do something
});

Because I have no idea how that might work with all of those invalid <td> elements in there. Instead, if you were to be more general and use $(".image a") as your selector, you might have an easier time.

psmit commented 6 years ago

What is the status of this issue? @ddddavidmartin I see you have a branch https://github.com/ddddavidmartin/paperless/tree/inline_document_viewing . Did the code there work, or were there some big issues?

ddddavidmartin commented 6 years ago

Hi @psmit, thanks for checking! I haven't had a chance to further look into this. On my branch there are only two commits (https://github.com/ddddavidmartin/paperless/commit/91c11e218a8cf768d46d10e819a9eeca69c47d14 and https://github.com/ddddavidmartin/paperless/commit/10b54806f026e8d0392738f45fc9d56c4f139a16). These modify Paperless to have two separate URLs to download and view documents respectively and should be a good starting point.

Modifying the actual HTML to get two buttons for the actions is somewhat out of my depth and I haven't been able to get myself to further look into it. Feel free to give it a try :)

ddddavidmartin commented 6 years ago

Hi @danielquinn, would you be able to label this issue as 'help wanted'? I don't seem to be able to do that. Thanks!