[jitsi-dev] OperationSetAutoAnswerSipImpl -- Not Loading Properties


#1

First time on a mailing list, so please excuse any mistakes I've made, also
not a Java wiz so my fix may be dumb....

Properties to configure:

net.java.sip.communicator.impl.protocol.sip.acc1.AUTO_ANSWER_CONDITIONAL_NAME=Alert-Info
net.java.sip.communicator.impl.protocol.sip.acc1.AUTO_ANSWER_CONDITIONAL_VALUE=alert-autoanswer

The way that loading is done improperly on this class. It's relying on
overriding load() and having the super class call the overridden load(),
however the OperationSetAutoAnswerSipImpl instance hasn't been fully
initialized, and will then proceed to blow away the configuration just
loaded.

A small change such as this fixes the bug and allows for proper loading of
auto-answer configuration (and more importantly, provisioning support):

.../communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java |
2 ++
1 file changed, 2 insertions(+)

diff --git
a/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
b/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
index 195e4c3..674f81c 100644

···

---
a/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
+++
b/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
@@ -74,6 +74,8 @@ public class OperationSetAutoAnswerSipImpl
             ProtocolProviderServiceSipImpl protocolProvider)
     {
         super(protocolProvider);
+
+ this.load();
     }

     /**

Haven't confirmed that save works correctly to begin with.


#2

Hi,

there is a AbstractOperationSetBasicAutoAnswer which calls the load
method in its constructor, and as OperationSetAutoAnswerSipImpl
extends it and override this method, it is invoked when instance is
created. I do not see a problem with current implementation. Adding a
simple print in OperationSetAutoAnswerSipImpl can show you that it is
loaded.

Regards
damencho

···

On Fri, Aug 9, 2013 at 3:55 AM, William Roush <strangewilliam@gmail.com> wrote:

First time on a mailing list, so please excuse any mistakes I've made, also
not a Java wiz so my fix may be dumb....

Properties to configure:

net.java.sip.communicator.impl.protocol.sip.acc1.AUTO_ANSWER_CONDITIONAL_NAME=Alert-Info
net.java.sip.communicator.impl.protocol.sip.acc1.AUTO_ANSWER_CONDITIONAL_VALUE=alert-autoanswer

The way that loading is done improperly on this class. It's relying on
overriding load() and having the super class call the overridden load(),
however the OperationSetAutoAnswerSipImpl instance hasn't been fully
initialized, and will then proceed to blow away the configuration just
loaded.

A small change such as this fixes the bug and allows for proper loading of
auto-answer configuration (and more importantly, provisioning support):

.../communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java | 2
++
1 file changed, 2 insertions(+)

diff --git
a/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
b/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
index 195e4c3..674f81c 100644
---
a/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
+++
b/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
@@ -74,6 +74,8 @@ public class OperationSetAutoAnswerSipImpl
             ProtocolProviderServiceSipImpl protocolProvider)
     {
         super(protocolProvider);
+
+ this.load();
     }

     /**

Haven't confirmed that save works correctly to begin with.

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#3

This Stackoverflow thread details the problem
http://stackoverflow.com/questions/18138397/java-class-initialization-order-and-overridden-methods/18138541

I'll get video reproducing the issue, the debug output and the fix with
expected behavior tomorrow if needed.

···

On Aug 9, 2013 2:18 AM, "Damian Minkov" <damencho@jitsi.org> wrote:

Hi,

there is a AbstractOperationSetBasicAutoAnswer which calls the load
method in its constructor, and as OperationSetAutoAnswerSipImpl
extends it and override this method, it is invoked when instance is
created. I do not see a problem with current implementation. Adding a
simple print in OperationSetAutoAnswerSipImpl can show you that it is
loaded.

Regards
damencho

On Fri, Aug 9, 2013 at 3:55 AM, William Roush <strangewilliam@gmail.com> > wrote:
> First time on a mailing list, so please excuse any mistakes I've made,
also
> not a Java wiz so my fix may be dumb....
>
> Properties to configure:
>
>
net.java.sip.communicator.impl.protocol.sip.acc1.AUTO_ANSWER_CONDITIONAL_NAME=Alert-Info
>
net.java.sip.communicator.impl.protocol.sip.acc1.AUTO_ANSWER_CONDITIONAL_VALUE=alert-autoanswer
>
>
> The way that loading is done improperly on this class. It's relying on
> overriding load() and having the super class call the overridden load(),
> however the OperationSetAutoAnswerSipImpl instance hasn't been fully
> initialized, and will then proceed to blow away the configuration just
> loaded.
>
> A small change such as this fixes the bug and allows for proper loading
of
> auto-answer configuration (and more importantly, provisioning support):
>
> .../communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
> 2
> ++
> 1 file changed, 2 insertions(+)
>
> diff --git
>
a/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
>
b/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
> index 195e4c3..674f81c 100644
> ---
>
a/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
> +++
>
b/src/net/java/sip/communicator/impl/protocol/sip/OperationSetAutoAnswerSipImpl.java
> @@ -74,6 +74,8 @@ public class OperationSetAutoAnswerSipImpl
> ProtocolProviderServiceSipImpl protocolProvider)
> {
> super(protocolProvider);
> +
> + this.load();
> }
>
> /**
>
>
> Haven't confirmed that save works correctly to begin with.
>
> _______________________________________________
> dev mailing list
> dev@jitsi.org
> Unsubscribe instructions and other list options:
> http://lists.jitsi.org/mailman/listinfo/dev

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#4

there is a AbstractOperationSetBasicAutoAnswer which calls the load
method in its constructor, and as OperationSetAutoAnswerSipImpl extends
it and override this method, it is invoked when instance is created. I
do not see a problem with current implementation. Adding a simple print
in OperationSetAutoAnswerSipImpl can show you that it is loaded.

William is right here. While not forbidden, "virtual constructor calls" are
bad and lead to unpredictable behavior. The SO article nicely explains why.

Regards
damencho

Ingo


#5

Hello,

This Stackoverflow thread details the problem
http://stackoverflow.com/questions/18138397/java-class-initialization-order-and-overridden-methods/18138541

I think you are right. The OperationSetAutoAnswerSipImpl.load() method
gets called in the AbstractOperationSetBasicAutoAnswer constructor
before the OperationSetAutoAnswerSipImpl fields are initialized. It
loads the properties and stores them in OperationSetAutoAnswerSipImpl
fields. But then those fields are later zeroed when they are initialized.

Regards,
Boris

···

On 8/9/13 9:27 AM, William Roush wrote:


#6

Ok, sorry for overlooking it. Will fix it soon.
Thanks William for the report and link with explanation :slight_smile:

···

On Aug 9, 2013 9:40 AM, "Ingo Bauersachs" <ingo@jitsi.org> wrote:

> there is a AbstractOperationSetBasicAutoAnswer which calls the load
> method in its constructor, and as OperationSetAutoAnswerSipImpl extends
> it and override this method, it is invoked when instance is created. I
> do not see a problem with current implementation. Adding a simple print
> in OperationSetAutoAnswerSipImpl can show you that it is loaded.

William is right here. While not forbidden, "virtual constructor calls" are
bad and lead to unpredictable behavior. The SO article nicely explains why.

> Regards
> damencho

Ingo

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#7

It is fixed now, thanks once again.

···

On Fri, Aug 9, 2013 at 9:57 AM, Damian Minkov <damencho@jitsi.org> wrote:

Ok, sorry for overlooking it. Will fix it soon.
Thanks William for the report and link with explanation :slight_smile:

On Aug 9, 2013 9:40 AM, "Ingo Bauersachs" <ingo@jitsi.org> wrote:

> there is a AbstractOperationSetBasicAutoAnswer which calls the load
> method in its constructor, and as OperationSetAutoAnswerSipImpl extends
> it and override this method, it is invoked when instance is created. I
> do not see a problem with current implementation. Adding a simple print
> in OperationSetAutoAnswerSipImpl can show you that it is loaded.

William is right here. While not forbidden, "virtual constructor calls"
are
bad and lead to unpredictable behavior. The SO article nicely explains
why.

> Regards
> damencho

Ingo

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev


#8

Awesome, thanks for looking at it so quickly, the lost feature is minor for
now but will make this pretty much the most desirable softphone in my
opinion.

Thanks for the fix and thanks for a great SIP phone.

···

On Fri, Aug 9, 2013 at 4:20 AM, Damian Minkov <damencho@jitsi.org> wrote:

It is fixed now, thanks once again.

On Fri, Aug 9, 2013 at 9:57 AM, Damian Minkov <damencho@jitsi.org> wrote:
> Ok, sorry for overlooking it. Will fix it soon.
> Thanks William for the report and link with explanation :slight_smile:
>
> On Aug 9, 2013 9:40 AM, "Ingo Bauersachs" <ingo@jitsi.org> wrote:
>>
>> > there is a AbstractOperationSetBasicAutoAnswer which calls the load
>> > method in its constructor, and as OperationSetAutoAnswerSipImpl
extends
>> > it and override this method, it is invoked when instance is created. I
>> > do not see a problem with current implementation. Adding a simple
print
>> > in OperationSetAutoAnswerSipImpl can show you that it is loaded.
>>
>> William is right here. While not forbidden, "virtual constructor calls"
>> are
>> bad and lead to unpredictable behavior. The SO article nicely explains
>> why.
>>
>> > Regards
>> > damencho
>>
>> Ingo
>>
>>
>> _______________________________________________
>> dev mailing list
>> dev@jitsi.org
>> Unsubscribe instructions and other list options:
>> http://lists.jitsi.org/mailman/listinfo/dev

_______________________________________________
dev mailing list
dev@jitsi.org
Unsubscribe instructions and other list options:
http://lists.jitsi.org/mailman/listinfo/dev