[sip-comm-dev] conversation style: issues 645 and 378


#1

Hi,

I just wanted to know whether anyone is working on these issues (645
and 378). I'd like to work on them and I wouldn't like to step on
someone else's toes.
I'm having have fun reading the Adium specification on themes[1] and
if no one is looking into it I'll start to code something soon.

Best regards,
Edgar

[1]
http://trac.adium.im/wiki/CreatingMessageStyles
http://trac.adium.im/wiki/CreatingMessageStyles/Tutorial

···

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#2

Hi Edgar,

I was planning to work on these at one moment, but never got around to them.

Adding Adium themes shouldn't be that difficult, as we're already using html and css. However the modification of the ChatConversationPanel could turn to be painful. Anyway, I'm very interested in this feature and I'll be more than happy to help.

Keep in mind that the Swing team is now working on JWebPane and as soon as it's finished we're probably going to replace the existing JEditorPane that we're using in the conversation area. It's just that JWebPane would be much more powerful html component.

Looking forward to your posts on the subject!
Yana

···

On Sep 9, 2009, at 6:27 PM, Edgar Poce wrote:

Hi,

I just wanted to know whether anyone is working on these issues (645
and 378). I'd like to work on them and I wouldn't like to step on
someone else's toes.
I'm having have fun reading the Adium specification on themes[1] and
if no one is looking into it I'll start to code something soon.

Best regards,
Edgar

[1]
http://trac.adium.im/wiki/CreatingMessageStyles
http://trac.adium.im/wiki/CreatingMessageStyles/Tutorial

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#3

Hi Yana,

Thanks for your help, I list bellow the different group of tasks that
I identified.

I think that I can start to work independently of the controller to
display the generated html. I plan to start to work on generating the
HTML code from and Adium template, which includes parsing the theme
metadata, reading the templates, and merging the template with a given
message to generate HTML code.

After I have that working I'll try to integrate it into the current
conversation panel, as you say it shouldn't be difficult since it
already displays HTML, however it seems that the current
implementation which relies on HTMLEditorKit doesn't support
Javascript and some Adium themes contain Javascript code.

If everything goes as expected then I'll try to add a UI to add new
themes in the options window and a control in the in the conversation
window to select the current theme .

What do you think?, does it sound like a feasible plan?

thanks,
Edgar

···

On Wed, Sep 9, 2009 at 6:19 PM, Yana Stamcheva <yana@sip-communicator.org> wrote:

Hi Edgar,

I was planning to work on these at one moment, but never got around to them.

Adding Adium themes shouldn't be that difficult, as we're already using html
and css. However the modification of the ChatConversationPanel could turn to
be painful. Anyway, I'm very interested in this feature and I'll be more
than happy to help.

Keep in mind that the Swing team is now working on JWebPane and as soon as
it's finished we're probably going to replace the existing JEditorPane that
we're using in the conversation area. It's just that JWebPane would be much
more powerful html component.

Looking forward to your posts on the subject!
Yana

On Sep 9, 2009, at 6:27 PM, Edgar Poce wrote:

Hi,

I just wanted to know whether anyone is working on these issues (645
and 378). I'd like to work on them and I wouldn't like to step on
someone else's toes.
I'm having have fun reading the Adium specification on themes[1] and
if no one is looking into it I'll start to code something soon.

Best regards,
Edgar

[1]
http://trac.adium.im/wiki/CreatingMessageStyles
http://trac.adium.im/wiki/CreatingMessageStyles/Tutorial

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#4

Hi Edgar,

Sounds like a great plan to me!

About the Javascript, yes HTMLEditorKit doesn't support Javascript, but AFAIK we don't have other possibility for now than JEditorPane and HTMLEditorKit, so we should wait for the JWebPane to be finished :frowning: Be aware that some of the css parameters won't be working either. However, I'm sure that we'll still be able to support some of the simpler (but also very nice) Adium themes and it's worth it :slight_smile:

Cheers,
Yana

···

On Sep 10, 2009, at 12:52 PM, Edgar Poce wrote:

Hi Yana,

Thanks for your help, I list bellow the different group of tasks that
I identified.

I think that I can start to work independently of the controller to
display the generated html. I plan to start to work on generating the
HTML code from and Adium template, which includes parsing the theme
metadata, reading the templates, and merging the template with a given
message to generate HTML code.

After I have that working I'll try to integrate it into the current
conversation panel, as you say it shouldn't be difficult since it
already displays HTML, however it seems that the current
implementation which relies on HTMLEditorKit doesn't support
Javascript and some Adium themes contain Javascript code.

If everything goes as expected then I'll try to add a UI to add new
themes in the options window and a control in the in the conversation
window to select the current theme .

What do you think?, does it sound like a feasible plan?

thanks,
Edgar

On Wed, Sep 9, 2009 at 6:19 PM, Yana Stamcheva > <yana@sip-communicator.org> wrote:

Hi Edgar,

I was planning to work on these at one moment, but never got around to them.

Adding Adium themes shouldn't be that difficult, as we're already using html
and css. However the modification of the ChatConversationPanel could turn to
be painful. Anyway, I'm very interested in this feature and I'll be more
than happy to help.

Keep in mind that the Swing team is now working on JWebPane and as soon as
it's finished we're probably going to replace the existing JEditorPane that
we're using in the conversation area. It's just that JWebPane would be much
more powerful html component.

Looking forward to your posts on the subject!
Yana

On Sep 9, 2009, at 6:27 PM, Edgar Poce wrote:

Hi,

I just wanted to know whether anyone is working on these issues (645
and 378). I'd like to work on them and I wouldn't like to step on
someone else's toes.
I'm having have fun reading the Adium specification on themes[1] and
if no one is looking into it I'll start to code something soon.

Best regards,
Edgar

[1]
http://trac.adium.im/wiki/CreatingMessageStyles
http://trac.adium.im/wiki/CreatingMessageStyles/Tutorial

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#5

Hi,

I've implemented most of the first part of the plan discussed
previously. I implemented a few classes which parse Adium metadata and
templates, and generate the corresponding HTML code.

I also tried to integrate the HTML generation with HTMLEditorKit and I
think it will not be possible to include Adium designs out of the box
currently because HTMLEditorKit doesn't support many css styles which
are used by most if not all Adium themes. I tried the most popular
themes and none seem to integrate nice with HTMLEditorKit.

I think Adium html generation can be used but only with a custom theme
specifically written for HTMLEditorKit.

Would it make sense to use Adium html generation with a SC specific
style without support of custom styles until a better html controller
is available? What do you think?

br,
Edgar

ps, I have the code in a small eclipse project with a single swing
screen. I can attach the project with the current code to the issue,
if there's interest.

A brief example of the API to write the generated HTML to a file:

AdiumTheme theme = new AdiumTheme(new File(STYLE));
AdiumConversation conversation = new AdiumConversation(theme, "local");
AdiumSender localSender = conversation.createSender("local");
AdiumSender remoteSender = conversation.createSender("remote");
FileWriter fw = new FileWriter(DESTINATION);
try
{
  fw.append(conversation.getBaseHtml());
  fw.append(conversation.getStyleHtml());
  fw.append(conversation.getHeaderHtml());
  fw.append(localSender.createMessage("Hello Remote!").toHtml());
  fw.append(remoteSender.createMessage("Hello Local!").toHtml());
  // consecutive messages from the same sender needs HTML handling in
the view controller
}
finally
{
  fw.close();
}

···

On Thu, Sep 10, 2009 at 12:11 PM, Yana Stamcheva <yana@sip-communicator.org> wrote:

Hi Edgar,

Sounds like a great plan to me!

About the Javascript, yes HTMLEditorKit doesn't support Javascript, but
AFAIK we don't have other possibility for now than JEditorPane and
HTMLEditorKit, so we should wait for the JWebPane to be finished :frowning: Be aware
that some of the css parameters won't be working either. However, I'm sure
that we'll still be able to support some of the simpler (but also very nice)
Adium themes and it's worth it :slight_smile:

