developer.jelix.org is not used any more and exists only for history. Post new tickets on the Github account.
developer.jelix.org n'est plus utilisée, et existe uniquement pour son historique. Postez les nouveaux tickets sur le compte github.

Opened 12 years ago

Closed 12 years ago

#720 closed enhancement (fixed)

Diff : patch for diff plugin + dilffhtml

Reported by: foxmask Owned by: foxmask
Priority: low Milestone: jelix 1.1
Component: jelix:utils Version: 1.1 beta 1
Severity: minor Keywords: diff
Cc: Blocked By:
Blocking: Documentation needed: no
Hosting Provider: Php version:

Description

this patch permits to display a diff in two way :

inline or side by side

so 3 files are modified :

1) defaultconfig.ini.php.dist with a new paramater :

; format : sidebyside or inline

diffFormatOutPut = sidebyside

2) the diff plugin

3) a new class in diffhtml TableDiffFormatter.

Attachments (5)

diff_avec_table.patch (4.3 KB) - added by foxmask 12 years ago.
the patch for the 3 modified files
diff_avec_table_plus_nouveau_parm_defaultconfig.patch (1.5 KB) - added by foxmask 12 years ago.
Patch de la fonction {diff} + ajout du parm diffFormatOutPut dans le fichier defaultconfig.ini.php.dist
difftableformatter.php (2.8 KB) - added by foxmask 12 years ago.
Class TableDiffFormatter?
diff_function.patch (7.0 KB) - added by foxmask 12 years ago.
function.diff.php patched to be able to display the Diff inline or sidebyside
plugin.diff.patch (8.0 KB) - added by foxmask 12 years ago.
Patch for Plugin Diff + HtmlTableDiffFormatter? class + CREDITS

Download all attachments as: .zip

Change History (24)

Changed 12 years ago by foxmask

the patch for the 3 modified files

comment:1 Changed 12 years ago by foxmask

to use diff ; then we could do

{diff $str1,$str2,$version1,$version2}

comment:2 Changed 12 years ago by laurentj

  • Component changed from jelix to jelix:utils
  • Milestone Jelix 1.1 beta 2 deleted
  • Priority changed from normal to low
  • review set to review-
  • Severity changed from major to minor
  • Type changed from task to enhancement
  • the TableDiffFormatter? class should be in a separate file, so we could update easier the diff lib.
  • to be compatible with the current signature, the new parameters of the diff plugin should be added at the end of the list of parameters.
  • the default values of this new parameters are not really coherent. Why "0" ? The default values should be an empty string, since it is use as a column title.
  • if both $version1 and $version2 are empty strings, the thead section should not be displayed.

comment:3 Changed 12 years ago by foxmask

goal using : {diff $str,$str2,$version1,$version2,$nodiffmsg), to make a diff between $str1 and $str2 and display in the Headline "version $version1 / version $version2"

1) ok i can make a seperate file 2) today we could use {diff } without giving the nodiffmsg parameter, if we put the $version1 and $version2 at the end of the signature, we will have to give all parameters of the diff function. 3) $version1 and $version2 are coherent with the source of the data where we get them. 4) $version1 and $version2 should not be empty.

So now you would prefer ?

1) in lib/jelix/plugins/tpl/html/function.diff.php {{function jtpl_function_html_diff($tpl, $str1,$str2,$nodiffmsg='Pas de différence',$version1=0,$version2=0) }}

2) line 202 of lib/diff/diffhtml.php (instead of the TableDiffFormatter? class) {{ require_once(dirname(FILE).'/difftableformatter.php'); }}

3) in the _block_header of TableDiffFormatter? : instead of {{

'<thead><tr><th colspan="2"> Version '.$this->version1.":</th>\n" .

'<th colspan="2"> Version '.$this->version2.":</th></tr></thead>\n";

}}

this : {{

'<thead><tr><th colspan="2">'.$this->version1.":</th>\n" .

'<th colspan="2">'.$this->version2.":</th></tr></thead>\n";

}}

Correct ?

comment:4 Changed 12 years ago by laurentj

$version1 and $version2 should not be empty.

I don't see why it should not be empty...

  1. no: at the end : ,$version1='',$version2=''
  2. no, as I said, don't touch the files of the diff library
  3. sorry, I don't see a difference here (please preview your comments and use the correct wiki syntax). You should add: if($this->version1 != '' || $this->version2 != '') {....}

comment:5 Changed 12 years ago by foxmask

3: sorry for my mistake between preview/submit.

2: ok i'll add

require_once(LIB_PATH.'diff/difftableformatter.php')

in function.diff.php

1: i thought $version1= and i wrote $version1=0 :D

Changed 12 years ago by foxmask

Patch de la fonction {diff} + ajout du parm diffFormatOutPut dans le fichier defaultconfig.ini.php.dist

Changed 12 years ago by foxmask

comment:6 Changed 12 years ago by foxmask

i've tested those files with the wiki module of jbugtracker and that went well ;)

comment:7 Changed 12 years ago by bastnic

Commite ça dans jbugtracker, on remontera ça après sur le trunk au maximum.

Ca évitera qu'on est à le mettre en place par nous-même.

Je regarderais ce que ça donne pour mes vcs.

comment:8 Changed 12 years ago by foxmask

c'est done on jbugtracker.

si ça convient pas n'hesitez pas à rollback à la revision 55.

comment:9 follow-ups: Changed 12 years ago by laurentj

