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

Add some type file preview and more #28

Merged
merged 52 commits into from
Oct 11, 2018
Merged

Add some type file preview and more #28

merged 52 commits into from
Oct 11, 2018

Conversation

basilelegal
Copy link
Contributor

Adds preview for scribus file .sla. fix #25
Adds test for gimp file .xfc. fix #26
Refactor openoffice and scribus preview builder.
Complete README.
Fix bug with exif tool. fix #21
Adds some PEP8.

@basilelegal basilelegal requested a review from buxx September 28, 2018 10:14
@buxx buxx changed the title Scribus Add some type file and more Sep 28, 2018
@buxx buxx changed the title Add some type file and more Add some type file preview and more Sep 28, 2018
cache_path
))
result = check_call(
[
Copy link

Choose a reason for hiding this comment

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

Basile, pour le script scribus, le mieux est de le stocker dans un string dans ton code et de faire du «pipe» avec la commande, genre echo $content | scribus ...`

Ne le change pas maintenant mais crees un ticket pour refactoriser ça


"""
abstract function to transform a file given in bytes to pdf
:param file_content: stream
Copy link

Choose a reason for hiding this comment

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

Abstract class, not function... en fait la docstring est pas au bon endroit

@@ -0,0 +1,162 @@
<?xml version="1.0" encoding="UTF-8"?>
<SCRIBUSUTF8NEW Version="1.4.6">
Copy link

Choose a reason for hiding this comment

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

Is this a file with.images inside ? If not, please change the file for a more complete one

setup.py Outdated
@@ -33,7 +33,9 @@
'Wand',
'PyPDF2',
'Pillow',
'Sweepatic-PyExifTool==0.2'
'wand',
Copy link

Choose a reason for hiding this comment

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

Duplicated dependency

cache_file = os.path.join(cache_path, preview_name)

if self._cache_file_process_already_running(cache_file):
# Note - Basile - infinite recursion protection
Copy link
Contributor

Choose a reason for hiding this comment

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

Notes must contain date, tale a look at https://algoo.trac.im/workspaces/11/folders/1194/pages/2043 , section "Commentaires".

Copy link
Contributor

Choose a reason for hiding this comment

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

up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

complete_path = file_path + '.' + file_ext
str, encoding = mimetypes.guess_type(complete_path)
_str, encoding = mimetypes.guess_type(complete_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

If y have a good reading of PEP8:

single_trailing_underscore_: used by convention to avoid conflicts with Python keyword, e.g.

You should name str_ instead _str.

if not os.path.exists(intermediate_pdf_file_path):
if os.path.exists(intermediate_pdf_file_path + '_flag'):
# Wait 2 seconds, then retry
time.sleep(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why waiting for 2 sec ? This may cause a process who use preview_generator to hang more than 2 second ?
An "INFO" comment may be nice here explaining the goal of this.

Copy link
Member

Choose a reason for hiding this comment

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

you're right that an explanation is a good idea.
The use case is the following one:

you ask for a jpeg preview of a libreoffice document. the first step is to convert odt to full pdf. If at the same time you ask for a jpeg preview of another page, then the full pdf will be geenrated twice, resulting in a corrupt file (and loss of time). Waiting 2 seconds let the running transformation to finish (the 2s delay is only executed in case a preview is already running)

@basilelegal could you add a comment explaining this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Author of this code is not @basilelegal (it's a move of code). But, maybe he is able to respond ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's coming with the next push

Copy link
Contributor

Choose a reason for hiding this comment

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

up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tracim
Copy link

tracim commented Oct 11, 2018

@buxx @basilelegal my point of view is that code is in a stable / managed state. Not perfect but problems are identified (and should be written in new issues - @basilelegal did you write these issues?)

My feeling is we can merge this and start from new branches for fixes remaining problems. @buxx what do you think?

@basilelegal
Copy link
Contributor Author

@tracim I created 2 new issues and I'm waiting for @buxx to do a full review before merging / new release.

@buxx
Copy link
Contributor

buxx commented Oct 11, 2018

@buxx @basilelegal my point of view is that code is in a stable / managed state. Not perfect but problems are identified (and should be written in new issues - @basilelegal did you write these issues?)

My feeling is we can merge this and start from new branches for fixes remaining problems. @buxx what do you think?

Agree with that, ready to merge !

@buxx buxx merged commit 2426a0d into master Oct 11, 2018
@buxx buxx deleted the scribus branch October 11, 2018 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants