[rfc][icedtea-web] Removing applications tab in jawas-about
Jiri Vanek
jvanek at redhat.com
Mon Jun 3 12:39:40 PDT 2013
On 05/31/2013 04:54 PM, Andrew Azores wrote:
> On 05/27/2013 06:56 AM, Jiri Vanek wrote:
>> On 05/24/2013 09:45 PM, Andrew Azores wrote:
>>> On 05/22/2013 10:22 AM, Jiri Vanek wrote:
>>>> On 05/22/2013 03:25 PM, Andrew Azores wrote:
>>>>> Removed applications.html and references to it in "jawas -about"
>>>>>
>>>>> Changelog:
>>>>>
>>>>> extra/net/sourceforge/javaws/about/Main.java: Removed applications tab
>>>>> extra/net/sourceforge/javaws/about/resources/applications.html: Removed unneeded file
>>>>
>>>>
>>>> Hi! To be honest - I'm for complete removal of this "about". Or at least of much more havy
>>>> refactoring
>>>>
>>>> Also to be honest(2) this is so deeply needed that it deserves an line in
>>>> http://icedtea.classpath.org/wiki/IcedTea-Web#IcedTea-Web_1.5 table.
>>>>
>>>> It should work at least somehow in headless mode, and should be generated from already existing
>>>> resources (eg authors, news...)
>>>>
>>>> So in short - get rid of "extras" jar (and it logic in makefile) and write specialised about
>>>> dialogue inisde netx itself:) Feel free to be inspired by existing one, but avoid duplicated
>>>> resources.
>>>>
>>>> if you want to bother with it (+1!) then please assign yourself on
>>>> http://icedtea.classpath.org/wiki/IcedTea-Web#IcedTea-Web_1.5, and go on!
>>>>
>>>>
>>>>
>>>> Best regards from CZ!
>>>> J.
>>>
>>> Hi Jiri,
>>>
>>> I'll take a look into creating that dialogue, it should be a good opportunity for me to keep
>>> learning more about the code base before delving into more difficult tasks.
>>>
>>
>> here you are http://icedtea.classpath.org/wiki/IcedTea-Web#IcedTea-Web_1.5 :))
>>
>> Please try to move in as small steps as possible or split the patch to as many logically-complete parts as possible, otherwise they will be very difficult to review.
>> This change should be simple code, but there will be probably huger amount of it.
>> You can start with moving the window into embedded one out of extreas.
>> Also it would be nice to have as much of it generated from NEWS, AUTHORS, maybe Changelog, COPYING? Reuse the rest?
>> just nits... I do not wont to put boundaries to your imagination which must be much more fresh then my is :)
>>
>> Best regards
>> J.
>>
>>
>>
> Changelog:
>
> Makefile.am: removed logic for extras.jar
>
> netx/net/sourceforge/jnlp/runtime/Boot.java: Added -headless -about mode
>
> netx/net/sourceforge/jnlp/resources/Messages.properties: Added messages used in -headless -about
>
> netx/net/sourceforge/jnlp/about/About.java: Changed name from Main, moved out of extras, added logic to generate content from AUTHORS, COPYING, NEWS, ChangeLog
>
> netx/net/sourceforge/jnlp/resources/about.jnlp: References changed About class
>
> netx/net/sourceforge/jnlp/about/Constants.java: String constants pulled out of About.java
>
> netx/net/sourceforge/jnlp/about/HTMLPanel.java: Moved out of extras.jar
>
> netx/net/sourceforge/jnlp/resources/about.html: Relocated
>
> netx/net/sourceforge/jnlp/resources/applications.html: Relocated
>
> netx/net/sourceforge/jnlp/resources/jamIcon.jpg: Relocated
>
> netx/net/sourceforge/jnlp/resources/notes.html: Relocated and commented out authors lines since there is a new Authors tab
>
> Lots of changed files but most of them have small changes made to them eg relocation, hopefully this is not too hard to review overall. I'm also not sure what else should be done for -about -headless, I'd definitely like some feedback on what else to put in there.
Ugh, general comment - never split patch to such a pieces - when I wrote split to small steps or logically-complex parts, thsi was not in my mind :)
Mostly the logically-compelx are refactoring, changes and tests. Not individual filles :) Deeply sorry for confusion.
But this round is not vaste of time:
Please, post all refactoring as separate changeset:
- move of classes and delation of unnecessary files
- makefile and code changes according to this change
- maybe thsi will affect some exiting tests. Please double ensoure that all unittests and reproducers works
In second changes you will add new code and tests
- again - except new tests this will affect some exiting tests. Please double ensure that all unittests and reproducers works
The codechanges itself:
- the about.jnlp file msut be reomoved. It have no place in new schema.
- code itself looks good (not deep investigations), but all new methods needs unittests
Whats is:
> +++/--- b/netx/net/sourceforge/jnlp/resources/notes.html
> @@ -18,6 +18,7 @@
> </div>
> </td>
> </tr>
> + <!--
> <tr>
> <td valign="Middle" nowrap="true" align="Justify">
> <div align="Center">
> @@ -41,6 +42,7 @@
> </div>
> </td>
> </tr>
> + -->
> </tbody>
> </table>
> </td>
>
>
??
The code should be removed, not commented, and tbh, why so? /me to lazy to read rest of file
- if (null != getOption("-about"))
- System.out.println(aboutMessage);
+ if (null != getOption("-about")) {
+ if (null != getOption("-headless")) {
+ System.out.println(itwInfoMessage);
+ System.exit(0);
+ } else {
+ System.out.println(aboutMessage);
+ }
+ }
I'mnot sure if I follow - the haedless message is much more complex then "normal" -it is correct, but I do not see the launching of dialogue
It is not launching in else branch?
for (InputStream stream : streams) {
+ try {
+ File tempfile = File.createTempFile("javaws-about", ".html");
+ tempfile.deleteOnExit();
+ FileOutputStream fos = new FileOutputStream(tempfile);
+
+ int bytesRead = stream.read(cbuf, 0, cbuf.length);
+ for (int i = 0; i < bytesRead; i++) {
+ if (cbuf[i] == '\n') {
+ fos.write(Constants.HTML.br);
Please do not use FileOutputStream as base class
Use (buffered)reader/writer build upon FileStream *with specified encoding* (utf-8) in this case
Also , TBH... This is the most complicated and actually strangest filesaving :)
+ public static class HTML {
+ public static final byte[] lt = {'&', 'l', 't', ';'};
+ public static final byte[] gt = {'&', 'g', 't', ';'};
+ public static final byte[] amp = {'&', 'a', 'm', 'p', ';'};
+ public static final byte[] br = {'<', 'b', 'r', '>'};
+ }
Nonnnoo :) Please use plain strings like "<br>" where possible. THis is not mission critical code :)
+ public static final int buffer_size = 20480; // 20KB
NOOOO! DO not reinvite the wheel:)
Please use buffered machanism already presented in java.
After ^ and ^^ remove, the other "public static final String " in Constants as separate class maybe lsot its purpose, but we will se how it will look in next round.
Two *RFC* at the end:
- tabs - You have created all the tabs. On one side, it is not bad, on second it is to compelx. Especially the Changelog one. Please try to think about it and reduce the amount of information to usefull limit.
Also the news should be limited to latest release or something like that. No borders to imagination! Try to thinkg about it like user, what do you expect to find here?
- the generation of content - I'm not sure if generating the files in runtime (and so adding News, License.. adn so on into INSTALL is good idea.
On the other hand, I'm not sure if some pre-generation (which I had in my mind initially) is better idea :)
Sorry if review is not straightforward, I did my best for such a supplicated one :(
Sorry one more times If I wrote my previous email in confusing way :(
And thank you very much for doing this, This patch on its best way!
J.
More information about the distro-pkg-dev
mailing list