@foxmask : merci de suivre correctement le processus de soumission de patch, en lisant la doc dans le wiki de developer.jelix.org

  1. fournir tout en un seul diff (fait un svn add sur les nouveaux fichiers
  2. mettre le flag review? pour demander une review
  3. met toi en propriétaire du ticket (accepted dans le status du ticket)

ajout du parm diffFormatOutPut dans le fichier defaultconfig.ini.php.dist

non, je ne suis pas d'accord, on peut ne pas vouloir dans une application afficher tout le temps un même type de diff. Enlève donc ça, et rajoute plutôt un paramètre au plugin diff.

D'ailleurs, ça serait mieux d'avoir :

function jtpl_function_html_diff($tpl, $str1,$str2, $options=array())

où $options est un tableau d'options pouvant contenir les élements 'nodiffmsg', 'version1', 'version2', 'type'. Et pour rester compatible avec l'existant, test si le paramètre $options est une chaîne ou un tableau, et si c'est une chaîne, traite le comme si c'etait $nodiffmsg.

Inclure aussi la classe du diff en fonction du type, et non pas systématiquement.

toujours review- donc.

@bastnic : c'est un peu dangereux ça. parce que si ça devient systématique, on va avoir un jelix patché dans tout les sens, et quand on va mettre à jour jelix, on risque de perdre des modifications. Vaut attendre que ce soit intégré en upstream. Surtout pour un patch comme ça, qui ne gène en rien l'avancement du projet jbugtracker.

comment:10 in reply to: ↑ 9 Changed 12 years ago by bastnic

@bastnic : c'est un peu dangereux ça. parce que si ça devient systématique, on va avoir un jelix patché dans tout les sens, et quand on va mettre à jour jelix, on risque de perdre des modifications. Vaut attendre que ce soit intégré en upstream. Surtout pour un patch comme ça, qui ne gène en rien l'avancement du projet jbugtracker.

Justement, je serais tenté de dire que vu que ça ne gène en rien l'avancement, ça fait une intégration en environnement concret. Dès que c'est validé, on met à jour jelix de jbugtracker. Ce qui m'intéresse uniquement c'est le résultat. J'en ai également besoin pour mes vcs, et je n'ai pas besoin d'avoir la version du trunk pour ça.

Mais tu as raison, c'était une demande par pure fainéantise. Je ne recommencerais plus :p.

comment:11 Changed 12 years ago by foxmask

donc je retire les modif sur le ficheir de config et function.diff.php et reprendrai avec ces recommandations.

comment:12 in reply to: ↑ 9 Changed 12 years ago by foxmask

  • Owner set to foxmask
  • Status changed from new to assigned

Replying to laurentj:

@foxmask : merci de suivre correctement le processus de soumission de patch, en lisant la doc dans le wiki de developer.jelix.org

ok je vais la relire.

  1. fournir tout en un seul diff (fait un svn add sur les nouveaux fichiers

ok je viens de le faire je l'attache dans la foulée.

  1. mettre le flag review? pour demander une review

ok, compris.

  1. met toi en propriétaire du ticket (accepted dans le status du ticket)

je n'ai pas saisi pourquoi je devais me mettre propriétaire du ticket. mais je fais comme tu le dis

Changed 12 years ago by foxmask

function.diff.php patched to be able to display the Diff inline or sidebyside

comment:13 Changed 12 years ago by foxmask

  • review changed from review- to review?

Voici.

dsl pour les multiples loupés.

cordialement

comment:14 follow-up: Changed 12 years ago by laurentj

je n'ai pas saisi pourquoi je devais me mettre propriétaire du ticket. mais je fais comme tu le dis

à partir du moment où tu proposes de t'occuper d'un ticket, tu dois te l'assigner, pour indiquer que tu bosses dessus. Ça évite à d'autres de faire le même boulot.

comment:15 in reply to: ↑ 14 Changed 12 years ago by foxmask

Replying to laurentj:

je n'ai pas saisi pourquoi je devais me mettre propriétaire du ticket. mais je fais comme tu le dis

à partir du moment où tu proposes de t'occuper d'un ticket, tu dois te l'assigner, pour indiquer que tu bosses dessus. Ça évite à d'autres de faire le même boulot.

ok, je croyais qu'il fallait etre developpeur sur le projet pour cela. (s'assigner un ticket)

merci pour l'éclaircissement.

comment:16 Changed 12 years ago by laurentj

  • Milestone set to jelix 1.1
  • review changed from review? to review-

Ok, that's better. But there are still a thing which annoy me : you completely replace the use of HtmlUnifiedDiffFormatter? by your new formatter. I don't want that.

The type option could be use to indicate the formater. I think it could have this following values :

Default value is unifieddiff, to keep compatibility with existent code. You should then include TableDiffFormater? or HtmlUnifiedDiffFormater? when needed.

You should also rename TableDiffFormater? to HtmlTableDiffFormater? (because there are other modifier only for text, so we need to differentiate them)

Don't use # for comments, only /* */ or .

Update also the lib/jelix/CREDITS file, and the comment header of the plugin, with your name etc.

Create your patch from the trunk directory, not from the lib directory.

If you provide your new patch quickly, we will integrate it into Jelix 1.1 :-)

Changed 12 years ago by foxmask

Patch for Plugin Diff + HtmlTableDiffFormatter? class + CREDITS

comment:17 Changed 12 years ago by foxmask

  • review changed from review- to review+

here it is.

comment:18 Changed 12 years ago by laurentj

it's ok. Thanks for this patch..

The commiter should also include the new file in the manifest file.

comment:19 Changed 12 years ago by laurentj

  • Resolution set to fixed
  • Status changed from assigned to closed

include in the trunk. r1178.

Note: See TracTickets for help on using tickets.