Revue de code entre la r3506 et la r3534
J'ai pris un peu de temps pour regarder le résultat des derniers commits. Matthieu, je pense que la plupart de ces remarques sont pour toi. N'hésites pas à me demander si tu ne comprends pas certaines remarques et/ou à aller demander à un lutin. Essayes de voir avec Tony ou Éric pour configurer ton IDE afin d'éviter les imports du genre : +import javax.swing.*; +import java.awt.*; "when *touch* enter pressed" ... I'm pretty sure you ment *key*, right ? + if(selectedRow != table.getRowCount()){ + table.setRowSelectionInterval(table.getSelectedRow(), table.getSelectedRow()); + } else { + table.setRowSelectionInterval(table.getSelectedRow()-1, table.getSelectedRow()-1); + } Tu peux économiser au moins 3 table.getSelectedRow() au visuel, et 1 en runtime. Des logs de ce genre devraient plutôt : - être en "debug" ; - être en anglais. + if (log.isInfoEnabled()) { + log.info("Test sur JXDatePicker"); + } Tu as encore des soucis d'indentation. Cf FinancialTransactionTable#editCellAt(int,int,EventObject) Est-ce que la TVA ne devrait pas être dans l'AccountingRule en vigueur ? FinancialTransactionViewHandler#addEntry + //Actual (2012) tva percentage : 19,6% + BigDecimal tvaPercentAdd = new BigDecimal(0.196); Sur une méthode publique, essayes de plus respecter un format de documentation type javadoc : + //to reinitialize model attribute + public void resetAttribute(){ Sur cette même méthode (UnlettringSelectionModel#resetAttribute), je pense que tu as un gros bug, et que tu supprimes une ligne sur 2 (les lignes paires en l'occurrence) : for (int i = 0; i < selectedRows.size(); i ++){ selectedRows.remove(i); } Imagines que tu as une liste de 6 éléments : A B C D E F Tu supprimes l'élément à l'index 0 (A). Tous les éléments alors APRÈS A sont donc "déplacés" vers la gauche. B a alors l'index 0, C=1, D=2, ... B C D E F Tu incrémentes ton index : i=1. Tu supprimes l'élément à l'index 1 (C). B D E F Tu incrémentes ton index : i=2. Tu supprimes l'élément à l'index 2 (E). B D F Arrêt de la boucle car ta as attend le .size() = 2. Tu as donc supprimé tous les éléments pairs de ta liste :) Le plus simple aurait été d'utiliser une méthode pour vider la liste : selectedRows.clear(); -> Même problème dans LetteringSelectionModel#resetAttribute D'ailleurs, je vais peut-être dire une connerie, mais ces 2 classes ont l'air très similaire, est-ce qu'il n'aurait pas été possible de les mutualiser ? Bon courage, Arnaud
Sur une méthode publique, essayes de plus respecter un format de documentation type javadoc : + //to reinitialize model attribute + public void resetAttribute(){
Sur cette même méthode (UnlettringSelectionModel#resetAttribute), je pense que tu as un gros bug, et que tu supprimes une ligne sur 2 (les lignes paires en l'occurrence) : for (int i = 0; i < selectedRows.size(); i ++){ selectedRows.remove(i); }
Imagines que tu as une liste de 6 éléments : A B C D E F Tu supprimes l'élément à l'index 0 (A). Tous les éléments alors APRÈS A sont donc "déplacés" vers la gauche. B a alors l'index 0, C=1, D=2, ... B C D E F
Tu incrémentes ton index : i=1. Tu supprimes l'élément à l'index 1 (C). B D E F
Tu incrémentes ton index : i=2. Tu supprimes l'élément à l'index 2 (E). B D F
Arrêt de la boucle car ta as attend le .size() = 2.
Tu as donc supprimé tous les éléments pairs de ta liste :)
Le plus simple aurait été d'utiliser une méthode pour vider la liste : selectedRows.clear();
-> Même problème dans LetteringSelectionModel#resetAttribute
J'ai fais une remarque là dessus hier, qui rejoint ce code. "selectedRows" est en fait un 2eme cache qui duplique celui de swing est qui n'est pas réellement cohérent avec la sélection réelle de la table. D'où les incohérence d'interface. Le code qui gère ça me fait peur. Surcharge de plusieurs méthodes du SelectionModel (method add/set/clear...) qui met a jour le cache. Le tout avec un plus 4 modèles pour une seule table (un réel qui délègue aux 3 autres suivant une option). Alors qu'un simple <ListSelectionListener OnValueChanged="updateSolde()" /> serait tellement plus simple est toujours cohérent avec la sélection de la table. La surcharge des SelectionModel a du sens seulement pour l'autoselection des lignes (lignes du même lettrages...), mais pas pour la mise à jour des champs (crédit/débit/champ). -- Éric Chatellier
Le 18/07/2012 10:08, Eric Chatellier a écrit :
Sur une méthode publique, essayes de plus respecter un format de documentation type javadoc : + //to reinitialize model attribute + public void resetAttribute(){
Sur cette même méthode (UnlettringSelectionModel#resetAttribute), je pense que tu as un gros bug, et que tu supprimes une ligne sur 2 (les lignes paires en l'occurrence) : for (int i = 0; i < selectedRows.size(); i ++){ selectedRows.remove(i); }
Imagines que tu as une liste de 6 éléments : A B C D E F Tu supprimes l'élément à l'index 0 (A). Tous les éléments alors APRÈS A sont donc "déplacés" vers la gauche. B a alors l'index 0, C=1, D=2, ... B C D E F
Tu incrémentes ton index : i=1. Tu supprimes l'élément à l'index 1 (C). B D E F
Tu incrémentes ton index : i=2. Tu supprimes l'élément à l'index 2 (E). B D F
Arrêt de la boucle car ta as attend le .size() = 2.
Tu as donc supprimé tous les éléments pairs de ta liste :)
Le plus simple aurait été d'utiliser une méthode pour vider la liste : selectedRows.clear();
-> Même problème dans LetteringSelectionModel#resetAttribute
J'ai fais une remarque là dessus hier, qui rejoint ce code. "selectedRows" est en fait un 2eme cache qui duplique celui de swing est qui n'est pas réellement cohérent avec la sélection réelle de la table. D'où les incohérence d'interface.
J'ai rien compris... Qu'est ce c'est le cache ?
Le code qui gère ça me fait peur. Surcharge de plusieurs méthodes du SelectionModel (method add/set/clear...) qui met a jour le cache.
Ouaip, dans le cas ou l'on ne peux rein déselectionner ça ce tente non ? Mais je ne capte toujours pas ce qu'est un cache...
Le tout avec un plus 4 modèles pour une seule table (un réel qui délègue aux 3 autres suivant une option).
C'est quoi 4 modèles ? Si tu parle du modèle de sélection, il change selon de le mode ou tu es : - affichage des "lettré" dois sélectionner les autres lettrés - affichage des "non lettré" qui est un simple DefaultSelectionModel - affichage des 2 qui délèguent au 2 modèles de sélection, je trouve ça naturel non ? Plus un modèle de donnée, et le compte est bon ?
Alors qu'un simple <ListSelectionListener OnValueChanged="updateSolde()" /> serait tellement plus simple est toujours cohérent avec la sélection de la table.
Rien à voir avec les soldes dans ce que tu parle plus haut ? Ici ok, c'est ce qui devrait être fait. Par contre : <ListSelectionModelonValueChanged="updateSelectedRow()"/> ne fonctionne pas, hein Eric !
La surcharge des SelectionModel a du sens seulement pour l'autoselection des lignes (lignes du même lettrages...), mais pas pour la mise à jour des champs (crédit/débit/champ).
Là on est d'accord. Sylvain
Le 18/07/2012 10:08, Eric Chatellier a écrit :
Sur une méthode publique, essayes de plus respecter un format de documentation type javadoc : + //to reinitialize model attribute + public void resetAttribute(){
Sur cette même méthode (UnlettringSelectionModel#resetAttribute), je pense que tu as un gros bug, et que tu supprimes une ligne sur 2 (les lignes paires en l'occurrence) : for (int i = 0; i < selectedRows.size(); i ++){ selectedRows.remove(i); }
Imagines que tu as une liste de 6 éléments : A B C D E F Tu supprimes l'élément à l'index 0 (A). Tous les éléments alors APRÈS A sont donc "déplacés" vers la gauche. B a alors l'index 0, C=1, D=2, ... B C D E F
Tu incrémentes ton index : i=1. Tu supprimes l'élément à l'index 1 (C). B D E F
Tu incrémentes ton index : i=2. Tu supprimes l'élément à l'index 2 (E). B D F
Arrêt de la boucle car ta as attend le .size() = 2.
Tu as donc supprimé tous les éléments pairs de ta liste :)
Le plus simple aurait été d'utiliser une méthode pour vider la liste : selectedRows.clear();
-> Même problème dans LetteringSelectionModel#resetAttribute J'ai fais une remarque là dessus hier, qui rejoint ce code. "selectedRows" est en fait un 2eme cache qui duplique celui de swing est qui n'est pas réellement cohérent avec la sélection réelle de la table. D'où les incohérence d'interface.
Le code qui gère ça me fait peur. Surcharge de plusieurs méthodes du SelectionModel (method add/set/clear...) qui met a jour le cache.
Le tout avec un plus 4 modèles pour une seule table (un réel qui délègue aux 3 autres suivant une option).
Alors qu'un simple <ListSelectionListener OnValueChanged="updateSolde()" /> serait tellement plus simple est toujours cohérent avec la sélection de la table.
La surcharge des SelectionModel a du sens seulement pour l'autoselection des lignes (lignes du même lettrages...), mais pas pour la mise à jour des champs (crédit/débit/champ).
Merci pour toutes ses remarques. Juste quelques petites précisions, en ce qui concerne les modèles : - Pour la sélection de lignes, il y en a effectivement un avec une délégation, mais sur deux DefaultListSelectionModel; - Le calcul des champs débit / crédit / solde s'effectue via un autre modèle que les deux délégués; Et je vais en effet supprimer cette liste inutile... Puisqu'on se trouve dans des DefaultListSelectionModel.
participants (4)
-
Arnaud Thimel -
Eric Chatellier -
Letellier Sylvain -
mallon