JMC-6256: JMC doesn't respect Long.MIN_VALUE as a missing value

[Alex Macdonald]

Whoops, just realized I hit reply instead of reply-all so this didn't get
sent to list, please find attached an updated patch.

On Thu, Dec 13, 2018 at 10:00 AM Alex Macdonald <almacdon at redhat.com> wrote:

> Hi Marcus,
>
> Thanks for the review and suggestion, I've amended the patch to keep the
> string constants inside TypeHandling.
>
> On Wed, Dec 12, 2018 at 4:48 PM Marcus Hirt <marcus.hirt at oracle.com>
> wrote:
>
>> Hi Alex,
>>
>> Quick comment before going to bed:
>>
>> I don't believe we should externalize Integer.MIN_VALUE etc. They are
>> defined
>> in the java core libraries. One could argue for MISSING_VALUE and
>> MISSING_VALUE_TOOLTIP to be externalized though.
>>
>> Kind regards,
>> Marcus
>>
>>
>> On 2018-12-12, 22:10, "jmc-dev on behalf of Alex Macdonald" <
>> jmc-dev-bounces at openjdk.java.net on behalf of almacdon at redhat.com> wrote:
>>
>>     Hi!
>>
>>     This patch addresses JMC-6256 [0], in which MIN_VALUEs are displayed
>> in
>>     their numeric form instead of being displayed as a N/A or missing
>> value.
>>     For testing purposes, I have been using the memoryleak.jfr recording
>> that
>>     is available as an attachment on JMC-6127 [1].
>>
>>     As per the comments in the bug report, there are 6 cases to consider
>> for
>>     missing values in JMC [2]. Additionally, it may be nice to show the
>> actual
>>     value in parenthesis in the tooltip [3].
>>
>>     Screenshots (album [4]):
>>     Before: https://imgur.com/aoGgzRn [5]
>>     After: https://imgur.com/DVnHQGZ [6]
>>
>>     The culprit here seems to be the way the conditionals in the
>>     "getValueString()" [7][8] end up processing the value. There is a
>>     duplication of the "instanceof IDisplayable" in the JfrPropertySheet
>> [7]
>>     and the TypeHandling [8], and the TypeHandling.getValueString() ends
>> up
>>     being called anyways in the JfrPropertySheet.getValueString(). As a
>> result,
>>     the conditional to see if it's an IDisplayable from the
>> JfrPropertySheet
>>     happens before the conditional to see if it is a min value [9] (which
>> is
>>     where we want to end up in this case).
>>
>>     This patch removes the duplicated conditional from the
>> JfrPropertySheet,
>>     because it is already correctly handled in TypeHandling.
>> Additionally, a
>>     function "getNumericString" has been introduced for the display of the
>>     tooltip. If the value is an IQuantity then "getNumericString" will
>> figure
>>     out which missing value type it is, and display it in the tooltip. If
>> the
>>     value is an IQuantity but not a missing value, then it delegates to
>>     TypeHandling.getValueString(). I've also included a quick unit test to
>>     verify the behaviour of "getNumericString".
>>
>>     Thoughts?
>>
>>     Cheers,
>>
>>     Alex
>>
>>     [0] https://bugs.openjdk.java.net/browse/JMC-6256
>>     [1] https://bugs.openjdk.java.net/browse/JMC-6127
>>     [2]
>>
>> https://bugs.openjdk.java.net/browse/JMC-6256?focusedCommentId=14229297&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14229297
>>     [3]
>>
>> https://bugs.openjdk.java.net/browse/JMC-6256?focusedCommentId=14229296&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14229296
>>     [4] https://imgur.com/a/3BZNHnG
>>     [5] https://imgur.com/aoGgzRn
>>     [6] https://imgur.com/DVnHQGZ
>>     [7]
>>
>> http://hg.openjdk.java.net/jmc/jmc/file/a76a464b3764/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/JfrPropertySheet.java#l307
>>     [8]
>>
>> http://hg.openjdk.java.net/jmc/jmc/file/efeb1e97bec3/core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/util/TypeHandling.java#l171
>>     [9]
>>
>> http://hg.openjdk.java.net/jmc/jmc/file/efeb1e97bec3/core/org.openjdk.jmc.common/src/main/java/org/openjdk/jmc/common/util/TypeHandling.java#l167
>>
>>
>>
>>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 6256-1.patch
Type: text/x-patch
Size: 8827 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/jmc-dev/attachments/20181213/8433084f/6256-1-0001.patch>