Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

previewer: pdfjs plugin addition #2402

Merged
merged 5 commits into from Nov 26, 2014
Merged

previewer: pdfjs plugin addition #2402

merged 5 commits into from Nov 26, 2014

Conversation

bouzlibop
Copy link
Member

  • Adds pdfjs previewer.

Signed-off-by: Adrian Pawel Baran adrian.pawel.baran@cern.ch

@bouzlibop
Copy link
Member Author

@lnielsen-cern Please check also this: zenodo/zenodo#15



def can_preview(f):
'''Returns True for PDFs, False for others'''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use double quotes `"""instead of single quotes'''```.

@jirikuncar
Copy link
Member

@bouzlibop if we are keeping previous implementation we need to create separate bundles. Please check also annotations interface that it's still working. Thank you.

WDYT about commit message: previewer: pdfjs plugin addition ?

@jirikuncar jirikuncar changed the title previewer: New pdfjs previewer previewer: pdfjs plugin addition Oct 14, 2014
@bouzlibop
Copy link
Member Author

@jirikuncar, the new commit message title sounds good.

I checked the annotations module. Thanks for pointing that out. I'll prepare a separate bundle for this new pdfjs previewer. I believe that it would be worth to have the name "pdfjs" for the bundle for new previewer and change the name of the bundle used in annotations to sth else. Just to keep the clearness of naming (this is what I've used: http://mozilla.github.io/pdf.js/).

@jirikuncar
Copy link
Member

@bouzlibop I have made some little fixes for annotations module. Feel free to rename the bundles after #2410 is merged.

@@ -1,5 +1,5 @@

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1: D208 Docstring is over-indented
118:1: F999 redefinition of unused 'compile_text' from line 111