Cheers,
Yana

On Sep 10, 2009, at 12:52 PM, Edgar Poce wrote:

Hi Yana,

Thanks for your help, I list bellow the different group of tasks that
I identified.

I think that I can start to work independently of the controller to
display the generated html. I plan to start to work on generating the
HTML code from and Adium template, which includes parsing the theme
metadata, reading the templates, and merging the template with a given
message to generate HTML code.

After I have that working I'll try to integrate it into the current
conversation panel, as you say it shouldn't be difficult since it
already displays HTML, however it seems that the current
implementation which relies on HTMLEditorKit doesn't support
Javascript and some Adium themes contain Javascript code.

If everything goes as expected then I'll try to add a UI to add new
themes in the options window and a control in the in the conversation
window to select the current theme .

What do you think?, does it sound like a feasible plan?

thanks,
Edgar

On Wed, Sep 9, 2009 at 6:19 PM, Yana Stamcheva >> <yana@sip-communicator.org> wrote:

Hi Edgar,

I was planning to work on these at one moment, but never got around to
them.

Adding Adium themes shouldn't be that difficult, as we're already using
html
and css. However the modification of the ChatConversationPanel could turn
to
be painful. Anyway, I'm very interested in this feature and I'll be more
than happy to help.

Keep in mind that the Swing team is now working on JWebPane and as soon
as
it's finished we're probably going to replace the existing JEditorPane
that
we're using in the conversation area. It's just that JWebPane would be
much
more powerful html component.

Looking forward to your posts on the subject!
Yana

On Sep 9, 2009, at 6:27 PM, Edgar Poce wrote:

Hi,

I just wanted to know whether anyone is working on these issues (645
and 378). I'd like to work on them and I wouldn't like to step on
someone else's toes.
I'm having have fun reading the Adium specification on themes[1] and
if no one is looking into it I'll start to code something soon.

Best regards,
Edgar

[1]
http://trac.adium.im/wiki/CreatingMessageStyles
http://trac.adium.im/wiki/CreatingMessageStyles/Tutorial

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#6

Hi Edgar,

Hi,

I've implemented most of the first part of the plan discussed
previously. I implemented a few classes which parse Adium metadata and
templates, and generate the corresponding HTML code.

Yet? :slight_smile: Sounds like a very good progress to me.

I also tried to integrate the HTML generation with HTMLEditorKit and I
think it will not be possible to include Adium designs out of the box
currently because HTMLEditorKit doesn't support many css styles which
are used by most if not all Adium themes. I tried the most popular
themes and none seem to integrate nice with HTMLEditorKit.

:frowning:

I think Adium html generation can be used but only with a custom theme
specifically written for HTMLEditorKit.

Would it make sense to use Adium html generation with a SC specific
style without support of custom styles until a better html controller
is available? What do you think?

Agree. I think it's good to have the parsing mechanism in place anyway and as soon as we have it we have nothing more to do about it but wait for a better controller. In the meantime I don't see any problem about using it to display some SIP Communicator specific themes, that would fit HTMLEditorKit. Just to be sure, the themes you call specific for HTMLEditorKit would be compliant with the Adium theme rules, right?

br,
Edgar

ps, I have the code in a small eclipse project with a single swing
screen. I can attach the project with the current code to the issue,
if there's interest.

That would be great!

Cheers,
Yana

···

On Sep 21, 2009, at 1:57 PM, Edgar Poce wrote:

A brief example of the API to write the generated HTML to a file:

AdiumTheme theme = new AdiumTheme(new File(STYLE));
AdiumConversation conversation = new AdiumConversation(theme, "local");
AdiumSender localSender = conversation.createSender("local");
AdiumSender remoteSender = conversation.createSender("remote");
FileWriter fw = new FileWriter(DESTINATION);
try
{
fw.append(conversation.getBaseHtml());
fw.append(conversation.getStyleHtml());
fw.append(conversation.getHeaderHtml());
fw.append(localSender.createMessage("Hello Remote!").toHtml());
fw.append(remoteSender.createMessage("Hello Local!").toHtml());
// consecutive messages from the same sender needs HTML handling in
the view controller
}
finally
{
fw.close();
}

On Thu, Sep 10, 2009 at 12:11 PM, Yana Stamcheva > <yana@sip-communicator.org> wrote:

Hi Edgar,

Sounds like a great plan to me!

About the Javascript, yes HTMLEditorKit doesn't support Javascript, but
AFAIK we don't have other possibility for now than JEditorPane and
HTMLEditorKit, so we should wait for the JWebPane to be finished :frowning: Be aware
that some of the css parameters won't be working either. However, I'm sure
that we'll still be able to support some of the simpler (but also very nice)
Adium themes and it's worth it :slight_smile:

Cheers,
Yana

On Sep 10, 2009, at 12:52 PM, Edgar Poce wrote:

Hi Yana,

Thanks for your help, I list bellow the different group of tasks that
I identified.

I think that I can start to work independently of the controller to
display the generated html. I plan to start to work on generating the
HTML code from and Adium template, which includes parsing the theme
metadata, reading the templates, and merging the template with a given
message to generate HTML code.

After I have that working I'll try to integrate it into the current
conversation panel, as you say it shouldn't be difficult since it
already displays HTML, however it seems that the current
implementation which relies on HTMLEditorKit doesn't support
Javascript and some Adium themes contain Javascript code.

If everything goes as expected then I'll try to add a UI to add new
themes in the options window and a control in the in the conversation
window to select the current theme .

What do you think?, does it sound like a feasible plan?

thanks,
Edgar

On Wed, Sep 9, 2009 at 6:19 PM, Yana Stamcheva >>> <yana@sip-communicator.org> wrote:

Hi Edgar,

I was planning to work on these at one moment, but never got around to
them.

Adding Adium themes shouldn't be that difficult, as we're already using
html
and css. However the modification of the ChatConversationPanel could turn
to
be painful. Anyway, I'm very interested in this feature and I'll be more
than happy to help.

Keep in mind that the Swing team is now working on JWebPane and as soon
as
it's finished we're probably going to replace the existing JEditorPane
that
we're using in the conversation area. It's just that JWebPane would be
much
more powerful html component.

Looking forward to your posts on the subject!
Yana

On Sep 9, 2009, at 6:27 PM, Edgar Poce wrote:

Hi,

I just wanted to know whether anyone is working on these issues (645
and 378). I'd like to work on them and I wouldn't like to step on
someone else's toes.
I'm having have fun reading the Adium specification on themes[1] and
if no one is looking into it I'll start to code something soon.

Best regards,
Edgar

[1]
http://trac.adium.im/wiki/CreatingMessageStyles
http://trac.adium.im/wiki/CreatingMessageStyles/Tutorial

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#7

Hi Yana,

I attached the current code to the issue #378, It's a work in
progress, I'll prepare a new version soon with fixes plus a SC
specific Adium Message Style.

···

On Wed, Sep 23, 2009 at 5:43 PM, Yana Stamcheva <yana@sip-communicator.org> wrote:

Would it make sense to use Adium html generation with a SC specific
style without support of custom styles until a better html controller
is available? What do you think?

Agree. I think it's good to have the parsing mechanism in place anyway and
as soon as we have it we have nothing more to do about it but wait for a
better controller. In the meantime I don't see any problem about using it to
display some SIP Communicator specific themes, that would fit
HTMLEditorKit. Just to be sure, the themes you call specific for
HTMLEditorKit would be compliant with the Adium theme rules, right?

yes, that's the idea, an Adium Message Style which only uses html plus
styles supported by HTMLEditorKit.

br,
Edgar

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#8

Hi Yana,

Sorry for the delay, I started to work on the Adium integration.
Before integrating the Adium HTML generation I prepared a patch with a
refactor of the current implementation, I made some changes that will
facilitate further work on adium integration.

