[rfc][icedtea-web][itweb-settings] Improve Icedtea-Web cache disk space

[Jie Kang]

Hello,

Two minor nits. You have my +1 after the changes :)

+        private final String prop;
+        Properties(final String prop) {
+            this.prop = prop;
+        }
I'd prefer the name property instead of prop. Short forms should be used sparingly in my opinion.

+        this.addComponentListener(new ComponentListener() {
Instead of using new ComponentListener() you should use ComponentAdapter(). That way you won't need to implement 3 empty methods.

"The class that is interested in processing a component event either implements this interface (and all the methods it contains) or extends the abstract ComponentAdapter class (overriding only the methods of interest)." [1]
[1] http://docs.oracle.com/javase/7/docs/api/java/awt/event/ComponentListener.html
+            @Override
+            public void componentResized(final ComponentEvent componentEvent) {
+            }
+
+            @Override
+            public void componentMoved(final ComponentEvent componentEvent) {
+            }
+
+            @Override
+            public void componentShown(final ComponentEvent componentEvent) {
+                listener.stateChanged(null);
+            }
+
+            @Override
+            public void componentHidden(final ComponentEvent componentEvent) {
+            }
+        });

+            final int powersListSize = (int) Math.floor(Math.log(cacheMaxSize) / Math.log(spinnerStepSize) + 1);
...
+            final Long result = original - (original % powersOf.get(String.valueOf(original).length() - 1)) + powersOf.get(String.valueOf(original).length() - 1);

For code like this I think comments describing the basics of what is occurring and/or why it is occurring would be very helpful.


Also in the future I would suggest creating unit tests in conjunction with new features (apply TDD!!!) so that we can be assured things work as expected without manual testing. E.g. the operation of the spinner could definitely be unit tested to make sure it increments/decrements by powers of 10 as expected.


Regards,

----- Original Message -----
> 
> Hello,
> 
> > Hello,
> > 
> > When you check the 'limit cache size box' with a size of 0, the JAR
> > compression option and cache location option are disabled. At the moment,
> > limiting cache size to 0 means that once icedtea-web finishes, the cache is
> > cleared of all files.
> > 
> > I think the description for Caching should be reworded a little to make
> > this
> > known. E.g.
> > 
> > "No files will be cached. Cached files will be deleted." can be reworded to
> > something like:
> > "Cached files will be deleted on icedtea-web close.
> 
> Okay
> 
> > As well, the ability to choose where icedtea-web downloads file should
> > still
> > be enabled even if cache has a limit of 0.
> 
> Yes since it still downloads the files even if they are deleted after use.
> This provides the user with more choice on where they want the files to be
> temporarily.
> 
> > In terms of code, it functionally looks okay but I would strongly suggest
> > adding logically placed line spaces throughout the code for easier reading.
> 
> Yes, sorry about that, I added spaces in a bunch of places now to increase
> readability.
> 
> Thank you,
> Lukasz Dracz
> 

-- 

Jie Kang