[RFC][IcedTeaWeb] Lexicographical sorting toggle for certificates in IcedTeaWebSettings

[Jiri Vanek]

On 05/16/2012 08:14 PM, Adam Domurad wrote:
> Here's a small patch for review that adds a toggle that enables lexicographical sorting (java default for Strings) by the columns of the IcedTeaWebSettings certificate viewer.
Hi!
Thanx for lookig into this!

One fundamental question - what is motivation for this? When I check itweb-setting certificate 
panel, I see pretty good working stuff except few details. But I don't think sorting  will help to 
orient in this table.

What will be better imho is to enable sorting buttons on table (including no sorting as it is no, 
because when I add certificate (accidentally eg) I expect it last or first and not sorted somewhere.
Also some kind of filtering input  text field(s) will be very nice.

If you will agree with me that little bit more complex change will be much more useful then (maybe 
redundant) sorting, then it it will be definitely worthy to get rid of DefaultTableModel (which I 
consider as one of most dangerous things in swing).

Also it will be very worthy to cover this class by unittests. (It is always excellence opportunity 
to cover more code with any changeset).

>
> Thanks, Adam.
>
> 2012-05-16  Adam Domurad<adomurad at redhat.com>
>
> 	Introduced two functions to reduce duplicated code.
>          Added lexicographical ordering of certificates for IcedTeaWebSettings.
>
> 	* netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java:
> 	Introduced functions void initializeCertificateTable(JTable certtable)
>          and JScrollPane createCertificateScrollPane(JTable certtable) to
>          encapsulate duplicate code.
>          Added a lexicographical sort (applied in initializeCertificateTable) to
>          the user and system tables for the certificate viewer.
>

IIRC the empty line in changelog is not recommended, but I'm the last one to criticize changelogs :)
>
> certificate-lexicographical-sorting.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java b/netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java
> --- a/netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java
> +++ b/netx/net/sourceforge/jnlp/security/viewer/CertificatePane.java
> @@ -54,6 +54,7 @@ import java.security.KeyStore;
>   import java.security.cert.Certificate;
>   import java.security.cert.X509Certificate;
>   import java.util.ArrayList;
> +import java.util.Comparator;
>   import java.util.Enumeration;
>   import java.util.List;
>
> @@ -73,6 +74,7 @@ import javax.swing.JTable;
>   import javax.swing.event.ChangeEvent;
>   import javax.swing.event.ChangeListener;
>   import javax.swing.table.DefaultTableModel;
> +import javax.swing.table.TableRowSorter;
>
>   import net.sourceforge.jnlp.security.CertificateUtils;
>   import net.sourceforge.jnlp.security.KeyStores;
> @@ -154,6 +156,41 @@ public class CertificatePane extends JPa
>           }
>       }
>
> +    /**
> +     * Initialize the certificate table (user or system).
> +     */
> +    private void initializeCertificateTable(JTable certtable){
> +        DefaultTableModel tableModel = new DefaultTableModel(issuedToAndBy, columnNames);

I would really like to avoid  DefaultTableModel tableModel = new DefaultTableModel ....
If you will insists on DefaultTableModel, then please try to keep by interface declaration.

TableModel whatever= new DefaultTableModel....


> +        certtable.setModel(tableModel);
> +        certtable.getTableHeader().setReorderingAllowed(false);
> +        certtable.setFillsViewportHeight(true);
> +
> +        TableRowSorter<DefaultTableModel>  tableSorter = new TableRowSorter<DefaultTableModel>(tableModel);
> +
> +        Comparator<String>  lexicographicalComparator = new Comparator<String>() {
> +            @Override
> +            public int compare(String o1, String o2) {
> +                return o1.compareTo(o2);
> +            }
> +        };
> +
> +        tableSorter.setComparator(0, lexicographicalComparator);
> +        tableSorter.setComparator(1, lexicographicalComparator);
> +
> +        certtable.setRowSorter(tableSorter);
> +    }

I believe that custom TableModel will be better place for sorting/filtering, but this point will be 
worthy only if we agree on little bit more complex change.

> +
> +    /**
> +     * Create a scroll pane for a certificate table (user or system).
> +     */
> +    private JScrollPane createCertificateScrollPane(JTable certtable){
> +        JScrollPane pane = new JScrollPane(certtable);
> +        pane.setPreferredSize(TABLE_DIMENSION);
> +        pane.setSize(TABLE_DIMENSION);
> +        pane.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
> +        return pane;
> +    }
> +
>       //create the GUI here.
>       protected void addComponents() {
>
> @@ -173,24 +210,12 @@ public class CertificatePane extends JPa
>           JPanel tablePanel = new JPanel(new BorderLayout());
>
>           // User Table
> -        DefaultTableModel userTableModel = new DefaultTableModel(issuedToAndBy, columnNames);
> -        userTable.setModel(userTableModel);
> -        userTable.getTableHeader().setReorderingAllowed(false);
> -        userTable.setFillsViewportHeight(true);
> -        JScrollPane userTablePane = new JScrollPane(userTable);
> -        userTablePane.setPreferredSize(TABLE_DIMENSION);
> -        userTablePane.setSize(TABLE_DIMENSION);
> -        userTablePane.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
> +        initializeCertificateTable(userTable);
> +        JScrollPane userTablePane = createCertificateScrollPane(userTable);

+1!

>
>           // System Table
> -        DefaultTableModel systemTableModel = new DefaultTableModel(issuedToAndBy, columnNames);
> -        systemTable.setModel(systemTableModel);
> -        systemTable.getTableHeader().setReorderingAllowed(false);
> -        systemTable.setFillsViewportHeight(true);
> -        JScrollPane systemTablePane = new JScrollPane(systemTable);
> -        systemTablePane.setPreferredSize(TABLE_DIMENSION);
> -        systemTablePane.setSize(TABLE_DIMENSION);
> -        systemTablePane.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
> +        initializeCertificateTable(systemTable);
> +        JScrollPane systemTablePane = createCertificateScrollPane(systemTable);
>

+2!

>           tabbedPane = new JTabbedPane();
>           tabbedPane.addTab(R("CVUser"), userTablePane);
> @@ -289,12 +314,8 @@ public class CertificatePane extends JPa
>       private void repopulateTables() {
>           initializeKeyStore();
>           readKeyStore();
> -        DefaultTableModel tableModel = new DefaultTableModel(issuedToAndBy, columnNames);
> -
> -        userTable.setModel(tableModel);
> -
> -        tableModel = new DefaultTableModel(issuedToAndBy, columnNames);
> -        systemTable.setModel(tableModel);
> +        initializeCertificateTable(userTable);
> +        initializeCertificateTable(systemTable);

+4!

>       }
>
>       public void focusOnDefaultButton() {
>
Tahnx for getting rid of redundant code. If nothing else then this (and unittests? O:) Should 
definitely go in.

Best regards and thanx for patch
    J.