The objectives of chatpanelrefactor.diff are the following:
1. Create an abstract contract for HTML generation (AbstractSIPHTML... files).
2. Move the current html generation to new subclasses (SIPChatHTML... files).
3. If the proposed change is accepted then create a patch which adds
new classes that provide HTML generation from Adium templates.

The patch modifies the following files:

in src/net/java/sip/communicator/impl/gui/main/chat:
new classes:
AbstractSIPHTMLDocument.java: abstract HTMLDocument subclass which
provides methods to handle ChatMessage objects
AbstractSIPHTMLEditorKit.java: : HTMLEditorKit that creates a
AbstractSIPHTMLDocument instead of a HTMLDocument
SIPChatHTMLDocument.java: subclass of AbstractSIPHTMLDocument which
handles the ChatMessages with the current html generation
SIPChatHTMLEditorKit.java: subclass of AbstractSIPHTMLEditorKit which
creates a SIPChatHTMLDocument instead of a HTMLDocument

modified:
ChatConversationComponent.java: minor refactor
ChatPanel.java: minor refactor
ChatConversationPanel.java: html generation removed, now placed in the
new classes above

in src/net/java/sip/communicator/impl/gui/main/chat
HistoryWindow.java: minor refactor

Looking forward to your comments,
Edgar

···

On Thu, Sep 24, 2009 at 6:22 PM, Edgar Poce <edgarpoce@gmail.com> wrote:

Hi Yana,

I attached the current code to the issue #378, It's a work in
progress, I'll prepare a new version soon with fixes plus a SC
specific Adium Message Style.

On Wed, Sep 23, 2009 at 5:43 PM, Yana Stamcheva > <yana@sip-communicator.org> wrote:

Would it make sense to use Adium html generation with a SC specific
style without support of custom styles until a better html controller
is available? What do you think?

Agree. I think it's good to have the parsing mechanism in place anyway and
as soon as we have it we have nothing more to do about it but wait for a
better controller. In the meantime I don't see any problem about using it to
display some SIP Communicator specific themes, that would fit
HTMLEditorKit. Just to be sure, the themes you call specific for
HTMLEditorKit would be compliant with the Adium theme rules, right?

yes, that's the idea, an Adium Message Style which only uses html plus
styles supported by HTMLEditorKit.

br,
Edgar

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#9

Hi Edgar,

Hi Yana,

Sorry for the delay, I started to work on the Adium integration.

Before integrating the Adium HTML generation I prepared a patch with a
refactor of the current implementation, I made some changes that will
facilitate further work on adium integration.

It's great to hear that you're making a good progress!

The objectives of chatpanelrefactor.diff are the following:
1. Create an abstract contract for HTML generation (AbstractSIPHTML... files).
2. Move the current html generation to new subclasses (SIPChatHTML... files).
3. If the proposed change is accepted then create a patch which adds
new classes that provide HTML generation from Adium templates.

I've revised your patch and it looks very good to me! I have some questions and comments though. See below:

(in no particular order)

- Just a suggestion..As the new AbstractSIPHTMLDocument and AbstractSIPHTMLEditorKit classes look quite generic I think it could be a good idea to move them to the net.java.sip.communicator.util.swing package. The same goes for SIPChatHTMLDocument/EditorKit. WDYT?

- I'd suggest also to rename AbstractSIPHTML[Document/EditorKit] to AbstractChatHTML[Document/EditorKit]. The thing is that this document is quite specific for chat usage and thus I think that we should precise it in the name. Then if I understand it correctly we could have SIPCommChatHTMLDocument and AdiumChatHTMLDocument extendingn the abstract one. WDYT?

- I was curious why we call clear() in the constructor of the ChatConversationPanel, but then I saw that this is where we initialize the document. May be it would be better to rename it to: initDocument or something like that.

- I know that Lubomir has made some modifications in the ChatConversationPanel recently and was explicitly looking if his changes were integrated in your patch. Almost everything is there, but I've noticed that AbstractSIPHTMLDocument.appendToEnd is different from the old ChatConversationPanel.appendMessageToEnd. The synchronization with "scrollToBottomRunnable" is missing and I was wondering if this was intentional or is just due to the fact that you've made the patch against a version that didn't contain this change?

- In SIPChatHTMLEditorKir you set the following properties:

SIPChatHTMLDocument doc = new SIPChatHTMLDocument();
        doc.setParser(getParser());
        doc.setAsynchronousLoadPriority(4);
        doc.setTokenThreshold(100);

Could you please tell us more about why we choose these values. Adding a comment there would be also of great help for people like me not familiar with document configurations:) Thanks!

- I think you could remove the declaration of START/END_PLAINTEXT_TAG in SIPChatHTMLDocument as they're already declared in the abstract class.

- In the ChatConversationPanel we could now remove the content type constants (HTML_CONTENT_TYPE, TEXT_CONTENT_TYPE) as they're not used any more.

I have just a few more comments about code formatting (for more information on project code convention follow the link: http://www.sip-communicator.org/index.php/Documentation/CodeConvention).

- Please use * instead of explicit class imports.

- In the new files you should add the license comment in the beginning of the file.

- Some javadoc comments are missing, could you please fill them out.

- Finally, please do NOT use the automatic code formatting of your IDE on the existing code. It makes quite a lot modifications in already formatted code, while not always doing a better formatting and also makes it quite difficult to review "real" modifications.

That's it :slight_smile:

Thanks for the great work!

Cheers,
Yana

···

On Oct 22, 2009, at 3:18 AM, Edgar Poce wrote:

The patch modifies the following files:

in src/net/java/sip/communicator/impl/gui/main/chat:
new classes:
AbstractSIPHTMLDocument.java: abstract HTMLDocument subclass which
provides methods to handle ChatMessage objects
AbstractSIPHTMLEditorKit.java: : HTMLEditorKit that creates a
AbstractSIPHTMLDocument instead of a HTMLDocument
SIPChatHTMLDocument.java: subclass of AbstractSIPHTMLDocument which
handles the ChatMessages with the current html generation
SIPChatHTMLEditorKit.java: subclass of AbstractSIPHTMLEditorKit which
creates a SIPChatHTMLDocument instead of a HTMLDocument

modified:
ChatConversationComponent.java: minor refactor
ChatPanel.java: minor refactor
ChatConversationPanel.java: html generation removed, now placed in the
new classes above

in src/net/java/sip/communicator/impl/gui/main/chat
HistoryWindow.java: minor refactor

Looking forward to your comments,
Edgar

On Thu, Sep 24, 2009 at 6:22 PM, Edgar Poce <edgarpoce@gmail.com> > wrote:

Hi Yana,

I attached the current code to the issue #378, It's a work in
progress, I'll prepare a new version soon with fixes plus a SC
specific Adium Message Style.

On Wed, Sep 23, 2009 at 5:43 PM, Yana Stamcheva >> <yana@sip-communicator.org> wrote:

Would it make sense to use Adium html generation with a SC specific
style without support of custom styles until a better html controller
is available? What do you think?

Agree. I think it's good to have the parsing mechanism in place anyway and
as soon as we have it we have nothing more to do about it but wait for a
better controller. In the meantime I don't see any problem about using it to
display some SIP Communicator specific themes, that would fit
HTMLEditorKit. Just to be sure, the themes you call specific for
HTMLEditorKit would be compliant with the Adium theme rules, right?

yes, that's the idea, an Adium Message Style which only uses html plus
styles supported by HTMLEditorKit.

br,
Edgar

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#10

Hi Yana, I added my comments inline

Hi Edgar,

The objectives of chatpanelrefactor.diff are the following:
1. Create an abstract contract for HTML generation (AbstractSIPHTML...
files).
2. Move the current html generation to new subclasses (SIPChatHTML...
files).
3. If the proposed change is accepted then create a patch which adds
new classes that provide HTML generation from Adium templates.

I've revised your patch and it looks very good to me! I have some questions
and comments though. See below:

(in no particular order)

- Just a suggestion..As the new AbstractSIPHTMLDocument and
AbstractSIPHTMLEditorKit classes look quite generic I think it could be a
good idea to move them to the net.java.sip.communicator.util.swing package.
The same goes for SIPChatHTMLDocument/EditorKit. WDYT?

Since these classes have no use outside the chat panel they probably
don't belong to a utils package, however if you foresee any future use
outside the chat panel I can move it to that package.

- I'd suggest also to rename AbstractSIPHTML[Document/EditorKit] to
AbstractChatHTML[Document/EditorKit]. The thing is that this document is
quite specific for chat usage and thus I think that we should precise it in
the name. Then if I understand it correctly we could have
SIPCommChatHTMLDocument and AdiumChatHTMLDocument extendingn the abstract
one. WDYT?

I agree about the renaming. I already have an AdiumChatHTMLDocument
subclass locally but I wanted the refactor to be reviewed before
adding adium specific classes, in order to separate the refactor from
the new functionality in two different patches.

- I was curious why we call clear() in the constructor of the
ChatConversationPanel, but then I saw that this is where we initialize the
document. May be it would be better to rename it to: initDocument or
something like that.

Agreed, I'll rename it. I made a minor change to the "clear" method
and invoked it in the initialization but didn't change the name
accordingly. thanks for noting it,

- I know that Lubomir has made some modifications in the
ChatConversationPanel recently and was explicitly looking if his changes
were integrated in your patch. Almost everything is there, but I've noticed
that AbstractSIPHTMLDocument.appendToEnd is different from the old
ChatConversationPanel.appendMessageToEnd. The synchronization with
"scrollToBottomRunnable" is missing and I was wondering if this was
intentional or is just due to the fact that you've made the patch against a
version that didn't contain this change?

I moved the synchronization from the method because the lock object is
outside reach (ChatConversationPanel.scrollToBottomRunnable instance),
now the synchronization is in the
ChatConversationPanel#processStatusMessage(), and I will add the
synchronization to ChatConversationPanel#processMessage(), these
methods are the entry point to append html in the Panel. Thanks for
noting it.

- In SIPChatHTMLEditorKir you set the following properties:

SIPChatHTMLDocument doc = new SIPChatHTMLDocument();
doc.setParser(getParser());
doc.setAsynchronousLoadPriority(4);
doc.setTokenThreshold(100);

Could you please tell us more about why we choose these values. Adding a
comment there would be also of great help for people like me not familiar
with document configurations:) Thanks!

I am not familiarized either, I've chosen these values because they
are the same options used by the default implementation of
HTMLEditorKit.

- I think you could remove the declaration of START/END_PLAINTEXT_TAG in
SIPChatHTMLDocument as they're already declared in the abstract class.

- In the ChatConversationPanel we could now remove the content type
constants (HTML_CONTENT_TYPE, TEXT_CONTENT_TYPE) as they're not used any
more.

Agreed, I remember that I kept some public constants in order to avoid
impact on other classes and keep the patch smaller an easier to
review, I'll remove the constants.

I have just a few more comments about code formatting (for more information
on project code convention follow the link:
http://www.sip-communicator.org/index.php/Documentation/CodeConvention).

Sorry for my mistake, I had the impression that I could use the
eclipse auto format safely, I'm using the format file downloaded from
the project. Probably it might be useful to update the format file to
match the requirements or use the eclipse checkstyle plugin.

I'll prepare a new patch with the proposed corrections and style modifications.

Thank you very much for your help,
Edgar

···

On Fri, Oct 23, 2009 at 12:29 PM, Yana Stamcheva <yana@sip-communicator.org> wrote:

On Oct 22, 2009, at 3:18 AM, Edgar Poce wrote:

- Please use * instead of explicit class imports.

- In the new files you should add the license comment in the beginning of
the file.

- Some javadoc comments are missing, could you please fill them out.

- Finally, please do NOT use the automatic code formatting of your IDE on
the existing code. It makes quite a lot modifications in already formatted
code, while not always doing a better formatting and also makes it quite
difficult to review "real" modifications.

That's it :slight_smile:

Thanks for the great work!

Cheers,
Yana

The patch modifies the following files:

in src/net/java/sip/communicator/impl/gui/main/chat:
new classes:
AbstractSIPHTMLDocument.java: abstract HTMLDocument subclass which
provides methods to handle ChatMessage objects
AbstractSIPHTMLEditorKit.java: : HTMLEditorKit that creates a
AbstractSIPHTMLDocument instead of a HTMLDocument
SIPChatHTMLDocument.java: subclass of AbstractSIPHTMLDocument which
handles the ChatMessages with the current html generation
SIPChatHTMLEditorKit.java: subclass of AbstractSIPHTMLEditorKit which
creates a SIPChatHTMLDocument instead of a HTMLDocument

modified:
ChatConversationComponent.java: minor refactor
ChatPanel.java: minor refactor
ChatConversationPanel.java: html generation removed, now placed in the
new classes above

in src/net/java/sip/communicator/impl/gui/main/chat
HistoryWindow.java: minor refactor

Looking forward to your comments,
Edgar

On Thu, Sep 24, 2009 at 6:22 PM, Edgar Poce <edgarpoce@gmail.com> wrote:

Hi Yana,

I attached the current code to the issue #378, It's a work in
progress, I'll prepare a new version soon with fixes plus a SC
specific Adium Message Style.

On Wed, Sep 23, 2009 at 5:43 PM, Yana Stamcheva >>> <yana@sip-communicator.org> wrote:

Would it make sense to use Adium html generation with a SC specific
style without support of custom styles until a better html controller
is available? What do you think?

Agree. I think it's good to have the parsing mechanism in place anyway
and
as soon as we have it we have nothing more to do about it but wait for a
better controller. In the meantime I don't see any problem about using
it to
display some SIP Communicator specific themes, that would fit
HTMLEditorKit. Just to be sure, the themes you call specific for
HTMLEditorKit would be compliant with the Adium theme rules, right?

yes, that's the idea, an Adium Message Style which only uses html plus
styles supported by HTMLEditorKit.

br,
Edgar

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#11

Hi Yana, I submitted a new patch with the proposed changes. I'll start
to work on the integration of HTML generation from Adium templates.

Best regards,
Edgar

···

On Fri, Oct 23, 2009 at 1:16 PM, Edgar Poce <edgarpoce@gmail.com> wrote:

Hi Yana, I added my comments inline

On Fri, Oct 23, 2009 at 12:29 PM, Yana Stamcheva > <yana@sip-communicator.org> wrote:

Hi Edgar,

On Oct 22, 2009, at 3:18 AM, Edgar Poce wrote:

The objectives of chatpanelrefactor.diff are the following:
1. Create an abstract contract for HTML generation (AbstractSIPHTML...
files).
2. Move the current html generation to new subclasses (SIPChatHTML...
files).
3. If the proposed change is accepted then create a patch which adds
new classes that provide HTML generation from Adium templates.

I've revised your patch and it looks very good to me! I have some questions
and comments though. See below:

(in no particular order)

- Just a suggestion..As the new AbstractSIPHTMLDocument and
AbstractSIPHTMLEditorKit classes look quite generic I think it could be a
good idea to move them to the net.java.sip.communicator.util.swing package.
The same goes for SIPChatHTMLDocument/EditorKit. WDYT?

Since these classes have no use outside the chat panel they probably
don't belong to a utils package, however if you foresee any future use
outside the chat panel I can move it to that package.

- I'd suggest also to rename AbstractSIPHTML[Document/EditorKit] to
AbstractChatHTML[Document/EditorKit]. The thing is that this document is
quite specific for chat usage and thus I think that we should precise it in
the name. Then if I understand it correctly we could have
SIPCommChatHTMLDocument and AdiumChatHTMLDocument extendingn the abstract
one. WDYT?

I agree about the renaming. I already have an AdiumChatHTMLDocument
subclass locally but I wanted the refactor to be reviewed before
adding adium specific classes, in order to separate the refactor from
the new functionality in two different patches.

- I was curious why we call clear() in the constructor of the
ChatConversationPanel, but then I saw that this is where we initialize the
document. May be it would be better to rename it to: initDocument or
something like that.

Agreed, I'll rename it. I made a minor change to the "clear" method
and invoked it in the initialization but didn't change the name
accordingly. thanks for noting it,

- I know that Lubomir has made some modifications in the
ChatConversationPanel recently and was explicitly looking if his changes
were integrated in your patch. Almost everything is there, but I've noticed
that AbstractSIPHTMLDocument.appendToEnd is different from the old
ChatConversationPanel.appendMessageToEnd. The synchronization with
"scrollToBottomRunnable" is missing and I was wondering if this was
intentional or is just due to the fact that you've made the patch against a
version that didn't contain this change?

I moved the synchronization from the method because the lock object is
outside reach (ChatConversationPanel.scrollToBottomRunnable instance),
now the synchronization is in the
ChatConversationPanel#processStatusMessage(), and I will add the
synchronization to ChatConversationPanel#processMessage(), these
methods are the entry point to append html in the Panel. Thanks for
noting it.

- In SIPChatHTMLEditorKir you set the following properties:

SIPChatHTMLDocument doc = new SIPChatHTMLDocument();
doc.setParser(getParser());
doc.setAsynchronousLoadPriority(4);
doc.setTokenThreshold(100);

Could you please tell us more about why we choose these values. Adding a
comment there would be also of great help for people like me not familiar
with document configurations:) Thanks!

