SPARK-1926, SPARK-1927, SPARK-1928, SPARK-1929, SPARK-1938 and SPARK-1937

[Alameyo]

SPARK-1926, SPARK-1927, SPARK-1928, SPARK-1929, SPARK-1938 and (here might be some changes in future) SPARK-1937.

[Alameyo]

Now also SPARK-1940.

[guusdk]

Yey! The first bit of work for GSoC! \o/

In short: There's nothing wrong with this code that I can see. Good job! I do have a couple of suggestions for improvement:

You appear to be applying an MVC pattern, correct? If so, please add that to your javadoc. It will help others that, at some point in the future, want to build further on this code!

It is probably a good idea to do input validation on the argument values in constructors and methods - make sure that what you think you receive, is actually what you do receive. This can be very simple: define, for each parameter, if the value can be null (and document that in javadoc), and in the first few lines, check if the value is valid. You can simply throw a runtime exception if it is not. You can throw a runtime/unchecked exception, as such a condition is a clear violation of the API 'contract' (the javadoc that you'll write) - a programming bug. I most of the time use an IllegalArgumentException when an argument has an illegal value, but you can also use an exception that is more specific to the type of violation. For instance: NullPointerException, IndexOutOfBoundsException, etc). It is primarily a matter of taste. Whatever you think is best (but I advice you to not spend to much time on this. Exception handling is good to have, but in itself, it is not the purpose of your project).

There's not a lot of logic yet, so this might simply not have come up yet, but: use logging! And not to stdout please, but use the logfiles. :)

I think your editor wraps comments/javadoc at 80 characters width? I'd change that to 120 or 160. No-one is using text terminals any more. ;)

It might be worth to combine somehow the existing 'PKI' tab, and your new 'certificates' tab. They both relate to 'private key infrastructure' - although I'm not sure yet exactly how these should be combined. Food for thought, perhaps.

I'm very happy to see that you immediately placed the files in logical packages, and used the translation files instead of hard-coding text strings. Small things like that show that you have a pretty good overview of the project already!

I'm looking forward to see more from you!

[guusdk]

Let's merge this code, after you build in something that, while under construction, does not show the new features, without toggling a property. I think you already did most of that work with your addition to org.jivesoftware.resource.Default ...

[Alameyo]

You appear to be applying an MVC pattern, correct?

Yes, I added note in Javadocs about it.

It is probably a good idea to do input validation on the argument values in constructors and methods.

Done

...use logging! And not to stdout please, but use the logfiles. ;)

Already I am doing that when catching exceptions. I am not sure if I should do it also somewhere else.
catch(Exception e) { Log.warning("Something wrong", e) }

I think your editor wraps comments/javadoc at 80 characters width? I'd change that to 120 or 160. No-one is using text terminals any more. ;)

Oh, yeah my comments/javadocs were only 80 while rest of the code 120. I changed it :)

It might be worth to combine somehow the existing 'PKI' tab, and your new 'certificates' tab.

I thought about it, but I am not sure how it will be combined well with panel for mutual authentication that I will need to add somewhere later in project. In any case it shouldn't be a problem to combine things from PKI tab with new panels in later.

Let's merge this code, after you build in something that, while under construction, does not show the new features, without toggling a property.

It seems that the simplest is commenting out addition of that tab, which now I have done.

[guusdk]

Although this will obviously do the trick, isn't the purpose of this if statement to enable/disable things, from the value of CERTIFICATES_MANAGER_DISABLED? Shouldn't we use that?

[Alameyo]

Now I think it's fine.

[guusdk]

Excellent. Let's not forget to switch this back after you're done with the project :)

[wrooot]

This looks a bit weird (spacing between options)

I understand that Advanced window has been stretched to accomodate larger Certificates tab. Not sure if something can be done about it. This looks different from what is used through out the application (spacing).

[Alameyo]

I didn't done nothing more than adding certificates tab, what would be visible for users with old panels. It seems I can reduce a bit that spacing between options. As I am checking older versions it doesn't appear for me as this dialog become stretched, but I can try to size it down.

[wrooot]

Having an empty spot at the bottom is not pretty too, but at least it would be consistent with other parts of Spark's GUI (say in Preferences, many windows have a few options and all of them are stacked at the top of the window).
This is the not stretched version for comparison

[Alameyo]

That's how it looks for me, so it is even more stretched here but it was this way all the time before I started adding Certificates tab. I am not sure yet why you and me see it different but I will try to investigate that, maybe I will also allow users to resize this.

[guusdk]

The UI will look very differently, depending on what OS you're using, and what theme/style gets enabled.

Let's go with @wrooot 's suggestion, and "anchor" the content (but not the buttons on the bottom) to the top-left corner (instead of 'stretching' it over the entire height). That should give you 'empty space' between the checkboxes and the Ok/Cancel/UseDefault buttons, if the window is larger than the content.