[rfc][icedtea-web] Certificate viewer cleanup
Jacob Wisor
gitne at gmx.de
Fri Sep 6 12:16:00 PDT 2013
Hello there!
Jiri Vanek wrote:
> On 08/29/2013 05:18 PM, Jacob Wisor wrote:
>> "Jacob Wisor"<gitne at xxxxxxxxxxxx> wrote:
>>>> "Jacob Wisor"<gitne at xxxxxxxxxxxx> wrote:
>>>>>> * Replaced old insufficient certificate viewer's action buttons handling with proper handling
>>>>>> * Made System-level certificates managable if current user has write access to a KeyStore (usually only root or admin)
>>>>>> * Added informative tool tip to disabled buttons when certificates are unmanagable
>>>>>> * Made certificate table uneditable
>>>>>> * Made certificate info table uneditable
>>>>>> * Made certificate info's descriptive JTextField have the same font size as its info table for better readability
>>>>>> * Added certificate file filters with Windows/Linux preference
>>>>>> * Added auto-suggested certificate file names when exporting
>>>>>> * Added automatically generated mnemonics
>>>>>> * Made labels link to corresponding JComponents
>>>>>> * Added a border to certificate type label for better readability
>>>>>> * Uniformed border sizes
>>>>>> * Added removing of multiple certificates at once
>>>>>> * Turned some only once occuring fields into static
>>>>>> * Added proper exception handling when setting system look & feel, hence removed superfluous "throws Exception" from method signatures
>>>>>>
>>>>>> Happy reviewing!
>>>>>>
>>>>>> Jacob
>>>> Pasting in this time.
>>>>
>>>> [...]
>> I have realized I have missed to add a new file to the patch. I have updated the copyright headers too.
>>
>
> Nice job, thank you! I have finally read it through!
>
>
> As drawback there is bunch of exceptions:
> java.io.EOFException
> at java.io.DataInputStream.readInt(DataInputStream.java:392)
> at sun.security.provider.JavaKeyStore.engineLoad(JavaKeyStore.java:645)
> at sun.security.provider.JavaKeyStore$JKS.engineLoad(JavaKeyStore.java:55)
> at java.security.KeyStore.load(KeyStore.java:1214)
> at net.sourceforge.jnlp.security.KeyStores.createKeyStoreFromFile(KeyStores.java:416)
> at net.sourceforge.jnlp.security.KeyStores.getKeyStore(KeyStores.java:140)
> at net.sourceforge.jnlp.security.KeyStores.getKeyStore(KeyStores.java:119)
> at net.sourceforge.jnlp.security.KeyStores.getClientKeyStores(KeyStores.java:235)
> at net.sourceforge.jnlp.security.VariableX509TrustManager.<init>(VariableX509TrustManager.java:145)
> at
> net.sourceforge.jnlp.security.VariableX509TrustManager.getInstance(VariableX509TrustManager.java:439)
> at
> net.sourceforge.jnlp.security.VariableX509TrustManagerJDK7.<init>(VariableX509TrustManagerJDK7.java:53)
> at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
> at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:57)
> at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
> at java.lang.reflect.Constructor.newInstance(Constructor.java:526)
> at net.sourceforge.jnlp.runtime.JNLPRuntime.getSSLSocketTrustManager(JNLPRuntime.java:319)
> at net.sourceforge.jnlp.runtime.JNLPRuntime.initialize(JNLPRuntime.java:256)
> at
> net.sourceforge.jnlp.security.viewer.CertificateViewer.showCertificateViewer(CertificateViewer.java:103)
> at net.sourceforge.jnlp.security.viewer.CertificateViewer.main(CertificateViewer.java:126)
> at net.sourceforge.jnlp.runtime.Boot.main(Boot.java:135)
Could you please elaborate on when this happens and how to reproduce it?
> and (more important)
>
> java.lang.NullPointerException
> at net.sourceforge.jnlp.security.viewer.CertificatePane.readKeyStore(CertificatePane.java:316)
> at net.sourceforge.jnlp.security.viewer.CertificatePane.repopulateTables(CertificatePane.java:344)
> at net.sourceforge.jnlp.security.viewer.CertificatePane.access$1200(CertificatePane.java:90)
> at
> net.sourceforge.jnlp.security.viewer.CertificatePane$CertificateTypeListener.actionPerformed(CertificatePane.java:408)
> at javax.swing.JComboBox.fireActionEvent(JComboBox.java:1260)
> at javax.swing.JComboBox.setSelectedItem(JComboBox.java:588)
> at javax.swing.JComboBox.setSelectedIndex(JComboBox.java:624)
> at javax.swing.plaf.basic.BasicComboPopup$Handler.mouseReleased(BasicComboPopup.java:835)
> at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:290)
> at java.awt.Component.processMouseEvent(Component.java:6505)
> at javax.swing.JComponent.processMouseEvent(JComponent.java:3312)
> at javax.swing.plaf.basic.BasicComboPopup$1.processMouseEvent(BasicComboPopup.java:499)
> at java.awt.Component.processEvent(Component.java:6270)
> at java.awt.Container.processEvent(Container.java:2229)
> at java.awt.Component.dispatchEventImpl(Component.java:4861)
> at java.awt.Container.dispatchEventImpl(Container.java:2287)
> at java.awt.Component.dispatchEvent(Component.java:4687)
> at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4832)
> at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4492)
> at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4422)
> at java.awt.Container.dispatchEventImpl(Container.java:2273)
> at java.awt.Window.dispatchEventImpl(Window.java:2719)
> at java.awt.Component.dispatchEvent(Component.java:4687)
> at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:735)
> at java.awt.EventQueue.access$200(EventQueue.java:103)
> at java.awt.EventQueue$3.run(EventQueue.java:694)
> at java.awt.EventQueue$3.run(EventQueue.java:692)
> at java.security.AccessController.doPrivileged(Native Method)
> at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
> at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:87)
> at java.awt.EventQueue$4.run(EventQueue.java:708)
> at java.awt.EventQueue$4.run(EventQueue.java:706)
> at java.security.AccessController.doPrivileged(Native Method)
> at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
> at java.awt.EventQueue.dispatchEvent(EventQueue.java:705)
> at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:242)
> at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:161)
> at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:154)
> at java.awt.WaitDispatchSupport$2.run(WaitDispatchSupport.java:182)
> at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:251)
> at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:733)
> at java.awt.EventQueue.access$200(EventQueue.java:103)
> at java.awt.EventQueue$3.run(EventQueue.java:694)
> at java.awt.EventQueue$3.run(EventQueue.java:692)
> at java.security.AccessController.doPrivileged(Native Method)
> at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
> at java.awt.EventQueue.dispatchEvent(EventQueue.java:703)
> at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:242)
> at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:161)
> at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:150)
> at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:146)
> at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:138)
> at java.awt.EventDispatchThread.run(EventDispatchThread.java:91)
>
>
> Both are quite serious as they stop the redrawing of table.
Same as above, please elaborate on how to reproduce.
> Two ideas - when digging in this, maybe to switch ~/icedtea-web-image/bin/javaws -viewer from
> Dialog to Frame would be nice - at least it will be accesible via alt+tab and maximiseable and so on...
I am not sure whether altering the stand-alone certificate viewer (javaws
-viewer) from JDialog to JFrame is a good idea. On Windows when running with
Oracle JRE the dialog is accessible via Alt+Tab indeed, but it does not show up
on the taskbar. I do not recall exactly, so please take my statement with a
grain of salt, this is also Microsoft's intended behavior by design for system
control dialogs in Windows versions prior to Vista.
Regardless of that, I would not advise to move to JFrame because from a
usability and UI style-guide point of view this stand-alone certificate viewer
is a true dialog (system control dialog) and is supposed to be and behave as
such. So, maximizing, minimizing, or resizing is probably contradicting the
primary meaning of a quick utility or system control dialog. Hence this concept
or UI style should be kept. Unless we were up to making some new UI for Window's
Modern UI or GNOME 3 UI etc. then it would perhaps make sense to move to some
other base UI component class. But, the Java Swing Framework has been
specifically intended and developed as a classic windowing API. There is no
other new UI framework in J2SE yet, so we are stuck with that for now and should
also stick to its style-guide.
Nevertheless, I would accept to have a custom Dialog class extending JFrame made
which is going to shows up on the taskbar specifically and not be resizeable
etc. as usual dialogs.
> - the tool tip text are pretty good {except {0} error) but are hard to be seen. Most people do not
> hover over disabled button. It would be nice to have them as label on visible(if useful)/invisible
> paanel eg. between buttons and table.
I have been thinking about it too and came to the conclusion that probably the
best solution would be to either have a small information icon display on the
affected buttons, so that users are going to get some hint as to hover over them
or add a status bar at the bottom of the dialog. Yet, both solutions pose some
problems. Adding an icon would actually require adding an icon resource which is
probably not native to the windowing system and may look oddly on many windowing
systems. Whereas a status bar may be to short or would become unproportionally
big to present those explanatory texts/messages (which could of course be
shortened).
Anyway, I think it is best to keep the UI controls and their behavior as generic
as possible. So, no dynamic hiding or showing of controls. Disabling and
enabling buttons dynamically has been originally intended to handle the kind of
situations we face here, namely making all possible actions or operations
constantly visible to the user, and yet making him or her unintrusively aware of
some actions not being available under certain circumstances. I would rather
leave it as is, unless you can present me some adequate information icon (or
perhaps stop icon?).
> Few nits in inline:
>
>
>> CertficateViewer_cleanup.patch
>>
>>
>> diff -r 420d72e5cee7 netx/net/sourceforge/jnlp/resources/Messages.properties
>> --- a/netx/net/sourceforge/jnlp/resources/Messages.properties
>> +++ b/netx/net/sourceforge/jnlp/resources/Messages.properties
>> @@ -275,6 +276,9 @@
>> SValidity=Validity
>>
>> # Certificate Viewer
>> +CVCannotImport=You can not import certificates of this type because {0}
>
> Those {0} are never avaluated.
>
>> +CVCannotRemove=You can not remove certificates of this type because {0}
>
> The CVCannotImport/CVCannotRemove seems to me randomly switched with CVCause.
> CVCannotImport/CVCannotRemove - are shown when I change certificate type, whether
> CVCause is shown when user/system tab.
>
> I think both are valid at all time, so their concation would be nice.
>> +CVCause=<ul><li>you are not logged in as an administrator,</li><li>you have no file system write permission to the keystore file,</li><li>the keystore file is inaccesible via network,</il><li>the keystore file is write protected by file system attributes,</li><li>you do not have a <code>FilePermission("write")</code>, or</li><li>the keystore file is otherwise physically inaccesible.</li></ul>
>> CVCertificateViewer=Certificates
>> CVCertificateType=Certificate Type
>> CVDetails=Details
I have made those messages evaluate correctly, but Mercurial has generated an
incorrect diff for some reason... :-o
So, as it seems what you were reviewing was not my correct final version. I
assume that this is also the reason for those runtime exceptions you got.
Sorry...
While writing this email response, I have checked it manually again and it seems
I have found a bug in Mercurial because although the code in the file is correct
the diff to tip does not account for that. I will have to dig deeper into that
before resubmitting this patch for rfc.
>> @@ -302,6 +303,37 @@
>> }
>>
>> /**
>> + * Checks whether the specified system-level or user-level {@link KeyStore}
>> + * is writable.
>> + * @param level
>> + * @return {@code true} if the specified {@code KeyStore} is writable,
>> + * {@code false} otherwise
>> + * @see KeyStores.Level
>> + */
>> + public static final boolean isKeyStoreWriteable(KeyStores.Level level, KeyStores.Type type) {
>> + if (level == null) throw new IllegalArgumentException("level == " + level);
>> + if (type == null) throw new IllegalArgumentException("type == " + type);
>> +
>> + FileOutputStream fileOutputStream = null;
>> +
>> + try {
>> + fileOutputStream = new FileOutputStream(KeyStores.getKeyStoreLocation(level, type), true);
>> + } catch (final FileNotFoundException fileNotFoundException) {
>> + fileOutputStream = null;
>> + } catch (final SecurityException securityException) {
>> + fileOutputStream = null;
>> + } finally {
>> + if (fileOutputStream != null) try {
>> + fileOutputStream.close();
>> + } catch (final IOException ioException) {
>> + // Don't bother, fileOutputStream is probably closed anyway
>> + }
>> + }
>> +
>> + return fileOutputStream != null;
>
>
> Its really enough? I did not know this "hack". Interesting :)
It's no hack. What's a hack about it? Just checking whether a file can be opened
for writing. Of course, this only works assuming that the semantics of
FileOutputStream are identical across platforms. But then on the other hand,
what would be Java if one was not to assume it, yes even expect it?
FileOutputStream should even conceal perhaps file system driver or OS specific
semantics.
Btw, with regards to semantics across platforms those adding File.canRead(),
File.canWrite(), and File.canExecute() have f*cked up hard because those methods
are utterly OS specific. They should have provided a more generic file system
access permissions model instead. It is totally contradicting Java's idea, but
that's another story to be told...
> Please add unittest for anyway.
I assumed you would say that. ;-)
>> + }
>> +
> ...snip...
>> * "Issued To" and "Issued By" string pairs for certs.
>> */
>> private String[][] issuedToAndBy = null;
>> - private final String[] columnNames = { R("CVIssuedTo"), R("CVIssuedBy") };
>> + private final static String[] columnNames = { R("CVIssuedTo"), R("CVIssuedBy") };
>> + private final static String CER_EXTENSION = "cer",
>> + CRT_EXTENSION = "crt",
>> + PFX_EXTENSION = "pfx",
>> + P12_EXTENSION = "p12",
>> + PKCS12 = "PKCS #12";
>
> I must say I'm quite confused by this extensions. What are they needed for now? (as they were not
> needed before)
I have put those Strings constants in because they are used with file filters
where they are functional string parameters as well as simple user-friendly
text. If one were to change those (for what ever reason) the appropriate
functional parameters and user-friendly texts will change accordingly because
both are based on the same "original" strings.
If you insist I can put those into the code as literals, but as I said, this
goes with losing the aforementioned convenience and coding safety.
>> - private final CertificateType[] certificateTypes = new CertificateType[] {
>> + private final static CertificateType[] certificateTypes = new CertificateType[] {
> ...
>>
>> userTable = new JTable(null);
>> systemTable = new JTable(null);
>> - disableForSystem = new ArrayList<JComponent>();
>
> Well I'm not sure about this expansion of array into four separated variables.
> Althoug disableForSystem will become really irrelevant name.
>
> I would go with JComponent[] button = new JComponent{
>> + this.importButton = new JButton();
>> + this.exportButton = new JButton();
>> + this.removeButton = new JButton();
>> + this.detailButton = new JButton();
> }
>
> As I believe there are few cases where iteration will win over duplicate code.
Sounds almost convincing, but why does this kind of helper array have to live
and take up memory space during CertificatePane's entire lifetime when it is
currently used by one method only, namely addComponents()? I would give into it
if addComponents() was called very often and thus creating many throw-away
helper arrays would become unacceptably costly. addComponents() is called only
once. So, I do not see any solid reason for doing this.
>>
>> addComponents();
>>
>> @@ -139,6 +167,9 @@
>> } else {
>> currentKeyStoreLevel = Level.SYSTEM;
>> }
>> + this.importButton.setEnabled(this.isKeyStoreWriteable =
>> + KeyStores.isKeyStoreWriteable(this.currentKeyStoreLevel,
>> + this.currentKeyStoreType));
>>
>> repopulateTables();
>> }
>> @@ -163,9 +194,11 @@
>> certificateTypePanel.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));
>>
>> JLabel certificateTypeLabel = new JLabel(R("CVCertificateType"));
>> + certificateTypeLabel.setBorder(BorderFactory.createEmptyBorder(0, 0, 0, 5));
>>
>> certificateTypeCombo = new JComboBox(certificateTypes);
>> certificateTypeCombo.addActionListener(new CertificateTypeListener());
>> + certificateTypeLabel.setLabelFor(this.certificateTypeCombo);
>>
>> certificateTypePanel.add(certificateTypeLabel, BorderLayout.LINE_START);
>> certificateTypePanel.add(certificateTypeCombo, BorderLayout.CENTER);
>> @@ -173,20 +206,24 @@
>> JPanel tablePanel = new JPanel(new BorderLayout());
>>
>> // User Table
>> - DefaultTableModel userTableModel = new DefaultTableModel(issuedToAndBy, columnNames);
>> - userTable.setModel(userTableModel);
>> + userTable.setModel(new NonEditableTableModel(issuedToAndBy, CertificatePane.columnNames));
>> userTable.getTableHeader().setReorderingAllowed(false);
>> userTable.setFillsViewportHeight(true);
>> + final CertificatePane.CertificateListSelectionListener certificateListSelectionListener;
>> + userTable.getSelectionModel().setSelectionMode(ListSelectionModel.MULTIPLE_INTERVAL_SELECTION);
>> + userTable.getSelectionModel().addListSelectionListener(certificateListSelectionListener =
>> + new CertificatePane.CertificateListSelectionListener());
>> JScrollPane userTablePane = new JScrollPane(userTable);
>> userTablePane.setPreferredSize(TABLE_DIMENSION);
>> userTablePane.setSize(TABLE_DIMENSION);
>> userTablePane.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
>>
>> // System Table
>> - DefaultTableModel systemTableModel = new DefaultTableModel(issuedToAndBy, columnNames);
>> - systemTable.setModel(systemTableModel);
>> + systemTable.setModel(new NonEditableTableModel(issuedToAndBy, CertificatePane.columnNames));
>> systemTable.getTableHeader().setReorderingAllowed(false);
>> systemTable.setFillsViewportHeight(true);
>> + systemTable.getSelectionModel().setSelectionMode(ListSelectionModel.MULTIPLE_INTERVAL_SELECTION);
>> + systemTable.getSelectionModel().addListSelectionListener(certificateListSelectionListener);
>> JScrollPane systemTablePane = new JScrollPane(systemTable);
>> systemTablePane.setPreferredSize(TABLE_DIMENSION);
>> systemTablePane.setSize(TABLE_DIMENSION);
>> @@ -199,35 +236,50 @@
>>
>> JPanel buttonPanel = new JPanel(new FlowLayout());
>>
>> - String[] buttonNames = { R("CVImport"), R("CVExport"), R("CVRemove"), R("CVDetails") };
>> - char[] buttonMnemonics = { KeyEvent.VK_I,
>> - KeyEvent.VK_E,
>> - KeyEvent.VK_M,
>> - KeyEvent.VK_D };
>> - ActionListener[] listeners = { new ImportButtonListener(),
>> - new ExportButtonListener(),
>> - new RemoveButtonListener(),
>> - new DetailsButtonListener() };
>> - JButton button;
>> + final String[] controlTexts = {
>> + R("CVCertificateType"),
>> + R("CVImport"),
>> + R("CVExport"),
>> + R("CVRemove"),
>> + R("CVDetails"),
>> + R("ButClose")
>> + };
>> + final Map<String, Character> mnemonicsMap = UI.generateMnemonics(controlTexts);
>> + certificateTypeLabel.setDisplayedMnemonic(mnemonicsMap.get(R("CVCertificateType")));
>>
>> //get the max width
>> int maxWidth = 0;
>> - for (int i = 0; i < buttonNames.length; i++) {
>> - button = new JButton(buttonNames[i]);
>> + for (int i = 1; i < 5; i++) {
>
> buttonNames.length is definitively better then "5".
>> + JButton button = new JButton(controlTexts[i]);
>> maxWidth = Math.max(maxWidth, button.getMinimumSize().width);
>> }
>>
>> - for (int i = 0; i < buttonNames.length; i++) {
>> - button = new JButton(buttonNames[i]);
>> - button.setMnemonic(buttonMnemonics[i]);
>> - button.addActionListener(listeners[i]);
>> - button.setSize(maxWidth, button.getSize().height);
>> - // import and remove buttons
>> - if (i == 0 || i == 2) {
>> - disableForSystem.add(button);
>> - }
>> - buttonPanel.add(button);
>> - }
>
> Yup, here is the duplicated code. I know there will be some ifs, but really iteration is preferred
> way.
>> + this.importButton.setText(controlTexts[1]);
>> + this.importButton.setMnemonic(mnemonicsMap.get(controlTexts[1]));
>> + this.importButton.addActionListener(new CertificatePane.ImportButtonListener());
>> + this.importButton.setSize(maxWidth, this.importButton.getSize().height);
>> + buttonPanel.add(this.importButton);
>> +
>> + this.exportButton.setText(controlTexts[2]);
>> + this.exportButton.setMnemonic(mnemonicsMap.get(controlTexts[2]));
>> + this.exportButton.addActionListener(new CertificatePane.ExportButtonListener());
>> + this.exportButton.setSize(maxWidth, this.exportButton.getSize().height);
>> + this.exportButton.setEnabled(false);
>> + buttonPanel.add(this.exportButton);
>> +
>> + this.removeButton.setText(controlTexts[3]);
>> + this.removeButton.setMnemonic(mnemonicsMap.get(controlTexts[3]));
>> + this.removeButton.addActionListener(new CertificatePane.RemoveButtonListener());
>> + this.removeButton.setSize(maxWidth, this.removeButton.getSize().height);
>> + this.removeButton.setEnabled(false);
>> + buttonPanel.add(this.removeButton);
>> +
>> + this.detailButton.setText(controlTexts[4]);
>> + this.detailButton.setMnemonic(mnemonicsMap.get(controlTexts[4]));
>> + this.detailButton.addActionListener(new CertificatePane.DetailsButtonListener());
>> + this.detailButton.setSize(maxWidth, this.detailButton.getSize().height);
>> + this.detailButton.setEnabled(false);
>> + buttonPanel.add(this.detailButton);
Iteration is definitely possible here, but I fear readability is going to suffer
more in this case than having gains from code compactness. If the number of
iterations were in the tenths or more I would do prefer iteration or size of
code before readability.
>> tablePanel.add(tabbedPane, BorderLayout.CENTER);
>> tablePanel.add(buttonPanel, BorderLayout.SOUTH);
> ...snip...
>>
>> @@ -112,12 +113,16 @@
>> private static void setSystemLookAndFeel() {
>> try {
>> UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
>> - } catch (Exception e) {
>> + } catch (ClassNotFoundException classNotFoundException) {}
>> + catch (InstantiationException instantiationException) {}
>> + catch (IllegalAccessException illegalAccessException) {}
>> + catch (UnsupportedLookAndFeelException unsupportedLookAndFeelException) {}
>> + catch (ClassCastException classCastException) {
>> // don't worry if we can't.
>> }
>
> Well, not sure if worthy, but as you wish. I would be proably fan of ex.printstackTrace in case of
> debug on. But I do not isnisits.
Sure, I can add this.
>> }
>>
>> - public static void main(String[] args) throws Exception {
>> + public static void main(String[] args) {
>> CertificateViewer.showCertificateViewer();
>> }
> ...
>> +/**
>> + * Impementation of a certificate viewer.
:-( typo. I will correct this right away.
>> + * <p>
>> + * The certificate viewer can be accessed via the "Certificates" tab or by
>
> I would add
> * The certificate viewer can be accessed via the "Certificates" tab in itw-settings or by
>
> But do not isnists.
>> + * invoking {@code javaws -viewer} on the command line.
>> + * </p>
>> + * @see net.sourceforge.jnlp.controlpanel.ControlPanel
>> + */
> ...snip...
>> +
>> +/**
>> + * This class provides methods and classes for common and recurring tasks while
>> + * dealing with the UI that are not provided by the Java SE platform.
>> + * @since 1.5
>> + */
>> +public final class UI {
>> + /**
>> + * Hide the default constructor from instantiating this class, because it is
>> + * a mere collection of diverese helper functionalities encapsulated in either
>> + * static methods or static nested classes.
>> + */
>> + private UI() {
>> + }
>> +
>> + /**
>> + * Generates corresponding mnemonics to texts of controls like buttons,
>> + * labels, check boxes, radio buttons, or menu items. This method is primarily
>> + * useful for applications which control's texts are loaded from localized
>> + * resources that either lack mnemonics for corresponding controls or when
>> + * storing mnemonics in resources is not desired.
>> + * <p align="justify"><b>NOTE:</b><br/>
>> + * This method works with Latin, Greek, and Cyrillic alphabets only!</p>
>> + * This method may produce multiple identical (recurring) mnemonics if a text
>> + * does not have enough distinct letters.
>> + * @todo Add support for right-to-left alphabets and other scripts.
>> + * @param strings A {@link String} array with all Window's or Dialog's
>> + * control's texts that shall get mnemonics
>> + * @return a {@link HashMap} of texts with their corresponding mnemonic characters
>> + */
>
> Ugh. Although the idea behind this is really _excelent_ I doubt it a bit.Or I doubt its complexity.
It is possible indeed. ;-) I have checked it. It requires specific code for some
languages. And, it is okay, since we always know what language those texts are.
I did not add support for other alpabets and scripts because I do not know them.
In general, for alphabet scripts it is important to avoid using extended or
composed characters as mnemonics as they may not be directly available on the
keyboard. Of course, this may not be possible in every case. Some languages like
Turkish or Vietnamese are quite likely to have words that are comprised of
composed characters entirely, even though these languages use latin alphabets.
Hence, in this case perhaps one is left with no other option than to use
composed characters and adding language specific code to this method.
I never claimed this method to be complete in every aspect. There is always room
for improvement, so why not use it? ;-)
> Before scratching it - few topics:
> - is really mnemoic based on localisation desired behaviour? I'm in favour that it is not. Mostly
> one simpli look into menu, and when he used this click and find click to often, he remebers the
> mneonic. Maybe it is even better when all alnguages have them same. But you know more about
> localisations then me.
No, definitely not. Mnemonics are almost always based on controls' texts. One
must not assume that mnemonics in e.g. English - the same letters - will be
available with localized texts. For all alphabet scripts, except for Korean
Hangul, localized mnemonics should be provided because this has been the UI
style guide ever since and is relatively easy to implement. If one were to force
identical mnemonics for all languages one would have to mix scripts or alphabets
in *one* text (as this is by UI style guide the case for Chinese, Japanese, and
Korean) and would have ugly " (<mnemonic>)" suffixes to texts in scripts and
alphabets where it is actually unnecessary. Anyway, it is a UI style guide
principle to have localized mnemonics.
After all, in some regions or for some languages multiple keyboard layouts are
used in parallel. This makes stuff even more complicated. The same applies to
accelerators or hot keys. They have to be localized too. So, some language
specific code has to be added in the future anyway. And, as already mentioned we
always know any text's language so it is no problem to put it all into one
method (or maybe sub-methods).
Actually, I could have added generic mnemonic support for Chinese, Japanese, and
Korean because they use U.S. English mnemonics anyway. But since IcedTea-Web
does not support these languages yet, adding this support should be better left
to their specific localizers.
> - the implementation should allwo default values - especialy for not "Latin, Greek, and Cyrillic "
> or strings which "does not have enough distinct letters"
As I mentioned above; There is always room for improvement. :-) Let us think
about it when IcedTea-Web is about to get support for other languages. Besides,
it is also a legal UI style guide to have multiple colliding or equal mnemonics
when no other options are available. It is even better to have colliding
mnemonics than none.
> - I think that the implementation is too complicated according what it do (see inline)
It is always going to be complicated as long as current human languages exist or
are in use because almost all of them are not systematic. And, this is exactly
the reason why we have computers in the first place. We deal with this
complexity once and implement it, so that from then on it is handled by a stupid
machine.
> - first letters of words should be preffered
If you read closely, they are indeed. ;-)
> - it will need unittest;)
Yes, possibly. But, this is going to require a per language unit test which will
quickly become very elaborate. So, I am rather skeptical that it would be a good
idea to insist on any unit test in this case since UI.generateMnemonics() does
not provide any core or crucial functionality nor is it probable for this method
to have any side effects (except for throwing an exception itself when fed with
inadequate data) on other parts of the application.
> I'm in grete favour to remove this from this changeset and proceed it as separate patch.
Only if you are willing to search for some other open source code that deals
with this problem (generating mnemonics based on language) sufficiently. I doubt
you will find anything. ;-)
>> + public static Map<String, Character> generateMnemonics(final String[] strings) {
>> + if (strings == null) throw new IllegalArgumentException("strings");
>
> At lest "input string was null" ;)
Yep, always check parameters in public methods! :-)
>> +
>> + final int stringsLength = strings.length;
>> + final HashMap<String, Character> hashMap = new HashMap<String, Character>(stringsLength);
> This to shoudl be final Map<String, Character> hashMap = new HashMap...
>
>> + for (int index = 0; index < stringsLength; index++) {
>> + // Remove all white spaces, because they must not be mnemonics
>
> maybe if (strings[index] == null) throw new IllegalArgumentException("strings") too?
In this case I would rather prefer to see a NPE pop up and change the
documentation to clearly state that an "array of {@code String}s" is to pass as
parameter. Null checking the entire array may take a while if it is quite long.
>> + final String s = strings[index], reducedString = s.replaceAll("[^A-Za-z0-9\u0391-\u03A9\u03B1-\u03C9\u0410-\u045F]" /*"[\\s\\p{Punct}\\p{Cntrl}\u0500-\uFFFF]"*/, "");
> s.replaceAll("/s","") should do the job ;)
> - If you really wont to remove only whitespaces, but I see different in regex.
> - Maybe jsut keep [A-Z,a-z] would be more usefull
No, it is correct this way. Removing whitespaces only will leave all
punctuation, mathematic, composed characters etc. in place. What is needed here
is actually to have acceptable mnemonic characters left over only.
>> + char mnemonic = '\u0000';
>> + final int reducedStringLength = reducedString.length();
>> + for (int charIndex = 0; charIndex < reducedStringLength && hashMap.containsValue(mnemonic = Character.toUpperCase(reducedString.charAt(charIndex))); charIndex++);
> interesting line :)
Pumped it to death! :-D
Infact, Oracle's and OpenJDK's compiler does produce the most compact and
fastest code when confronted with as little but as complex statements as
possible. It is because language level optimizations in compilers are the
easiest to implement and have received the most research attention, in contrast
to bytecode level optimizations for example.
>> + hashMap.put(s, mnemonic);
>> + }
>> +
>> + return hashMap;
>> + }
>> +
>
>
> I would probably move this to separate calss file.
>
> Also it would be nice to have some smoke test for this, but as I'm not sure of comlexity I do not
> isnists.
Why? Its purpose is concerned with UI, isn't it?
>> + /**
>> + * A table model that is in effect a {@link DefaultTableModel} except for no
>> + * cell being editable.
>> + * @see DefaultTableModel
>> + */
>> + public static class NonEditableTableModel extends DefaultTableModel {
>> + /**
>> + * Constructs a {@link javax.swing.table.TableModel} that serves only one
>> + * purpose: make cells of certificate tables not editable.
>> + * @see DefaultTableModel#DefaultTableModel()
>> + */
>> + public NonEditableTableModel() {
>> + super();
>> + }
>> +
>> + /**
>> + * Constructs a {@link javax.swing.table.TableModel} that serves only one
>> + * purpose: make cells of certificate tables not editable.
>> + * @param rowCount the number of rows the table holds
>> + * @param columnCount the number of columns the table holds
>> + * @see DefaultTableModel#DefaultTableModel(int,int)
>> + */
>> + public NonEditableTableModel(final int rowCount, final int columnCount) {
>> + super(rowCount, columnCount);
>> + }
>> +
>> + /**
>> + * Constructs a {@link javax.swing.table.TableModel} that serves only one
>> + * purpose: make cells of certificate tables not editable.
>> + * @param data the data of the table
>> + * @param columnNames the names of the columns
>> + * @see DefaultTableModel#DefaultTableModel(Object[][],Object[])
>> + */
>> + public NonEditableTableModel(final Object[][] data, final Object[] columnNames) {
>> + super(data, columnNames);
>> + }
>> +
>> + /**
>> + * Constructs a {@link javax.swing.table.TableModel} that serves only one
>> + * purpose: make cells of certificate tables not editable.
>> + * @param columnNames {@code array} containing the names of the new columns;
>> + * if this is {@code null} then the model has no columns
>> + * @param rowCount the number of rows the table holds
>> + * @see DefaultTableModel#DefaultTableModel(Object[],int)
>> + */
>> + public NonEditableTableModel(final Object[] columnNames, final int rowCount) {
>> + super(columnNames, rowCount);
>> + }
>> +
>> + /**
>> + * Constructs a {@link javax.swing.table.TableModel} that serves only one
>> + * purpose: make cells of certificate tables not editable.
>> + * @param columnNames {@code vector} containing the names of the new columns;
>> + * if this is {@code null} then the model has no columns
>> + * @param rowCount the number of rows the table holds
>> + * @see DefaultTableModel#DefaultTableModel(Vector,int)
>> + */
>> + public NonEditableTableModel(final Vector columnNames, final int rowCount) {
>> + super(columnNames, rowCount);
>> + }
>> +
>> + /**
>> + * Constructs a {@link javax.swing.table.TableModel} that serves only one
>> + * purpose: make cells of certificate tables not editable.
>> + * @param data the data of the table, a {@code Vector} of {@code Vector}s
>> + * of {@code Object} values
>> + * @param columnNames {@code vector} containing the names of the new columns
>> + * @see DefaultTableModel#DefaultTableModel(Vector,Vector)
>> + */
>> + public NonEditableTableModel(final Vector data, final Vector columnNames) {
>> + super(data, columnNames);
>> + }
>> +
>> + /**
>> + * This method always returns {@code false} to make the table's cells not
>> + * editable.
>> + * @param row the row whose value to be queried
>> + * @param column the column whose value to be queried
>> + * @return always {@code false}
>> + */
>> + @Override
>> + public boolean isCellEditable(final int row, final int column) {
>> + return false;
>> + }
>> + }
>> +}
>> \ No newline at end of file
>>
>
>
> Again - greate work! Thank you vcery much for it
>
> J.
I will fix the mentioned nits as soon as possible which though may take quite a
while because I am involved in some other stuff currently.
Regards,
Jacob
More information about the distro-pkg-dev
mailing list