I am not familiarized either, I've chosen these values because they
are the same options used by the default implementation of
HTMLEditorKit.

- I think you could remove the declaration of START/END_PLAINTEXT_TAG in
SIPChatHTMLDocument as they're already declared in the abstract class.

- In the ChatConversationPanel we could now remove the content type
constants (HTML_CONTENT_TYPE, TEXT_CONTENT_TYPE) as they're not used any
more.

Agreed, I remember that I kept some public constants in order to avoid
impact on other classes and keep the patch smaller an easier to
review, I'll remove the constants.

I have just a few more comments about code formatting (for more information
on project code convention follow the link:
http://www.sip-communicator.org/index.php/Documentation/CodeConvention).

Sorry for my mistake, I had the impression that I could use the
eclipse auto format safely, I'm using the format file downloaded from
the project. Probably it might be useful to update the format file to
match the requirements or use the eclipse checkstyle plugin.

I'll prepare a new patch with the proposed corrections and style modifications.

Thank you very much for your help,
Edgar

- Please use * instead of explicit class imports.

- In the new files you should add the license comment in the beginning of
the file.

- Some javadoc comments are missing, could you please fill them out.

- Finally, please do NOT use the automatic code formatting of your IDE on
the existing code. It makes quite a lot modifications in already formatted
code, while not always doing a better formatting and also makes it quite
difficult to review "real" modifications.

That's it :slight_smile:

Thanks for the great work!

Cheers,
Yana

The patch modifies the following files:

in src/net/java/sip/communicator/impl/gui/main/chat:
new classes:
AbstractSIPHTMLDocument.java: abstract HTMLDocument subclass which
provides methods to handle ChatMessage objects
AbstractSIPHTMLEditorKit.java: : HTMLEditorKit that creates a
AbstractSIPHTMLDocument instead of a HTMLDocument
SIPChatHTMLDocument.java: subclass of AbstractSIPHTMLDocument which
handles the ChatMessages with the current html generation
SIPChatHTMLEditorKit.java: subclass of AbstractSIPHTMLEditorKit which
creates a SIPChatHTMLDocument instead of a HTMLDocument

modified:
ChatConversationComponent.java: minor refactor
ChatPanel.java: minor refactor
ChatConversationPanel.java: html generation removed, now placed in the
new classes above

in src/net/java/sip/communicator/impl/gui/main/chat
HistoryWindow.java: minor refactor

Looking forward to your comments,
Edgar

On Thu, Sep 24, 2009 at 6:22 PM, Edgar Poce <edgarpoce@gmail.com> wrote:

Hi Yana,

I attached the current code to the issue #378, It's a work in
progress, I'll prepare a new version soon with fixes plus a SC
specific Adium Message Style.

On Wed, Sep 23, 2009 at 5:43 PM, Yana Stamcheva >>>> <yana@sip-communicator.org> wrote:

Would it make sense to use Adium html generation with a SC specific
style without support of custom styles until a better html controller
is available? What do you think?

Agree. I think it's good to have the parsing mechanism in place anyway
and
as soon as we have it we have nothing more to do about it but wait for a
better controller. In the meantime I don't see any problem about using
it to
display some SIP Communicator specific themes, that would fit
HTMLEditorKit. Just to be sure, the themes you call specific for
HTMLEditorKit would be compliant with the Adium theme rules, right?

yes, that's the idea, an Adium Message Style which only uses html plus
styles supported by HTMLEditorKit.

br,
Edgar

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#12

Hi Edgar,

Hi Yana, I added my comments inline

Hi Edgar,

The objectives of chatpanelrefactor.diff are the following:
1. Create an abstract contract for HTML generation (AbstractSIPHTML...
files).
2. Move the current html generation to new subclasses (SIPChatHTML...
files).
3. If the proposed change is accepted then create a patch which adds
new classes that provide HTML generation from Adium templates.

I've revised your patch and it looks very good to me! I have some questions
and comments though. See below:

(in no particular order)

- Just a suggestion..As the new AbstractSIPHTMLDocument and
AbstractSIPHTMLEditorKit classes look quite generic I think it could be a
good idea to move them to the net.java.sip.communicator.util.swing package.
The same goes for SIPChatHTMLDocument/EditorKit. WDYT?

Since these classes have no use outside the chat panel they probably
don't belong to a utils package, however if you foresee any future use
outside the chat panel I can move it to that package.

I just thought that if they don't depend on any of the existing user interface components it would be wise to put them in the util package. Thus they could be used in a plugin that would like to integrate a chat component for example.

- I'd suggest also to rename AbstractSIPHTML[Document/EditorKit] to
AbstractChatHTML[Document/EditorKit]. The thing is that this document is
quite specific for chat usage and thus I think that we should precise it in
the name. Then if I understand it correctly we could have
SIPCommChatHTMLDocument and AdiumChatHTMLDocument extendingn the abstract
one. WDYT?

I agree about the renaming. I already have an AdiumChatHTMLDocument
subclass locally but I wanted the refactor to be reviewed before
adding adium specific classes, in order to separate the refactor from
the new functionality in two different patches.

- I was curious why we call clear() in the constructor of the
ChatConversationPanel, but then I saw that this is where we initialize the
document. May be it would be better to rename it to: initDocument or
something like that.

Agreed, I'll rename it. I made a minor change to the "clear" method
and invoked it in the initialization but didn't change the name
accordingly. thanks for noting it,

- I know that Lubomir has made some modifications in the
ChatConversationPanel recently and was explicitly looking if his changes
were integrated in your patch. Almost everything is there, but I've noticed
that AbstractSIPHTMLDocument.appendToEnd is different from the old
ChatConversationPanel.appendMessageToEnd. The synchronization with
"scrollToBottomRunnable" is missing and I was wondering if this was
intentional or is just due to the fact that you've made the patch against a
version that didn't contain this change?

I moved the synchronization from the method because the lock object is
outside reach (ChatConversationPanel.scrollToBottomRunnable instance),
now the synchronization is in the
ChatConversationPanel#processStatusMessage(), and I will add the
synchronization to ChatConversationPanel#processMessage(), these
methods are the entry point to append html in the Panel. Thanks for
noting it.

Ok

- In SIPChatHTMLEditorKir you set the following properties:

SIPChatHTMLDocument doc = new SIPChatHTMLDocument();
       doc.setParser(getParser());
       doc.setAsynchronousLoadPriority(4);
       doc.setTokenThreshold(100);

Could you please tell us more about why we choose these values. Adding a
comment there would be also of great help for people like me not familiar
with document configurations:) Thanks!

