[sip-comm-dev] Call Recording


#21

Dmitri, I committed callRecording.patch and recordButton.png in r7584.
Thank you for your good work on the useful feature that call recording
is!

Here's an incomplete list of the issues I've found thus far:

- We require license headers in all files.

- We require you to @author classes not only when you've created them in
their entirety but also when you add methods, fields.

- Our Eclipse IDE formatter is not 100% in accord with our coding
convention. The best rule of thumb is to use it but to follow the rules
of the existing code which you modify.

- Eclipse IDE puts whitespace in empty lines but we don't want it and
it's easy to discover in the Compare Editor of Eclipse IDE (e.g. the
Team Synchronization perspective shows the changed files and launches
the Compare Editor on double click).

I tried to take care of the above but I found them to be a productivity
killer during my review.

- I don't find the Recorder interface and the API related to it in
OperationSetBasicTelephony and MediaAwareCall to be very good.
#startRecording takes a file name so it's not very clear to me what
happens when one decides to call it two times with different file names
without calling #stopRecording. Then #stopRecording doesn't really
identify which #startRecording it corresponds to. It seems to me that
you could've returned a Recorder from
OperationSetBasicTelephony#startRecording(fileName) and then you
could've had Recorder#stop instead of
OperationSetBasicTelephony#stopRecording. Anyway, I'd rather have the
comments of the other community members on it prior to asking you to
modify your code.

- I don't understand the "Save call to..." dialog depicted in the
screenshot save-call-to.png. When I browse for Location, it ask me not
only for a directory but it also requires a filename. Why not directly
open the Save file dialog upon clicking the Record button and have the
supported file formats in its filter?

- The notification displayed in the screenshot
call-saved-successfully.png should use our notification service, it
shouldn't be a dialog. Our notification service displays notifications,
for example, when somebody starts typing an instant message to you in a
new conversation or you receive an instant message.

- You're still using a MediaDeviceSession for the call recording while
you only need a Processor.


#22

Dmitri, here's a request from Emil shared in a private conversation:

We already use file dialogs and there are operating system specifics
with respect to which file dialog implementation gets used on which
operating system because of their different looks and feels. For
example, the file dialog that you currently use for Call Recording on
Mac OS X is not the type we use for "Send file" in chats on Mac OS X.
Please try to use the file dialog implementations in accord with the
operating system as we do elsewhere. In accord with the SIP
Communicator architecture and design and in order to avoid
duplication, if there's a need to implement the file dialog
functionality in a service so that it can be consumed from different
bundles, please introduce it.

···

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


#23

Hey folks,

Dmitri, you've done a great job! Call recording has been very high on
our project wish list for quite a while now. This is really a great
addition to SIP Communicator! Thank you and Lubomir for spending time on
this and doing such a great job. I'd also like to add a couple of issues
here:

* It would be nice to have default names set in the file chooser when
starting a record.
* Record button remains highlighted after turning it off (this is
probably more of an UI issue.
* Every time I start a recording I get the following exception:

javax.media.NoDataSinkException: Cannot find a DataSink for: null
  at javax.media.Manager.createDataSink(Manager.java:1894)
  at net.java.sip.communicator.impl.neomedia.RecorderImpl.startRecording(RecorderImpl.java:88)
  at net.java.sip.communicator.service.protocol.media.MediaAwareCall.startRecording(MediaAwareCall.java:592)
  at net.java.sip.communicator.service.protocol.media.AbstractOperationSetBasicTelephony.startRecording(AbstractOperationSetBasicTelephony.java:140)
  at net.java.sip.communicator.impl.gui.main.call.RecordButton.actionPerformed(RecordButton.java:153)
  at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:1882)
  at javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2202)
  at javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:420)
  at javax.swing.JToggleButton$ToggleButtonModel.setPressed(JToggleButton.java:269)
  at javax.swing.plaf.basic.BasicButtonListener.mouseReleased(BasicButtonListener.java:236)
  at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:231)
  at java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:231)
  at java.awt.Component.processMouseEvent(Component.java:5602)
  at javax.swing.JComponent.processMouseEvent(JComponent.java:3129)
  at java.awt.Component.processEvent(Component.java:5367)
  at java.awt.Container.processEvent(Container.java:2010)
  at java.awt.Component.dispatchEventImpl(Component.java:4068)
  at java.awt.Container.dispatchEventImpl(Container.java:2068)
  at java.awt.Component.dispatchEvent(Component.java:3903)
  at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4256)
  at java.awt.LightweightDispatcher.processMouseEvent(Container.java:3936)
  at java.awt.LightweightDispatcher.dispatchEvent(Container.java:3866)
  at java.awt.Container.dispatchEventImpl(Container.java:2054)
  at java.awt.Window.dispatchEventImpl(Window.java:1801)
  at java.awt.Component.dispatchEvent(Component.java:3903)
  at java.awt.EventQueue.dispatchEvent(EventQueue.java:463)
  at java.awt.EventDispatchThread.pumpOneEventForHierarchy(EventDispatchThread.java:269)
  at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:190)
  at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:184)
  at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:176)
  at java.awt.EventDispatchThread.run(EventDispatchThread.java:110)