output="previewer/pdfjs.js",
weight=20,
bower={
"pdfjs-build": "git://github.com/bouzlibop/pdfjs-build"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lnielsen-cern Here I put the url to my repository with customized pdfjs-build.
I tested it and it's working. Probably it should be one of zenodo repositories.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the upstream package and just install custom js/css files from Invenio (Zenodo) repo?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The official bower package for pdfjs is pdfjs-dist, but this one can be probably only used to build own version of the viewer and it doesn't contain the official version of the viewer (the one I am using here).
The two example ways of how to obtain this official viewer are:

  1. Use the official repo https://github.com/mozilla/pdf.js as endpoint for bower install and then build the viewer locally (node make generic)
  2. Use the 3rd party repo (eg. git://github.com/RiptideCloud/pdfjs-build.git), which comes with the prebuild version of the viewer

And then, there still remains the problem with some paths hardcoded in the scripts (like PDFJS.imageResourcesPath for example), which we need to customize in order to make the viewer work in invenio.

The simplest solution which comes to my mind (as I mentioned in the first comment) is to fork the 3rd party repo and make the changes we need.

But I'm not sure if this is the best way to solve this.

@lnielsen
Copy link
Member

During integration on Zenodo I've made some changes that should likely be added to the pull request, so I'm just changing it to WIP

@lnielsen lnielsen changed the title previewer: pdfjs plugin addition WIP previewer: pdfjs plugin addition Oct 20, 2014
@bouzlibop
Copy link
Member Author

@lnielsen-cern could you please specify what are those changes that needs to be add here ?

@lnielsen
Copy link
Member

Yes, it is in these commits:
zenodo/invenio@8061bf7
zenodo/invenio@a5e7429
zenodo/invenio@12e5e3e
zenodo/invenio@0ca6c46
zenodo/invenio@da81901

However, also we should like copy the viewer html and css into Invenio and use the standard bower build of pdfjs.

@bouzlibop
Copy link
Member Author

@lnielsen-cern I got through the commits you pointed out and applied the changes to this PR. Now I will try to look into switching to bower build.

@bouzlibop
Copy link
Member Author

@lnielsen-cern what do you mean by "use the standard bower build of pdfjs" ? Should we use the official repo https://github.com/mozilla/pdf.js as endpoint for bower install and then build the viewer locally (node make generic) ? Or should we do it in some other way?

@lnielsen
Copy link
Member

Official is probably not the right word. "Existing" bower package is better. As far as I remember you said that the official build could not be used out of the box.

@bouzlibop
Copy link
Member Author

I think that this one - mozilla/pdfjs-dist - can be used only for building some custom viewer. In our case we can use this one: RiptideCloud/pdfjs-build or (as I mentioned in the previous comment) - we can use official mozilla/pdfjs repo as endpoint for bower and then locally perform node make generic to build the viewer.

In either case, what about this changes https://github.com/bouzlibop/pdfjs-build/compare/RiptideCloud:master...master#diff-564dfd8dd910689dffde37ab83a2a010L58 in viewer.js ? Do you think we can solve that problem with relative path?

@lnielsen
Copy link
Member

and then locally perform node make generic to build the viewer.

I would like to avoid running extra commands besides bower install so probably best to use RiptideCloud/pdfjs-build

In either case, what about this changes bouzlibop/pdfjs-build@RiptideCloud:master...master#diff-564dfd8dd910689dffde37ab83a2a010L58 in viewer.js ? Do you think we can solve that problem with relative path?

The viewer.js I would simply integrate in Invenio source code.

@bouzlibop
Copy link
Member Author

@lnielsen-cern I've just added the full screen mode and I've changed the structure a little bit - now I'm using the RiptideCloud/pdfjs-build package. I still have to test a few things here, but meanwhile if you find a time please have a birds-eye view over the changes 😄

@jirikuncar
Copy link
Member

I would rename invenio/modules/previewer/static/js/previewer/pdfjs_previewer/pdfjs.js to invenio/modules/previewer/static/js/previewer/pdfjs/viewer.js and prepare initial commit with original version in name of the original author and then commit our changes.

@lnielsen
Copy link
Member

@bouzlibop Can you take https://github.com/lnielsen-cern/invenio/tree/pu-previewer-pdfjs and repush this PR. I've rebased the code, cleaned up some few things and checked that all code from Zenodo was integrated.

Once it's done, you can remove the WIP from the title, so that can get it integrated.

@bouzlibop
Copy link
Member Author

@lnielsen-cern, many thanks for the clean up.

Today I tested this PR (after taking the lnielsen-cern/invenio/tree/pu-previewer-pdfjs) and in order to make things work I did some changes in this commit 49d6b69. Specifically files:

  • invenio/modules/previewer/bundles.py
  • invenio/modules/previewer/static/css/previewer/pdfjs/viewer.css

Could you have a look at this final version?

PS: I tested this PR in both invenio and zenodo (ASSETS_DEBUG=False/True)

@bouzlibop bouzlibop changed the title WIP previewer: pdfjs plugin addition previewer: pdfjs plugin addition Nov 19, 2014
@lnielsen
Copy link
Member

LGTM. Just missing the test to pass (seems to be a sphinx doc build issue).

@bouzlibop
Copy link
Member Author

@jirikuncar, I think it's ready to be merged.

@lnielsen
Copy link
Member

@bouzlibop Can you just try to run ``python setup.py build_sphinx -b html" locally to see why the documentation build fails.

@bouzlibop
Copy link
Member Author

/home/adrian/.virtualenvs/invenio/src/invenio/docs/ext/elasticsearch.rst:21: WARNING: autodoc: failed to import module u'invenio.ext.elasticsearch'; the following exception was raised:
Traceback (most recent call last):
  File "/home/adrian/.virtualenvs/invenio/local/lib/python2.7/site-packages/sphinx/ext/autodoc.py", line 335, in import_object
    __import__(self.modname)
  File "/home/adrian/.virtualenvs/invenio/src/invenio/invenio/ext/elasticsearch/__init__.py", line 86, in <module>
    from pyelasticsearch import ElasticSearch as PyElasticSearch
ImportError: No module named pyelasticsearch
/home/adrian/.virtualenvs/invenio/src/invenio/docs/modules/jsonalchemy.rst:557: WARNING: autodoc: failed to import class u'MongoDBStorage' from module u'invenio.modules.jsonalchemy.jsonext.engines.mongodb_pymongo'; the following exception was raised:
Traceback (most recent call last):
  File "/home/adrian/.virtualenvs/invenio/local/lib/python2.7/site-packages/sphinx/ext/autodoc.py", line 335, in import_object
    __import__(self.modname)
  File "/home/adrian/.virtualenvs/invenio/src/invenio/invenio/modules/jsonalchemy/jsonext/engines/mongodb_pymongo.py", line 22, in <module>
    import pymongo
ImportError: No module named pymongo
/home/adrian/.virtualenvs/invenio/src/invenio/docs/modules/oauthclient.rst:37: WARNING: autodoc: failed to import module u'invenio.modules.oauthclient.contrib.github'; the following exception was raised:
Traceback (most recent call last):
  File "/home/adrian/.virtualenvs/invenio/local/lib/python2.7/site-packages/sphinx/ext/autodoc.py", line 335, in import_object
    __import__(self.modname)
  File "/home/adrian/.virtualenvs/invenio/src/invenio/invenio/modules/oauthclient/contrib/github.py", line 80, in <module>
    import github3
ImportError: No module named github3

@jirikuncar
Copy link
Member

you need to run pip install -e .[docs] first.

@bouzlibop
Copy link
Member Author

Thanks @jirikuncar, now there are no errors during python setup.py build_sphinx -b html. I will try to restart Travis build then.

lnielsen and others added 5 commits November 26, 2014 10:11
* Adds cleancss assets filter with support for rebasing relative URLs
  against COLLECT_STATIC_ROOT.

Signed-off-by: Lars Holm Nielsen <lars.holm.nielsen@cern.ch>
* Adds pdfjs viewer component from Mozilla pdf.js as a single commit.
  This commit is needed since the viewer part of pdf.js cannot be
  installed via bower.

* NOTE: All source code in commit is copyrighted by Mozilla
  Foundation and licensed under Apache License Version 2.0.

Reviewed-by: Lars Holm Nielsen <lars.holm.nielsen@cern.ch>
Signed-off-by: Adrian Pawel Baran <adrian.pawel.baran@cern.ch>
* Integrates pdf.js support in Invenio including customization of
  default pdf.js viewer.

* Fixes problem with pdftk bundle naming collision.

* Fixes problem with jQuery loading in records module.

Reviewed-by: Lars Holm Nielsen <lars.holm.nielsen@cern.ch>
Signed-off-by: Adrian Pawel Baran <adrian.pawel.baran@cern.ch>
* Fixes issue with ZIP files with no content.

* Fixes minor styling issues.

* Changes icons from Bootstrap to Font Awesome.

Signed-off-by: Lars Holm Nielsen <lars.holm.nielsen@cern.ch>
* Adds new alias for `webassets.filter.ExternalTool`.

Signed-off-by: Adrian Pawel Baran <adrian.pawel.baran@cern.ch>
@jirikuncar jirikuncar merged commit 9e31b2f into inveniosoftware:pu Nov 26, 2014
@bouzlibop bouzlibop deleted the zenodo-194-pdfpreviewer branch February 5, 2015 08:52
@jirikuncar jirikuncar modified the milestones: v2.x, v2.0.0 Mar 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants