Fix comparing QuantityType with inverted dimensions

[mherwege]

Comparing QuantityType with inverted units was opposite of what it should be.

Also included tests to verify.

See openhab/openhab-addons#18122 (comment)

@andrewfg @jimtng

[mherwege]

While this returns the correct sign when comparing values, this is still not ideal. The big issues is that you can have a < b and b < a at the same time. And there is no solution for that. That also means that whenever comparing QuantityTypes the order of the arguments becomes very important. a > b, therefore b < a is no longer true.

[jimtng]

I am uncomfortable with a > b holds true whilst b > a is also true.

Perhaps we just don't do any inversions here, which would render the above anomaly impossible, right? Make it the caller's responsibility to do inversion if they wanted to.

[mherwege]

I am uncomfortable with a > b holds true whilst b > a is also true.

Perhaps we just don't do any inversions here, which would render the above anomaly impossible, right? Make it the caller's responsibility to do inversion if they wanted to.

Yes exactly. I don't like it either, but at some point the comparison was made possible with inverted dimensions. It just doesn't work.
It makes it harder to do any of the state range profiles on color temperatures with a mix of Kelvin and mirek. And there are very strong advocates here to treat mirek and Kelvin as interchangeable. I guess there will also be impact on the state description min and max boundaries. And there are probably other cases I didn't think of yet.
So the right semantics of the comparison should be implemented everywhere the potential inversion is done.

By the way, could the whole discussion on the forum (https://community.openhab.org/t/colors-and-json-issue/161860/31) also be impacted by this?

[andrewfg]

I don't think we really need this PR as in the other PRs we will always convert quantities to the same common target unit value before doing a compare.

[mherwege]

I don't think we really need this PR as in the other PRs we will always convert quantities to the same common target unit value before doing a compare.

Except that the compare is the wrong way around. And the method is called all the time. It is very confusing to have a method that overrides a standard method with exactly the opposite behaviour. I am not adding anything here, I am just trying to fix a bug and providing a test to avoid it in the future.

We can remove the method of course and that would make sure this confusing behaviour never gets called.

[ccutrer]

equals should still work though, and as it just delegates to (internal)CompareTo, it needs fixed if we’re no longer allowing invertible units with compareTo

I also think internalCompareTo only exists so that it can easily call itself for the invertible case, and without that reason we may want to collapse its implementation back into the main compareTo

[mherwege]

equals should still work though, and as it just delegates to (internal)CompareTo, it needs fixed if we’re no longer allowing invertible units with compareTo

That's fixed now.

I also think internalCompareTo only exists so that it can easily call itself for the invertible case, and without that reason we may want to collapse its implementation back into the main compareTo

I thought so myself, but the signature is different (Quantity<T> vs Quantity<?>). And as it is still called from the equals method, I kept it.

[andrewfg]

signature is different (Quantity vs Quantity<?>)

@mherwege I must admit that I struggle with Java generics syntax. And I notice that the basicprofiles StateFilter classes produces an enormous amount of warnings about wrong casts (relating to QuantityType<?> vs. QuantityType<T> vs. QuantityType) where the original code author either just ignored the warnings or added Suppress annotations. Personally I don't like such tricks to eliminate such warnings, and would like to clean that up. So I wonder if you know the proper way to do it?

[mherwege]

I must admit that I struggle with Java generics syntax.

You are not alone. This sometimes feels like black magic to me as well. In my understanding, the main issue is that the compiler does not know the real class of T, it is only known at runtime. So trying to cast something to class T will always generate a compiler warning. Using ? just indicates it can be any type, so as long as it doesn't need to be cast, it is fine. I also am not always able to escape all conversion warnings.

I did find a way to do so in this case, collapsing the method, by making comparable work accross QuantityTypes. But I couldn't get rid of all warnings in the class and I gave up for now.

[ccutrer]

Hey, one of the Java topics I actually do understand fairly well!

• QuantityType<?> and QuantityType are the same. The latter is kind of deprecated, and you should never use it.
• @mherwege is correct, that at runtime the generic type is erased completely. That's why you can't do instanceof QuantityType<SomeType>, only instanceof QuantityType<?>. All generic checks are done at compile time. A QuantityType<SomeType> is implicitly convertible to QuantityType<?> (like passing an object to a method that takes QuantityType<?>), but not the opposite (returning QuantityType<?> cannot be assigned to a QuantityType<SomeType> without an explicit cast). Note that SomeType might be a concrete type, or it might be a type parameter, like T. Either way, it's a type, possibly with indirection, but not a wildcard.
• If you have public SomeType<T> someMethod(SomeType<T> t), the Ts must stay the same. You can't return a SomeType<OtherType> than what was passed in to the method, nor a SomeType<?> (which you got from some method returning that, such as QuantityType.inverse()). You could return a SomeType<U>, but only as long as U can be determined in some way by the compiler. This similarly applies in this PR that the T from the class definition must be the same all the way through the class - implements Comparable<QuantityType<T>> means compareTo must have the signature public int compareTo(QuantityType<T>). But when you call compareTo(other.inverse()), other.inverse() returns a QuantityType<?> which may not be compatible, and the compiler complains - it has no way to know that we're checking at runtime that the inverse unit is compatible, and thus that the result of other.inverse() will be a QuantityType<T>. The change in interface in this PR means you can now pass QuantityType<Mired> to a QuantityType<Kelvin>.compareTo method (and it will raise an IllegalArgumentException). Interestingly... this means prior to this PR, even though internalCompareTo could handle inverse units, the signature of compareTo wouldn't allow it (at least not without warnings?), and thus we're not actually breaking anything in this PR. compareTo never worked. equals did.

[mherwege]

@ccutrer Thank your for your explanation.
The more I think about this, the less sure I feel about eliminating the private internalCompareTo. To do that, I had to make QuantityType comparable independent on the unit. Before, the compiler would give a warning if you e.g. tried to compare Speed with Temperature somewhere in code. It won't anymore with my change (if I intepret things right). It will result in a runtime exception if you try to compare QuantityTypes with incompatible units instead. I actually doubt that is a good thing.
What is your view? Should I revert this?

[ccutrer]

Yes, that seems reasonable. For some reason I was thinking compareTo accepted Object, so it didn't really matter, but that's equals.

[andrewfg]

one of the Java topics I actually do understand fairly well!

@ccutrer wow. Brilliant! Thank you!