Cheers,
Emil

На 12.08.10 13:21, Lubomir Marinov написа:

···

Dmitri, here's a request from Emil shared in a private conversation:

We already use file dialogs and there are operating system specifics
with respect to which file dialog implementation gets used on which
operating system because of their different looks and feels. For
example, the file dialog that you currently use for Call Recording on
Mac OS X is not the type we use for "Send file" in chats on Mac OS X.
Please try to use the file dialog implementations in accord with the
operating system as we do elsewhere. In accord with the SIP
Communicator architecture and design and in order to avoid
duplication, if there's a need to implement the file dialog
functionality in a service so that it can be consumed from different
bundles, please introduce it.

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

--
Emil Ivov, Ph.D. 67000 Strasbourg,
Project Lead France
SIP Communicator
emcho@sip-communicator.org PHONE: +33.1.77.62.43.30
http://sip-communicator.org FAX: +33.1.77.62.47.31

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


#24

Hi all,

Thank you, Lubomir, for doing the integration of my patch and making a
lot of fixes!

- I agree the Recorder API isn't perfect in the sense that it assumes
that startRecording should be always followed by stopRecording.
RecorderImpl has a corresponding internal state, so calling
startRecording the second time does nothing.

- Now that you mention it, the "Save call to..." dialog itself does
seem redundant...

- About the "Save call to..." file chooser. I don't understand how
exactly is it different from everything else, to me the "Send file" in
chats looks the same. Furthemore, in the MainToolBar I see that
GenericFileDialog.create creates this chooser and I use the same. The
only difference is that in the first case it's called with
SipCommFileChooser.LOAD_FILE_OPERATION and I use
SipCommFileChooser.SAVE_FILE_OPERATION.

- About using MediaDeviceSession instead of Processor, I already tried
to refactor that, but found it difficult to accomplish. I can try it
again once the other issues are fixed.

Emil,
That exception is very strange. It's caused by null from
deviceSession#getOutputDataSource() when the processor is null or not
realized. But I don't know why it works on my system and not on
yours... The UI button remains highlighted as a result of this
exception.

Cheers,
Dmitri

···

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


#25

Hi,

I made a patch that changes some UI issues:
- A file chooser dialog for specifying filename istead of a custom
dialog with a file chooser button.
- SC notifications after a call is recorded instead of a popup.

I've sent Emil a log patch to get more details about his reported exception.

And I think I know where the confusion about different look on Mac OS
X comes from. In SipCommFileChooser#create there is a condition that
basically goes like
if(OSUtils.IS_MAC)
// create SipCommFileDialogImpl
else
// create SipCommFileChooserImpl
SipCommFileChooserImpl extends JFileChooser, but SipCommFileDialogImpl
extends java.awt.FileDialog. I don't know why this is done so and I
don't have a Mac to test, but I think this is the reason for different
file choosing dialogs.

Cheers,
Dmitri

callrec-ui.patch (11.1 KB)


#26

Dmitri, I committed callrec-ui.patch in r7588. Thank you for acting
upon my comments!

···

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


#27

Hi, Lubomir.

Thanks for commiting the changes.
I've noticed that in the trunk version of resources.properties the
property CALL_SAVED_TO is
plugin.callrecordingconfig.CALL_SAVED_TO={0}
Unfortunately while constructing the notification popup this causes
the Html2Text.extractText to run the html parser on the file path
string and I suppose it does not like the / character of the path, so
the empty string is returned and nothing is displayed. A simple
solution I found is to just wrap the path with <pre> tag.

Index: src/net/java/sip/communicator/impl/gui/main/call/RecordButton.java

···

===================================================================
--- src/net/java/sip/communicator/impl/gui/main/call/RecordButton.java (revision
7607)
+++ src/net/java/sip/communicator/impl/gui/main/call/RecordButton.java (working
copy)
@@ -183,9 +183,11 @@
                             NotificationManager.CALL_SAVED,
                             resources.getI18NString(
                                     "plugin.callrecordingconfig.CALL_SAVED"),
+ "<pre>" +
                             resources.getI18NString(
                                     "plugin.callrecordingconfig.CALL_SAVED_TO",
- new String[] { callFilename }));
+ new String[] { callFilename }) +
+ "</pre>");
             }
         }
     }

On Mon, Aug 16, 2010 at 3:46 PM, Lubomir Marinov <lubo@sip-communicator.org> wrote:

Dmitri, I committed callrec-ui.patch in r7588. Thank you for acting
upon my comments!

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


#28

Dmitri, it's true that I removed the quotation marks from the
CALL_SAVED_TO property value and the reason was the respective
notification displayed on my Ubuntu had the quotation marks displayed
though they didn't seem necessary at all. Anyway, the problem that you
are reporting now with Html2Text.extractText seems like a deficiency
of PopupMessageHandlerSwingImpl and, consequently, its solution seems
to be appropriate there in the very PopupMessageHandler implementation
instead of where you offer it at the specific use of the abstract
notification service. If you feel that the slash character is a
problem for PopupMessageHandler implementations in general and that
you want to impose a constraint on the PopupMessageHandler or the
NotificationManager in general that the slash characters should always
be escaped in a specific way (e.g. by using the <pre> tag), then you
should also describe the constraint in question in the respective
Javadocs.