I am not familiarized either, I've chosen these values because they
are the same options used by the default implementation of
HTMLEditorKit.

Wise:)

- I think you could remove the declaration of START/END_PLAINTEXT_TAG in
SIPChatHTMLDocument as they're already declared in the abstract class.

- In the ChatConversationPanel we could now remove the content type
constants (HTML_CONTENT_TYPE, TEXT_CONTENT_TYPE) as they're not used any
more.

Agreed, I remember that I kept some public constants in order to avoid
impact on other classes and keep the patch smaller an easier to
review, I'll remove the constants.

I have just a few more comments about code formatting (for more information
on project code convention follow the link:
http://www.sip-communicator.org/index.php/Documentation/CodeConvention).

Sorry for my mistake, I had the impression that I could use the
eclipse auto format safely, I'm using the format file downloaded from
the project. Probably it might be useful to update the format file to
match the requirements or use the eclipse checkstyle plugin.

I'm using the same formatter and it's ok to continue using it. The thing is that on existing code if we make "format" it will add arbitrary line returns based on some obscure assumptions :slight_smile: (Just added a note on our code conventions page.)

Thanks again for the hard work!

Cheers,
Yana

···

On Oct 23, 2009, at 2:16 PM, Edgar Poce wrote:

On Fri, Oct 23, 2009 at 12:29 PM, Yana Stamcheva > <yana@sip-communicator.org> wrote:

On Oct 22, 2009, at 3:18 AM, Edgar Poce wrote:

I'll prepare a new patch with the proposed corrections and style modifications.

Thank you very much for your help,
Edgar

- Please use * instead of explicit class imports.

- In the new files you should add the license comment in the beginning of
the file.

- Some javadoc comments are missing, could you please fill them out.

- Finally, please do NOT use the automatic code formatting of your IDE on
the existing code. It makes quite a lot modifications in already formatted
code, while not always doing a better formatting and also makes it quite
difficult to review "real" modifications.

That's it :slight_smile:

Thanks for the great work!

Cheers,
Yana

The patch modifies the following files:

in src/net/java/sip/communicator/impl/gui/main/chat:
new classes:
AbstractSIPHTMLDocument.java: abstract HTMLDocument subclass which
provides methods to handle ChatMessage objects
AbstractSIPHTMLEditorKit.java: : HTMLEditorKit that creates a
AbstractSIPHTMLDocument instead of a HTMLDocument
SIPChatHTMLDocument.java: subclass of AbstractSIPHTMLDocument which
handles the ChatMessages with the current html generation
SIPChatHTMLEditorKit.java: subclass of AbstractSIPHTMLEditorKit which
creates a SIPChatHTMLDocument instead of a HTMLDocument

modified:
ChatConversationComponent.java: minor refactor
ChatPanel.java: minor refactor
ChatConversationPanel.java: html generation removed, now placed in the
new classes above

in src/net/java/sip/communicator/impl/gui/main/chat
HistoryWindow.java: minor refactor

Looking forward to your comments,
Edgar

On Thu, Sep 24, 2009 at 6:22 PM, Edgar Poce <edgarpoce@gmail.com> >>> wrote:

Hi Yana,

I attached the current code to the issue #378, It's a work in
progress, I'll prepare a new version soon with fixes plus a SC
specific Adium Message Style.

On Wed, Sep 23, 2009 at 5:43 PM, Yana Stamcheva >>>> <yana@sip-communicator.org> wrote:

Would it make sense to use Adium html generation with a SC specific
style without support of custom styles until a better html controller
is available? What do you think?

Agree. I think it's good to have the parsing mechanism in place anyway
and
as soon as we have it we have nothing more to do about it but wait for a
better controller. In the meantime I don't see any problem about using
it to
display some SIP Communicator specific themes, that would fit
HTMLEditorKit. Just to be sure, the themes you call specific for
HTMLEditorKit would be compliant with the Adium theme rules, right?

yes, that's the idea, an Adium Message Style which only uses html plus
styles supported by HTMLEditorKit.

br,
Edgar

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#13

That was fast :slight_smile: I'll try to integrate it this weekend then.

Thanks!
Yana

···

On Oct 24, 2009, at 4:10 AM, Edgar Poce wrote:

Hi Yana, I submitted a new patch with the proposed changes. I'll start
to work on the integration of HTML generation from Adium templates.

Best regards,
Edgar

On Fri, Oct 23, 2009 at 1:16 PM, Edgar Poce <edgarpoce@gmail.com> > wrote:

Hi Yana, I added my comments inline

On Fri, Oct 23, 2009 at 12:29 PM, Yana Stamcheva >> <yana@sip-communicator.org> wrote:

Hi Edgar,

On Oct 22, 2009, at 3:18 AM, Edgar Poce wrote:

The objectives of chatpanelrefactor.diff are the following:
1. Create an abstract contract for HTML generation (AbstractSIPHTML...
files).
2. Move the current html generation to new subclasses (SIPChatHTML...
files).
3. If the proposed change is accepted then create a patch which adds
new classes that provide HTML generation from Adium templates.

I've revised your patch and it looks very good to me! I have some questions
and comments though. See below:

(in no particular order)

- Just a suggestion..As the new AbstractSIPHTMLDocument and
AbstractSIPHTMLEditorKit classes look quite generic I think it could be a
good idea to move them to the net.java.sip.communicator.util.swing package.
The same goes for SIPChatHTMLDocument/EditorKit. WDYT?

Since these classes have no use outside the chat panel they probably
don't belong to a utils package, however if you foresee any future use
outside the chat panel I can move it to that package.

- I'd suggest also to rename AbstractSIPHTML[Document/EditorKit] to
AbstractChatHTML[Document/EditorKit]. The thing is that this document is
quite specific for chat usage and thus I think that we should precise it in
the name. Then if I understand it correctly we could have
SIPCommChatHTMLDocument and AdiumChatHTMLDocument extendingn the abstract
one. WDYT?

I agree about the renaming. I already have an AdiumChatHTMLDocument
subclass locally but I wanted the refactor to be reviewed before
adding adium specific classes, in order to separate the refactor from
the new functionality in two different patches.

- I was curious why we call clear() in the constructor of the
ChatConversationPanel, but then I saw that this is where we initialize the
document. May be it would be better to rename it to: initDocument or
something like that.

Agreed, I'll rename it. I made a minor change to the "clear" method
and invoked it in the initialization but didn't change the name
accordingly. thanks for noting it,

- I know that Lubomir has made some modifications in the
ChatConversationPanel recently and was explicitly looking if his changes
were integrated in your patch. Almost everything is there, but I've noticed
that AbstractSIPHTMLDocument.appendToEnd is different from the old
ChatConversationPanel.appendMessageToEnd. The synchronization with
"scrollToBottomRunnable" is missing and I was wondering if this was
intentional or is just due to the fact that you've made the patch against a
version that didn't contain this change?

I moved the synchronization from the method because the lock object is
outside reach (ChatConversationPanel.scrollToBottomRunnable instance),
now the synchronization is in the
ChatConversationPanel#processStatusMessage(), and I will add the
synchronization to ChatConversationPanel#processMessage(), these
methods are the entry point to append html in the Panel. Thanks for
noting it.

- In SIPChatHTMLEditorKir you set the following properties:

SIPChatHTMLDocument doc = new SIPChatHTMLDocument();
       doc.setParser(getParser());
       doc.setAsynchronousLoadPriority(4);
       doc.setTokenThreshold(100);

Could you please tell us more about why we choose these values. Adding a
comment there would be also of great help for people like me not familiar
with document configurations:) Thanks!

I am not familiarized either, I've chosen these values because they
are the same options used by the default implementation of
HTMLEditorKit.

- I think you could remove the declaration of START/END_PLAINTEXT_TAG in
SIPChatHTMLDocument as they're already declared in the abstract class.

