[rfc][icedtea-web] Certificate viewer cleanup
Jiri Vanek
jvanek at redhat.com
Mon Sep 9 04:50:37 PDT 2013
On 09/06/2013 09:16 PM, Jacob Wisor wrote:
> 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!
>>
>>
...snip...
>> 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.
Well, no important work to do to reproduce - just start _any_ itw-settings, javaws or plugin....
>
>> 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.
>
ok. Here we disagree on 100% :) Please take bellow as my personal opinions:
- non resizeable dialog window is most evil thing which can happen to any windows manager.
- javaws -viewer is 100% stand alone application so it should be jframe (focusable both alt+tab and
visible in application bar)
- custom jdialog is perhaps to much work for to few - and do not solve the resizeability O:)
If you disagree (as you looks you are), please do not include this sugestion in your changeset. If
somebody else (me? :) ) will find it worthy of patch, then it will happen and we will see.
>> - 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?).
From abowe I like the status bar most. It Can show just part of the error mesage and add text like
"hover mouse over button to see the full message). Or there can be something like "You are missing
some permissions, hover mouse...", or "All your permissions looks correct"
But otherwise I consider the solutions as equally ok and proceed as you wish.
>
>> 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...
ouch. Please update then patch wehn you will found free cycle.
>
> 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
Sorry for confuse you (Thats why I have used "hack" instead of hack). There is nothing hackhish on
this! It was just unknow procedure for me.
> 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.
>
In java there is to much switches like If linux then else.. If windows then ...else... So be not so
sure about this:(
> 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
yah :(( Sad example of above :(
> 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. ;-)
definitely!-)
>
>>> + }
>>> +
>> ...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.
fair enough. I have quicky checked the rest of netx and it looks like there is no more of those strings.
>
>>> - 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.
Jsut to think about - Do we need to care about fe bytes long arrays to be garbage-collected - imho
not. But we need to take care about errornes of duplicate code. I would really lik to encourage you
to proceed with the array.
>
>>> 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.
I'm the last one who can note this.
Thank you!
>
>>> + * <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
not passed? :(
>>
>> 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
I dont think so. You do not need to do complex tests. Imho very simple test with few input strings
anch asserts what is in returned map. They should also serve as documentin what the hell is it
dooing (on corner cases) :)
Don't forget you do not need properties in this test, so it will become compeltely
language-independent.
> 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. ;-)
Ugh, I doubt too. But I also doubt of language depndent mnemonics. See - ctrl+c, ctrl+v ctrl+o,
ctrl-n, ctrl-s - all are well known international mnemonics.
If you can move this to separate changeset, it would really be the best.
>
>>> + 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.
I do not feel convinced but do as you wish.
>
>>> + 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
Then the comemnt is wrong
> characters left over only.
Then I would go with simple
s.replaceAll("[^A-Za-z0-9]", "" )
>
>>> + 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.
Well, Premature optimalization is root of all evil :)
Readable code in non-performance-critical parts is much more prefeared then optimized one.
So again - I do not feel convinced but do as you wish.
>
>>> + 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?
Yes it is. Butit is long enough to deserve 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