···

On Tue, Aug 17, 2010 at 9:40 PM, Dmitri Melnikov <dmitri807@gmail.com> wrote:

I've noticed that in the trunk version of resources.properties the
property CALL_SAVED_TO is
plugin.callrecordingconfig.CALL_SAVED_TO={0}
Unfortunately while constructing the notification popup this causes
the Html2Text.extractText to run the html parser on the file path
string and I suppose it does not like the / character of the path, so
the empty string is returned and nothing is displayed. A simple
solution I found is to just wrap the path with <pre> tag.

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


#29

Hi,

Well, it's not hard to add the <pre> tags to the argument of
Html2Text.extractText in PopupMessageHandlerSwingImpl. But what's
puzzling to me is why Html2Text.extractText is used there at all. It's
function as I understand is to extract text from html code. But is it
necessary? Is there such a case where html code is always passed as a
message and cannot be extracted before?

···

On Wed, Aug 18, 2010 at 1:48 AM, Lubomir Marinov <lubo@sip-communicator.org> wrote:

On Tue, Aug 17, 2010 at 9:40 PM, Dmitri Melnikov <dmitri807@gmail.com> wrote:

I've noticed that in the trunk version of resources.properties the
property CALL_SAVED_TO is
plugin.callrecordingconfig.CALL_SAVED_TO={0}
Unfortunately while constructing the notification popup this causes
the Html2Text.extractText to run the html parser on the file path
string and I suppose it does not like the / character of the path, so
the empty string is returned and nothing is displayed. A simple
solution I found is to just wrap the path with <pre> tag.

Dmitri, it's true that I removed the quotation marks from the
CALL_SAVED_TO property value and the reason was the respective
notification displayed on my Ubuntu had the quotation marks displayed
though they didn't seem necessary at all. Anyway, the problem that you
are reporting now with Html2Text.extractText seems like a deficiency
of PopupMessageHandlerSwingImpl and, consequently, its solution seems
to be appropriate there in the very PopupMessageHandler implementation
instead of where you offer it at the specific use of the abstract
notification service. If you feel that the slash character is a
problem for PopupMessageHandler implementations in general and that
you want to impose a constraint on the PopupMessageHandler or the
NotificationManager in general that the slash characters should always
be escaped in a specific way (e.g. by using the <pre> tag), then you
should also describe the constraint in question in the respective
Javadocs.

---------------------------------------------------------------------
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


#30

Well, it's not hard to add the <pre> tags to the argument of
Html2Text.extractText in PopupMessageHandlerSwingImpl. But what's
puzzling to me is why Html2Text.extractText is used there at all.

Emil told me that it solved a problem in PopupMessageHandlerSwingImpl
which would fill the screen when given big HTML messages to display.

It's
function as I understand is to extract text from html code. But is it
necessary? Is there such a case where html code is always passed as a
message and cannot be extracted before?

There are multiple implementations of PopupMessageHandler and it's
their decision whether they are capable of displaying HTML. The
alternative of sanitizing all messages before passing them to the
PopupMessageHandler requires calls to Html2Text.extractText much more
often in comparison and deprives all PopupMessageHandler
implementations of dealing with HTML in accord with their own
capabilities, requirements.

···

On Thu, Aug 19, 2010 at 10:51 AM, Dmitri Melnikov <dmitri807@gmail.com> wrote:

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


#31

I see. Then can you please add this fix to the code:

Index: src/net/java/sip/communicator/impl/swingnotification/PopupMessageHandlerSwingImpl.java

···

===================================================================
--- src/net/java/sip/communicator/impl/swingnotification/PopupMessageHandlerSwingImpl.java (revision
7615)
+++ src/net/java/sip/communicator/impl/swingnotification/PopupMessageHandlerSwingImpl.java (working
copy)
@@ -108,6 +108,8 @@

     /**
      * Builds the popup component with given informations.
+ * Message content is wrapped with HTML "pre" tag to ensure
+ * that text such as full pathnames is displayed correctly.
      *
      * @param titleString message title
      * @param message message content
@@ -136,7 +138,7 @@
         msgTitle.setPreferredSize(new Dimension(200, msgTitleHeight));
         msgTitle.setFont(msgTitle.getFont().deriveFont(Font.BOLD));

- String plainMessage = Html2Text.extractText(message);
+ String plainMessage = Html2Text.extractText("<pre>" + message
+ "</pre>");
         JTextArea msgContent = new JTextArea(plainMessage);
         msgContent.setLineWrap(true);
         msgContent.setWrapStyleWord(true);

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


#32

Dmitri, thank for the patch! I committed it in r7626.

···

On Thu, Aug 19, 2010 at 12:02 PM, Dmitri Melnikov <dmitri807@gmail.com> wrote:

Index: src/net/java/sip/communicator/impl/swingnotification/PopupMessageHandlerSwingImpl.java
- String plainMessage = Html2Text.extractText(message);
+ String plainMessage = Html2Text.extractText("<pre>" + message
+ "</pre>");

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