compat1x creation of DateTimeType does not produce equivalent objects about openhab-core HOT 16 CLOSED

This:

    public static void test() {
        DateTimeType gen1 = new DateTimeType();
        DateTimeType gen2 = new DateTimeType(gen1.toString());
        DateTimeType gen3 = new DateTimeType(gen2.toString());

        System.out.println( "gen1 " + (gen1.equals(gen2) ? "equals" : "does not equal") + " gen2" );
        System.out.println( "gen2 " + (gen2.equals(gen3) ? "equals" : "does not equal") + " gen3" );
        System.out.println( "gen3 " + (gen3.equals(gen1) ? "equals" : "does not equal") + " gen1" );
    }

produces this:

gen1 does not equal gen2
gen2 equals gen3
gen3 does not equal gen1

from openhab-core.

openhab-core is a set of bundle that are owned by @kaikreuzer
It's not about the distribution or addons.
So, why not using its own repo if he maintains that bundles?

from openhab-core.

@watou: I kept it separate as this is closer to the ESH code base - it is simply some code around the ESH runtime that for the one or other reason is not within the ESH repo. As a preparation for opening up (=write access) the openhab Github organisation to a broader group of people, I wanted to keep this governance of this part separate and closer to ESH, so that it can be closely synched.

But back to your issue: Thanks for the analysis, I will fix it. Are there already any issues open regarding the memory leaks that you are mentioning?

from openhab-core.

So, why not using its own repo if he maintains that bundles?

I was just curious why these important org.openhab.* packages were copyright an individual and kept in a repo under a different github account, as it's different from the rest. (For example, this file's initial contributor submitted the code with a different copyright header — just pointing out a possible EPLv1.0 issue.) And thanks for the explanation, Kai. Do you foresee this repo moving to the non-profit's account/ownership at some point?

Thanks for the analysis, I will fix it. Are there already any issues open regarding the memory leaks that you are mentioning?

I discovered them in my own testing of the Nest binding on 2.0b2, but any 1.x add-on that compares DateTimeTypes will get false unexpectedly if it went through the compat1x translation. It had initially looked like the DateTimeType non-threadsafe bug was back, but on further inspection equality test is failing, I believe for the reason given.

from openhab-core.

@watou, this is not directly a bug in the compatibility layer. What is happening here is that toString() uses a pattern which drops the milliseconds - therefore gen2 is different to gen1, but gen2 and gen3 are identical (both without milliseconds). The very same tests should fail on openHAB 1 as well.

Now there are three solutions:

1. Don't use toString() for the ESH->OH type mapping, but rather transplant the calendar object itself
2. Make toString() pass the equals test - this is actually, what we expect in general; the downside is that we are losing the milliseconds on this type
3. Change toString() to also include the milliseconds.

Any opinions on what might be the best option?

from openhab-core.

Option 1, but clone the calendar on constructing the new one, since Calendar is mutable.

from openhab-core.

I would tend to either 2 or 3 - remember that it is a general assumption for all types that they are serialisable to a string and reconstructable from it (this discussion was also there when introducing the location item). Note that states are always sent as strings - on the local event bus, via the REST API, through MQTT etc. So you will come across the same problem at different places.

from openhab-core.

OK, option 2 then, if you mean changing the equals method in both ESH and compat1x versions to compare their toString() results instead of comparing calendars. It's more important I think to keep existing ISO8601 formats. The internal Calendar should retain its full time resolution in any case.

from openhab-core.

Or better, the above plus preserve the milliseconds in the toString() serialised format (using the ISO8601 hh:mm:ss.sss time format) when there is a fraction of a second in the calendar.

from openhab-core.

This is option 3 then, right? Now you have voted for all of them ;-)

from openhab-core.

Option 4 = options 2 and 3. ;-) Option 3 by itself is probably (not tested) insufficient by itself based on the description of Calendar.equals().

There is still an argument to clone the calendar, because the compat layer is only supposed to be ensuring 1.x bindings work as they do on the old runtime, where a state object taken from the event bus equals the one put on the event bus.

So my final vote is 1, but if you disagree with my point above, my second choice is 4.

Related: Do both ESH and OH2 require Java 1.8? If so, ESH's DateTimeType could improve its ISO8601 (de)serialization using java.time.* .

from openhab-core.

Ok, depending on how Calendar behaves, my option 3 was meant to be your option 4 ;-)

Related: Do both ESH and OH2 require Java 1.8?

No, ESH is still on Java 7 as a minimal requirement.

from openhab-core.

Do you foresee this repo moving to the non-profit's account/ownership at some point?

In case this question was overlooked. I would be concerned to not have an affirmative answer here. Thanks! ;-)

from openhab-core.

Do you foresee this repo moving to the non-profit's account/ownership at some point?

Yes, as explained in my https://github.com/kaikreuzer/openhab-core/issues/16#issuecomment-179959043, this could be potentially under the ownership of the Eclipse Foundation. But whereever it ends up, there is no reason for concerns as the code is under EPL and thus can be cloned or forked by anybody anytime.

Another question: toString() is not stating the time zone. So should 1pm CET thus be considered equal to 1pm PDT? Or would we want to include the timezone as well in the toString() value?

from openhab-core.

should 1pm CET thus be considered equal to 1pm PDT? Or would we want to include the timezone as well in the toString() value?

I think the full resolution and timezone of the Calendar should be in the serialised, ISO8601-compliant string format, and DateTimeType.equals() should return true if Calendar.equals() is true. Comparing whether two DateTimeTypes refer to the same moment is a different subject, perhaps new DateTimeType methods that delegate to Calendar.

About this repo and your copyright header changes on previously committed files, it might be helpful if you published a status update on the forum covering the governance and IP issues for all org.openhab and org.eclipse.smarthome repos, so everyone has a perfectly clear picture of what exactly they are using and contributing to. I for one would appreciate such an update. Thanks for all you do.

from openhab-core.

I for one would appreciate such an update

I'll certainly do this once the NPO gets closer.

DateTimeType.equals() should return true if Calendar.equals() is true

This isn't that easy - Calendar.equals e.g. also checks for firstDayOfWeek equivalence and I am not sure we want this... Anyhow, #17 should fix the issue for your cases without addressing the toString() issues for the moment. I have created eclipse-archived/smarthome#992 to follow up the discussion on how we want to proceed with the toString().

from openhab-core.