- In the ChatConversationPanel we could now remove the content type
constants (HTML_CONTENT_TYPE, TEXT_CONTENT_TYPE) as they're not used any
more.

Agreed, I remember that I kept some public constants in order to avoid
impact on other classes and keep the patch smaller an easier to
review, I'll remove the constants.

I have just a few more comments about code formatting (for more information
on project code convention follow the link:
http://www.sip-communicator.org/index.php/Documentation/CodeConvention).

Sorry for my mistake, I had the impression that I could use the
eclipse auto format safely, I'm using the format file downloaded from
the project. Probably it might be useful to update the format file to
match the requirements or use the eclipse checkstyle plugin.

I'll prepare a new patch with the proposed corrections and style modifications.

Thank you very much for your help,
Edgar

- Please use * instead of explicit class imports.

- In the new files you should add the license comment in the beginning of
the file.

- Some javadoc comments are missing, could you please fill them out.

- Finally, please do NOT use the automatic code formatting of your IDE on
the existing code. It makes quite a lot modifications in already formatted
code, while not always doing a better formatting and also makes it quite
difficult to review "real" modifications.

That's it :slight_smile:

Thanks for the great work!

Cheers,
Yana

The patch modifies the following files:

in src/net/java/sip/communicator/impl/gui/main/chat:
new classes:
AbstractSIPHTMLDocument.java: abstract HTMLDocument subclass which
provides methods to handle ChatMessage objects
AbstractSIPHTMLEditorKit.java: : HTMLEditorKit that creates a
AbstractSIPHTMLDocument instead of a HTMLDocument
SIPChatHTMLDocument.java: subclass of AbstractSIPHTMLDocument which
handles the ChatMessages with the current html generation
SIPChatHTMLEditorKit.java: subclass of AbstractSIPHTMLEditorKit which
creates a SIPChatHTMLDocument instead of a HTMLDocument

modified:
ChatConversationComponent.java: minor refactor
ChatPanel.java: minor refactor
ChatConversationPanel.java: html generation removed, now placed in the
new classes above

in src/net/java/sip/communicator/impl/gui/main/chat
HistoryWindow.java: minor refactor

Looking forward to your comments,
Edgar

On Thu, Sep 24, 2009 at 6:22 PM, Edgar Poce <edgarpoce@gmail.com> >>>> wrote:

Hi Yana,

I attached the current code to the issue #378, It's a work in
progress, I'll prepare a new version soon with fixes plus a SC
specific Adium Message Style.

On Wed, Sep 23, 2009 at 5:43 PM, Yana Stamcheva >>>>> <yana@sip-communicator.org> wrote:

Would it make sense to use Adium html generation with a SC specific
style without support of custom styles until a better html controller
is available? What do you think?

Agree. I think it's good to have the parsing mechanism in place anyway
and
as soon as we have it we have nothing more to do about it but wait for a
better controller. In the meantime I don't see any problem about using
it to
display some SIP Communicator specific themes, that would fit
HTMLEditorKit. Just to be sure, the themes you call specific for
HTMLEditorKit would be compliant with the Adium theme rules, right?

yes, that's the idea, an Adium Message Style which only uses html plus
styles supported by HTMLEditorKit.

br,
Edgar

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#14

Dear All,
as Emil suggested I have tried with different settings (enable/disable SIMPLE mode) to
check connection using sipphone and iptel accounts (connection is between Germany and
Austria)

a) Both partners are shown offline although both are online (we can call each other although
connection is not good. so it seems not to depend on SIMPLE modus.

Question: which transport mode is the most preferred for SC (UDP/TLS/TCP ?) - I have
selected UDP with both SIP accounts.

b) zRTP with audio is not working (both accounts; same with video) - according to roadmap
it is not ready yet (if I interpret correctly)

c) as mentioned earlier in the forum by Szautner Bela: first call (only sometimes) can be
received, later on no connection possible, also arriving calls cannot be accepted (ringing
further)

d) repeating asking passwords for SIP account (especially with iptel very often, sipphone
sometimes) - here the (correct!) password gets deleted if message is ignored. However it
helps if after ignoring message the password is set again in the account settings, then
the connection keeps longer.

e) presence/absence with jabber-account works very well (if partner turns on/off SC the
status is immediately indicated).

kind regards, MS

···

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#15

Hi MS,

some remarks inline.

Do you or the other party have a way to do some protocol traces? I know you are
not a programmer, however for the Mac it is possible to use a tool called wireshark
that can trace the network traffic.

Regards,
Werner

Mr Smith schrieb:

Dear All,
as Emil suggested I have tried with different settings (enable/disable SIMPLE mode) to
check connection using sipphone and iptel accounts (connection is between Germany and
Austria)

a) Both partners are shown offline although both are online (we can call each other although
connection is not good. so it seems not to depend on SIMPLE modus.

Question: which transport mode is the most preferred for SC (UDP/TLS/TCP ?) - I have
selected UDP with both SIP accounts.

Stay with UDP - other transports are not well supported by many other clients and SIP servers
that manage your accounts. The audio data always use UDP, you cannot select another transports.

To reduce the required bandwidht you may try to select another codec (assuming you set SC
to German) go to Werkzeuge -> Einstellungen, select Media and try another codec. the GSM
code uses less bandwidth for example (disable ALAW, ULAW that use mor bandwidth). Of course
the other parties's client must support these codecs as well.

b) zRTP with audio is not working (both accounts; same with video) - according to roadmap
it is not ready yet (if I interpret correctly)

ZRTP works only if both parties support this. I assume that the other party does not use SC?
If you like to use ZRTP with a client that does not have native support of ZRTP then I would
propose to use Phil Zimmermann's "bump-in-the-wire" implementation - give me a ping if you
need more info on that.

···

c) as mentioned earlier in the forum by Szautner Bela: first call (only sometimes) can be
received, later on no connection possible, also arriving calls cannot be accepted (ringing
further)

d) repeating asking passwords for SIP account (especially with iptel very often, sipphone
sometimes) - here the (correct!) password gets deleted if message is ignored. However it
helps if after ignoring message the password is set again in the account settings, then
the connection keeps longer.

e) presence/absence with jabber-account works very well (if partner turns on/off SC the
status is immediately indicated).

kind regards, MS

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#16

Hey MS,

Mr Smith wrote:

Dear All, as Emil suggested I have tried with different settings
(enable/disable SIMPLE mode) to check connection using sipphone and
iptel accounts (connection is between Germany and Austria)

Actually, I was wondering whether the sipphone.com server supports this.
Is it working with other clients? If yes then could you please open an
issue in our tracker? (A wireshark trace would be very helpful)

c) as mentioned earlier in the forum by Szautner Bela: first call
(only sometimes) can be received, later on no connection possible,
also arriving calls cannot be accepted (ringing further)

We'd need the SC logs and (if possible) a wireshark dump in order to
diagnose this one.

d) repeating asking passwords for SIP account (especially with iptel
very often, sipphone sometimes) - here the (correct!) password gets
deleted if message is ignored. However it helps if after ignoring
message the password is set again in the account settings, then the
connection keeps longer.

Yes, I believe I already mentioned this is a known issue. Could you
please open an issue in our tracker?

e) presence/absence with jabber-account works very well (if partner
turns on/off SC the status is immediately indicated).

Cool! Thanks for the heads up!

Cheers,
Emil

···

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#17

Hi Werner,
I disabled now ALAW and ULAW codecs (on both SC clients) - sound quality improved
significantly (not interrupting anymore; video not working yet).
Also connection is made quite quicklybbut zRTP is not working (using sipphone; other partner could not go online with iptel today -
we both use sip-communicator latest version on WinXP SP2).

another (different) partner using MacOS was to test jabber+otr only. He is not using SC.

I have zfone installed (0.92 build 218) - if you could send me some advice to get it
running with SC I would be very happy (using both zRTP enabled on SC and active Zfone will
deliver error message. So I shut down zfone while using SC).
Did not test yet lates Zfone3 with SC.

