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

Ajoute la possibilité d'utiliser des images SVG dans un tutoriel #2430

Merged
merged 6 commits into from
Mar 23, 2015

Conversation

firm1
Copy link
Contributor

@firm1 firm1 commented Mar 16, 2015

Q R
Correction de bugs ? oui
Nouvelle Fonctionnalité ? non
Tickets (issues) concernés #1634

Cette PR permet de donner la possibilité d'ajouter des images de types svg à un tutoriel. Des tests sont associés à la résolution du bug pour éviter les regressions.

Note pour QA

  • Installer la dépendance os necessaire (elle est précisée dans le update.md) : apt-get install libffi-dev
  • Mettre à jour les dépendances python : pip install --upgrade -r requirements.txt -r requirements-dev.txt
  • Créer un tutoriel et insérer un lien vers une image svg (dans l'introduction, conclusion et/ou extrait)
  • Publier le tutoriel, et constatez que vous n'avez plus d'erreur 500.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.00% when pulling 81e202c on firm1:fix-1634 into 3e45ff2 on zestedesavoir:dev.

@SpaceFox
Copy link
Contributor

\o/ je sens que j'aime cette PR !

ext = filename.split(".")[-1].lower()
if ext == "svg":
cairosvg.svg2png(url=down_path,
write_to=os.path.join(pt, "images", filename.split(".")[0] + ".png"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Question très naïve : est-ce qu'on est obligés de convertir ? De mémoire les navigateurs gèrent le SVG non ?

Si on a une contrainte, il faut penser à vérifier que la conversion SVG --> PNG conserve bien les informations de transparence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question très naïve : est-ce qu'on est obligés de convertir ?

L'obligation de conversion est là uniquement pour l'export dans les autres formats.

Aujourd'hui on peut déjà mettre du svg dans un tuto, il s'affiche bien dans le navigo hors ligne. Mais lors de la publication, ça plante entre autre parce que PIL ne sait pas gérer le format SVG.

Copy link
Contributor

Choose a reason for hiding this comment

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

Erf. Dommage pour PIL, ça aurait allégé nos PDF en plus. Mais du coup OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

C'est vraiment dommage, parce que ça permettrait d'afficher les icônes de tutos correctement sur des écrans à densité de pixel plus haute (rétina...)...

Exemple:

Et question: quelle est la taille en pixel du PNG ? Ca dépend du SVG ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'est vraiment dommage, parce que ça permettrait d'afficher les icônes de tutos correctement sur des écrans à densité de pixel plus haute (rétina...)

les icones de tuto, c'est un autre problème. Elles sont en png, et il faudrait les convertir en svg pour obtenir ce que tu veux. Sauf qu'il n'existe quasi rien qui permet de convertir du png en pdf.

Et question: quelle est la taille en pixel du PNG ? Ca dépend du SVG ?

ça dépend du svg.

Copy link
Contributor

Choose a reason for hiding this comment

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

ça dépend du svg.

Donc théoriquement, on peut uploader un SVG avec une taille quasi-infinie, mais qui fasse seulement quelques Ko, et qui génèrera un PNG de peut-être plusieurs Go ? Et si j'ai pas la bonne echelle sur le SVG, j'aurais une image beacoup trop petite (alors qu'a la base c'est du vectoriel...). Je sais pas si c'est envisageable, mais il faudrait avoir une taille min et une taille max en pixels (hauteur et largeur) pour le PNG en sortie...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En fait, pour replacer le contexte ici, il s'agit uniquement des svg présents dans les liens externes (on ne peut pas ajouter de svg a une gallerie). Donc les svg ne sont uploadés que le jour ou un validateur valide le tutoriel.

Donc partant de là, si ton image est trop petite, ça se voit lors de la validation puisque ton navigateur affichera toujours la version svg. Si ton image est immense, ça se voit aussi lors de la validation.

@firm1
Copy link
Contributor Author

firm1 commented Mar 16, 2015

Ah petite note tout de même, j'ai fait la PR en l'état pour laisser la QA faire son travail. Mais il ne faudra pas la merger tant que je n'ai pas rassemblé les 5 commits en un seul.

Désolé.

@artragis
Copy link
Member

Je réserve la QA pour demain midi.

correction pep8

suppression d'un print

corrections des svg avec chemins relatifs

mise à jour des actions de releases
@landscape-bot
Copy link

Code Health
Repository health decreased by 0.00% when pulling b8f8a6c on firm1:fix-1634 into 3e45ff2 on zestedesavoir:dev.

@firm1
Copy link
Contributor Author

firm1 commented Mar 17, 2015

Mais il ne faudra pas la merger tant que je n'ai pas rassemblé les 5 commits en un seul.

C'est maintenant rassemblé

@@ -22,13 +22,14 @@ Assurez vous que les dépendances suivantes soient résolues :
- libxlst-dev (peut être appelée libxlst1-dev sur certains OS comme ubuntu
- libz-dev (peut être libz1g-dev sur système 64bits)
- python-sqlparse
- libffi : ``apt-get install libffi-dev``
Copy link
Contributor

Choose a reason for hiding this comment

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

Pour Mac OS et Windows, c'est quoi les dépendances ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Il n'ya normalement pas de dépendance pour les autres OS

@DevHugo
Copy link
Contributor

DevHugo commented Mar 17, 2015

Fail copie-collé désolé.

@artragis
Copy link
Member

  • Créer un tutoriel et insérer un lien vers une image svg (dans l'introduction, conclusion et/ou extrait)
  • Publier le tutoriel, et constatez que vous n'avez plus d'erreur 500.

Pas d'erreur 500 à signaler quand j'utilise un lien externe.
Donné que rien ne change dans les galleries, j'ai supposé que rien de spécial n'est fait quand on met un svg dans la gallerie.

Par contre, effectivement un svg qui est initialement gros reste gros et ça peut vite devenir lourd en therme de png. il faudrait limiter la taille du PNG généré à 800px par exemple.

@firm1
Copy link
Contributor Author

firm1 commented Mar 17, 2015

Donné que rien ne change dans les galleries, j'ai supposé que rien de spécial n'est fait quand on met un svg dans la gallerie.

oui, les svg sont interdit dans les galleries.

Par contre, effectivement un svg qui est initialement gros reste gros et ça peut vite devenir lourd en therme de png.

Quand tu dis reste gros, c'est à l'export ou c'est à l'affichage de la page web ?

@artragis
Copy link
Member

De ce que j'ai vu : à l'export.

Le 17 mars 2015 14:15, firm1 notifications@github.com a écrit :

Donné que rien ne change dans les galleries, j'ai supposé que rien de
spécial n'est fait quand on met un svg dans la gallerie.

oui, les svg sont interdit dans les galleries.

Par contre, effectivement un svg qui est initialement gros reste gros et
ça peut vite devenir lourd en therme de png.

Quand tu dis reste gros, c'est à l'export ou c'est à l'affichage de la
page web ?


Reply to this email directly or view it on GitHub
#2430 (comment)
.

@firm1
Copy link
Contributor Author

firm1 commented Mar 17, 2015

De ce que j'ai vu : à l'export.

Ok, je vais voir comment je peux définir une taille maximale aux png générés

@pierre-24
Copy link
Member

oui, les svg sont interdit dans les galleries.

Question: est ce qu'il y a une raison particulière ou est ce que c'est tout simplement parce que c'est pas le but de la PR (hors-scope) et que ça en demande une autre ?

@firm1
Copy link
Contributor Author

firm1 commented Mar 17, 2015

Question: est ce qu'il y a une raison particulière ou est ce que c'est tout simplement parce que c'est pas le but de la PR (hors-scope) et que ça en demande une autre ?

Plusieurs raisons :

  • Le scope de la PR : je m'en tiens ici à la correction du bug, pas à l'ajout d'une nouvelle feature
  • La feature "gérer les svg" dans les gallerie est un peu plus complexe que ce que ça laisse présager
  • La lib easy_thumbnails qui gère nos miniatures ne gère pas les svg.
  • Globalement les svg sont rapidement casse bonbon

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.05% when pulling f9f8fc2 on firm1:fix-1634 into 3e45ff2 on zestedesavoir:dev.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.02% when pulling 187570b on firm1:fix-1634 into 0708f8b on zestedesavoir:dev.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.01% when pulling e384e0d on firm1:fix-1634 into 0708f8b on zestedesavoir:dev.

@firm1
Copy link
Contributor Author

firm1 commented Mar 19, 2015

Bon, sur vos conseils j'ai appliqué un resizing de l'image avant de la convertir, avec une max_size à 800 pixels en hauteur comme en largeur.

Vous pouvez QA quand vous voulez ( cc @artragis )

@SpaceFox
Copy link
Contributor

Notre largeur d'images standard n'est pas de 960px ? https://github.com/zestedesavoir/zds-site/blob/dev/zds/settings.py#L210

@SpaceFox
Copy link
Contributor

Et si tu as 30 secondes pour passer sur ces commentaires : https://landscape.io/diff/114341

@artragis
Copy link
Member

Vous pouvez QA quand vous voulez ( cc @artragis )

pour moi c'est toujours entre 13h et 14h.

@firm1
Copy link
Contributor Author

firm1 commented Mar 19, 2015

Notre largeur d'images standard n'est pas de 960px ?

Ah bien vu, je corrige ça en même temps que la remarque de landscape

@landscape-bot
Copy link

Code Health
Repository health increased by 0.03% when pulling f27ded3 on firm1:fix-1634 into 0708f8b on zestedesavoir:dev.

@artragis
Copy link
Member

ça passe pas :


Request Method:     POST
Request URL:    http://127.0.0.1:8000/tutoriels/validation/valid/
Django Version:     1.6.10
Exception Type:     KeyError
Exception Value:    

'width'

Exception Location:     /home/fdambrine/pause_midi/zds-site/zds/tutorial/views.py in resize_svg, line 3143
Python Executable:  /home/fdambrine/pause_midi/zds-site/zdsenv/bin/python
Python Version:     2.7.6
Python Path:    

['/home/fdambrine/pause_midi/zds-site',
 '/home/fdambrine/pause_midi/zds-site/zdsenv/lib/python2.7',
 '/home/fdambrine/pause_midi/zds-site/zdsenv/lib/python2.7/plat-x86_64-linux-gnu',
 '/home/fdambrine/pause_midi/zds-site/zdsenv/lib/python2.7/lib-tk',
 '/home/fdambrine/pause_midi/zds-site/zdsenv/lib/python2.7/lib-old',
 '/home/fdambrine/pause_midi/zds-site/zdsenv/lib/python2.7/lib-dynload',
 '/usr/lib/python2.7',
 '/usr/lib/python2.7/plat-x86_64-linux-gnu',
 '/usr/lib/python2.7/lib-tk',
 '/home/fdambrine/pause_midi/zds-site/zdsenv/local/lib/python2.7/site-packages',
 '/home/fdambrine/pause_midi/zds-site/zdsenv/local/lib/python2.7/site-packages/git/ext/gitdb',
 '/home/fdambrine/pause_midi/zds-site/zdsenv/local/lib/python2.7/site-packages/gitdb/ext/smmap']

Server time:    jeu, 19 Mar 2015 13:56:52 +0100

svg utilisé : http://www.html5rocks.com/static/demos/svgmobile_fundamentals/images/HTML5-logo-faded-gradient.svg

@pierre-24
Copy link
Member

Quel horible image. Et grosse tranche de rire sur le /home/fdambrine/pause_midi/ :p

Plus sérieusement, c'est parce que ce SVG ne déclare pas explicitement sa taille, mais a plutôt un attribut viewbox. Ça fait longtemps que j'ai plus tripoté du SVG, mais il me semble que c'est correct aussi.

@pierre-24
Copy link
Member

Voir à ce sujet la spécification W3C qui précise que si non spécifié, il faut assumer "100%", whatever that means.

Voir aussi à ce sujet la spécification de viewBox, toujours du W3C, qui dit qu'on ne fait pas n'importe quoi avec ça (entre autre qu'il faut faire gaffe à la valeur de preserveAspectRatio).

@artragis
Copy link
Member

Quelle horrible image.

J'ai regardé sur google gigantic svg image le troisième ou quatrième lien m'a donné ça.

(voir à ce sujet la spécification W3C qui précise que si non spécifié, il faut assumer "100%", whatever that means).

donc pour toi firm1, le fix de ce bug devrait se résumer à

try:
    width = float(svg.attrib["width"])
except KeyError:
    width = 950 # en fait le param du settings.py

Et grosse tranche de rire sur le /home/fdambrine/pause_midi/ :p

J'avais prévenu "entre 13h et 14h"...

@landscape-bot
Copy link

Code Health
Repository health increased by 0.03% when pulling 7275491 on firm1:fix-1634 into 0708f8b on zestedesavoir:dev.

@firm1
Copy link
Contributor Author

firm1 commented Mar 19, 2015

Merci @artragis pour la QA et pour le correctif du coup, j'en ai profité pour rajouter ton image dans les tests. Faut croire que je n'ai pas lu toute la spec du format svg, j'aurai juré que les tailles étaient obligatoires :)

@coveralls
Copy link

Coverage Status

Coverage decreased (-31.29%) to 47.16% when pulling 7275491 on firm1:fix-1634 into 0708f8b on zestedesavoir:dev.

@pierre-24
Copy link
Member

Waw. Pourquoi coveralls est pas content ? :o

@firm1
Copy link
Contributor Author

firm1 commented Mar 19, 2015

Waw. Pourquoi coveralls est pas content ?

Quelque chose me dit que coveralls a un peu buggué (il n'a pas du recevoir les taux de couverture des autres tests) car travis est très content lui.


def resize_svg(source, max_size=960):

max_size = int(settings.THUMBNAIL_ALIASES[""]["content"]["size"][0])
Copy link
Member

Choose a reason for hiding this comment

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

Pourquoi une clef vide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A cause de Easy_thumbnails qui dit :

Use the target '' for project-wide aliases

@landscape-bot
Copy link

Code Health
Repository health increased by 0.02% when pulling 7275491 on firm1:fix-1634 into c827d7e on zestedesavoir:dev.

@artragis
Copy link
Member

QA réalisée, ça marche pour moi !

@firm1
Copy link
Contributor Author

firm1 commented Mar 23, 2015

Merci @artragis pour la QA.

@SpaceFox
Copy link
Contributor

SpaceFox commented Mar 6, 2016

@firm1 cette fonctionnalité semble être la seule à utiliser lxml mais ne l'ajoute pas dans les requirements (elle devait y être avant)

  1. Sais-tu à quoi lxml peut servir d'autre ?
  2. Sais-tu si on peut la remplacer par quelque chose qui ne met pas 10 minutes à être compilé à chaque fois qu'on tente de l'installer / la mettre à jour ?

@artragis
Copy link
Member

artragis commented Mar 6, 2016

Il me semble que LXML nous sert à parser les tutos pour les enregistrer
dans l'index solar.

Le 06/03/2016 01:29, SpaceFox a écrit :

@firm1 https://github.com/firm1 cette fonctionnalité semble être la
seule à utiliser |lxml| mais ne l'ajoute pas dans les requirements
(elle devait y être avant)

  1. Sais-tu à quoi |lxml| peut servir d'autre ?
  2. Sais-tu si on peut la remplacer par quelque chose qui ne met pas
    10 minutes à être compilé à chaque fois qu'on tente de l'installer ?


Reply to this email directly or view it on GitHub
#2430 (comment).

@firm1
Copy link
Contributor Author

firm1 commented Mar 6, 2016

Sais-tu à quoi lxml peut servir d'autre ?

Elle servait aussi a parser le format .tuto a l'époque. Mais je pense
qu'elle a d'autres liens

Sais-tu si on peut la remplacer par quelque chose qui ne met pas 10
minutes à être compilé à chaque fois qu'on tente de l'installer ?

Malheureusement c'est le seul concurrent sur le marché. Ce pendant il
faudrait voir les options d'install de la lib pour reduire le temps
d'install.

Le dim. 6 mars 2016 10:05, artragis notifications@github.com a écrit :

Il me semble que LXML nous sert à parser les tutos pour les enregistrer
dans l'index solar.

Le 06/03/2016 01:29, SpaceFox a écrit :

@firm1 https://github.com/firm1 cette fonctionnalité semble être la
seule à utiliser |lxml| mais ne l'ajoute pas dans les requirements
(elle devait y être avant)

  1. Sais-tu à quoi |lxml| peut servir d'autre ?
  2. Sais-tu si on peut la remplacer par quelque chose qui ne met pas
    10 minutes à être compilé à chaque fois qu'on tente de l'installer ?


Reply to this email directly or view it on GitHub
<
#2430 (comment)
.


Reply to this email directly or view it on GitHub
#2430 (comment)
.

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.

None yet

8 participants