[rfc][icedtea-web] Cache viewer cleanup

[Jakob Wisor]

Jiri Vanek schrieb:
> On 10/01/2013 10:51 PM, Jacob Wisor wrote:
>> Hello,
>>
>>> Jiri Vanek wrote:
>>>> On 09/16/2013 02:17 AM, Jakob Wisor wrote:
>>>>> Hello there!
>>>>>
>>>>> I have checked and corrected the code's formatting, and followed
>>>>> Jiri's reasonable advice.
>>>>>
>>>>> Jiri Vanek wrote:
>>>>>> On 07/31/2013 01:47 AM, Jacob Wisor wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> * Added closing of cache viewer by ESC key
>>>>>>> * Added proper dis-/enabling of cache viewer's buttons
>>>>>>> * Added busy mouse cursor indicator when populating the cache viewer
>>>>>>>
>>>>>>> Everything is done on purpose on the AWT thread as before. Although
>>>>>>> I am personally not in favor
>>>>>>> of anonymous classes, I have tried to stick to the prevalent coding
>>>>>>> style.
>>>>>>>
>>>>>>> Unfortunately, I was unable to test the cache viewer's behavior
>>>>>>> because I do not know how to put
>>>>>>> synthetic cache entries into the cache. Can anyone help me with
>>>>>>> that? What does a the cache look
>>>>>>> like when filled with resources? Is there any index file that keeps
>>>>>>> track of the cache?
>>>>
>>>> As this is gui change only, it shouldbe ok without test.
>>>> However cache is set of jars and jnlp files saved in
>>>> url-like-filestrucutre
>>>>
>>>> eg http:\\my.domain\myApp.jnlp with myJAr in "." codebase will be
>>>> saved like
>>>> cache/http/my/domain/myApp.jnlp
>>>> cache/http/my/domain/myJar.jnlp
>>>>
>>>> and yes, there is index file in tom cache directory
>>>>
>>>> I have some memorry that there are already tests for it - most simple
>>>> case how to import jar into cache is to lunch javaws ;)
>>>>
>>>>>>>
>>>> ...,snip...
>>>>>> Those should be in finally block
>>>>>>> + parent.getContentPane().setCursor(Cursor.getDefaultCursor());
>>>>>
>>>>> Done. Please check whether I have done it correctly or the way you
>>>>> have meant.
>>>>>
>>>>>>> }
>>>>
>>>> Thank you.
>>  >>>> [...]
>>>>>> Please mention both Cache viewer cleanup and CerViewr viewer
>>>>>> cleanup in news.
>>>>>
>>>>> Will do but they are separate patches.
>>>>>
>>>>> Happy reviewing!
>>>>
>>>> For _this_ patch, the NEWS line should be attached explicitly.
>>>> For CerViewr the separate patch is ok.
>>>>
>>>>
>>>> No more review needed. With above fixed, ok to head (and 1.4 if wonted)
>>>
>>> Okay! :-) Thank you for reviewing. But, I want to test it at least a
>>> little bit. Now, that I have written a few JNLP application and applet
>>> tests I have kinda also found the time and leisure to really do it. ;-)
>>> It's better do it once before feeling sorry afterwards.
>>
>> After throughly testing this patch I think it is ready to be pushed.
>> Please review the NEWS and ChangeLog sections. I have also decided to 
>> include the new NonEditableTableModel class with this patch, since 
>> this patch can also make use of it. It is also going to be ready for 
>> the upcoming certificate viewer patch.
> 
> I'm happy with the patch code, however the delete button have not worked 
> for me :(

Right :( Sorry for any inconvenience. Thank you for catching this. I forgot to 
test the delete button again after changing the cache table's data model.

> Also I have noted  two empty catch blocks. Please log the exceptions in 
> Debug mode on.

Ah yes, I was not aware that this would be of any interest. Done.

> Also  - that is nit probably for another change set (if changeset at 
> all) - is multiple selection or "delete all" button in your queue?

Indeed, I have thought about this too (multi-select is probably a better 
approach). Generally speaking there is still a lot of room for improvement, 
especially when compared to e.g. Oracle's JNLP client (I don't endorse Oracle's 
JNLP client). This and other stuff is on my list of improvements, though I am 
unsure about as to when or ever since my time is limited.

> Once delete button working/not working cleared and catch block logged, 
> ok to go.

A change to the cache viewer's table model has caused this. I have reverted to 
DirectoryNode for column 0. I am not really fond of it because DirectoryNode 
should implement Comparable, but this would cause even more work and probably 
testing. File, as initially intended for column 0, does implement Comparable, so 
IMHO is more suitable as a complex type in the model. But, column 0 contains 
effectively file names only, hence the default String comparator is functionally 
equivalent for the user. The delete button is working as intended again.

Regards,
Jacob