kind regards, MS

···

Hi MS,

some remarks inline.

Do you or the other party have a way to do some protocol traces? I know you are
not a programmer, however for the Mac it is possible to use a tool called wireshark
that can trace the network traffic.

Regards,
Werner

Mr Smith schrieb:

Dear All,
as Emil suggested I have tried with different settings (enable/disable SIMPLE mode) to
check connection using sipphone and iptel accounts (connection is between Germany and
Austria)

a) Both partners are shown offline although both are online (we can call each other although
connection is not good. so it seems not to depend on SIMPLE modus.

Question: which transport mode is the most preferred for SC (UDP/TLS/TCP ?) - I have
selected UDP with both SIP accounts.

Stay with UDP - other transports are not well supported by many other clients and SIP servers
that manage your accounts. The audio data always use UDP, you cannot select another transports.

To reduce the required bandwidht you may try to select another codec (assuming you set SC
to German) go to Werkzeuge -> Einstellungen, select Media and try another codec. the GSM
code uses less bandwidth for example (disable ALAW, ULAW that use mor bandwidth). Of course
the other parties's client must support these codecs as well.

b) zRTP with audio is not working (both accounts; same with video) - according to roadmap
it is not ready yet (if I interpret correctly)

ZRTP works only if both parties support this. I assume that the other party does not use SC?
If you like to use ZRTP with a client that does not have native support of ZRTP then I would
propose to use Phil Zimmermann's "bump-in-the-wire" implementation - give me a ping if you
need more info on that.

c) as mentioned earlier in the forum by Szautner Bela: first call (only sometimes) can be
received, later on no connection possible, also arriving calls cannot be accepted (ringing
further)

d) repeating asking passwords for SIP account (especially with iptel very often, sipphone
sometimes) - here the (correct!) password gets deleted if message is ignored. However it
helps if after ignoring message the password is set again in the account settings, then
the connection keeps longer.

e) presence/absence with jabber-account works very well (if partner turns on/off SC the
status is immediately indicated).

kind regards, MS

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

--
Mit freundlichen Grüßen
Mr Smith
mailto:mr.smith476@googlemail.com

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#18

MS,

which error messages do you see for ZRTP? Maybe that can help to hunt the
bug :slight_smile: .

If you have problems with audio because of "low bandwidth" then
you may have even more problems with video - bandwidth requirement
for video is higher than for audio.

Regards,
Werner

Mr Smith schrieb:

···

Hi Werner,
I disabled now ALAW and ULAW codecs (on both SC clients) - sound quality improved
significantly (not interrupting anymore; video not working yet).
Also connection is made quite quicklybbut zRTP is not working (using sipphone; other partner could not go online with iptel today -
we both use sip-communicator latest version on WinXP SP2).

another (different) partner using MacOS was to test jabber+otr only. He is not using SC.

I have zfone installed (0.92 build 218) - if you could send me some advice to get it
running with SC I would be very happy (using both zRTP enabled on SC and active Zfone will
deliver error message. So I shut down zfone while using SC).
Did not test yet lates Zfone3 with SC.

kind regards, MS

Hi MS,

some remarks inline.

Do you or the other party have a way to do some protocol traces? I know you are
not a programmer, however for the Mac it is possible to use a tool called wireshark
that can trace the network traffic.

Regards,
Werner

Mr Smith schrieb:

Dear All,
as Emil suggested I have tried with different settings (enable/disable SIMPLE mode) to
check connection using sipphone and iptel accounts (connection is between Germany and
Austria)

a) Both partners are shown offline although both are online (we can call each other although
connection is not good. so it seems not to depend on SIMPLE modus.

Question: which transport mode is the most preferred for SC (UDP/TLS/TCP ?) - I have
selected UDP with both SIP accounts.

Stay with UDP - other transports are not well supported by many other clients and SIP servers
that manage your accounts. The audio data always use UDP, you cannot select another transports.

To reduce the required bandwidht you may try to select another codec (assuming you set SC
to German) go to Werkzeuge -> Einstellungen, select Media and try another codec. the GSM
code uses less bandwidth for example (disable ALAW, ULAW that use mor bandwidth). Of course
the other parties's client must support these codecs as well.

b) zRTP with audio is not working (both accounts; same with video) - according to roadmap
it is not ready yet (if I interpret correctly)

ZRTP works only if both parties support this. I assume that the other party does not use SC?
If you like to use ZRTP with a client that does not have native support of ZRTP then I would
propose to use Phil Zimmermann's "bump-in-the-wire" implementation - give me a ping if you
need more info on that.

c) as mentioned earlier in the forum by Szautner Bela: first call (only sometimes) can be
received, later on no connection possible, also arriving calls cannot be accepted (ringing
further)

d) repeating asking passwords for SIP account (especially with iptel very often, sipphone
sometimes) - here the (correct!) password gets deleted if message is ignored. However it
helps if after ignoring message the password is set again in the account settings, then
the connection keeps longer.

e) presence/absence with jabber-account works very well (if partner turns on/off SC the
status is immediately indicated).

kind regards, MS

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#19

MS,

just forgot: if Zfone is installed: make sure that *Zfone is not active* in parallel
to SC with ZRTP enabled. This will definitely not work. Both implementations
distrurb each other.

Werner

Mr Smith schrieb:

···

Hi Werner,
I disabled now ALAW and ULAW codecs (on both SC clients) - sound quality improved
significantly (not interrupting anymore; video not working yet).
Also connection is made quite quicklybbut zRTP is not working (using sipphone; other partner could not go online with iptel today -
we both use sip-communicator latest version on WinXP SP2).

another (different) partner using MacOS was to test jabber+otr only. He is not using SC.

I have zfone installed (0.92 build 218) - if you could send me some advice to get it
running with SC I would be very happy (using both zRTP enabled on SC and active Zfone will
deliver error message. So I shut down zfone while using SC).
Did not test yet lates Zfone3 with SC.

kind regards, MS

Hi MS,

some remarks inline.

Do you or the other party have a way to do some protocol traces? I know you are
not a programmer, however for the Mac it is possible to use a tool called wireshark
that can trace the network traffic.

Regards,
Werner

Mr Smith schrieb:

Dear All,
as Emil suggested I have tried with different settings (enable/disable SIMPLE mode) to
check connection using sipphone and iptel accounts (connection is between Germany and
Austria)

a) Both partners are shown offline although both are online (we can call each other although
connection is not good. so it seems not to depend on SIMPLE modus.

Question: which transport mode is the most preferred for SC (UDP/TLS/TCP ?) - I have
selected UDP with both SIP accounts.

Stay with UDP - other transports are not well supported by many other clients and SIP servers
that manage your accounts. The audio data always use UDP, you cannot select another transports.

To reduce the required bandwidht you may try to select another codec (assuming you set SC
to German) go to Werkzeuge -> Einstellungen, select Media and try another codec. the GSM
code uses less bandwidth for example (disable ALAW, ULAW that use mor bandwidth). Of course
the other parties's client must support these codecs as well.

b) zRTP with audio is not working (both accounts; same with video) - according to roadmap
it is not ready yet (if I interpret correctly)

ZRTP works only if both parties support this. I assume that the other party does not use SC?
If you like to use ZRTP with a client that does not have native support of ZRTP then I would
propose to use Phil Zimmermann's "bump-in-the-wire" implementation - give me a ping if you
need more info on that.

c) as mentioned earlier in the forum by Szautner Bela: first call (only sometimes) can be
received, later on no connection possible, also arriving calls cannot be accepted (ringing
further)

d) repeating asking passwords for SIP account (especially with iptel very often, sipphone
sometimes) - here the (correct!) password gets deleted if message is ignored. However it
helps if after ignoring message the password is set again in the account settings, then
the connection keeps longer.

e) presence/absence with jabber-account works very well (if partner turns on/off SC the
status is immediately indicated).

kind regards, MS

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net