Bug in tooltip show delay

There appears to be a bug in how the delay for showing tooltips is calculated. Looking at 8.0.8, TooltipWindow::timerCallback() has

if (newTip.isNotEmpty()
    && newTip != tipShowing
    && now > lastCompChangeTime + (uint32) millisecondsBeforeTipAppears)
{
    showTip();
}

But

lastCompChangeTime + (uint32) millisecondsBeforeTipAppears

can easily rollover since Time::getApproximateMillisecondCounter() counts since boot.

If the full range of uint32 is desired, then probably want something like:

if (newTip.isNotEmpty() && newTip != tipShowing)
{
    uint32 elapsed = now >= lastCompChangeTime ? now - lastCompChangeTime : UINT_MAX - lastCompChangeTime + now;
    if (elapsed > (uint32)millisecondsBeforeTipAppears)
    {
        showTip();
    }
}

Did you encounter this bug? This would only occur if you have your system running for 49 days, 16 hours, and 48 minutes, and then happen to hover over an item that would trigger a tooltip only milliseconds before the rollover occurs. As soon as the rollover happens, it will be another ~50 days before it occurs again.

I would think the chances of this actually happening are extremely low. If it happened to you, I would look into lottery tickets :smiley:

Yes, I have indeed encountered this bug.

The fault in your logic would appear to be in assuming that setMillisecondsBeforeTipAppears(int newTimeMs) will only be called for small values of newTimeMs. If newTimeMs is sufficiently large and the system it is executing on has a sufficiently large up-time, then this bug will manifest on every such call.

There are three cases I can think of where newTimeMs might be very large (e.g. INT_MAX): one is while experimenting, another is while debugging, and yet another is as a convenient mechanism to effectively disable tooltips.

None of these use cases are in conflict with documentation, or indeed, any reasonable expectations of behavior. I feel confident in saying that no reasonable person would expect this method to behave differently based on how long ago the executing system was booted.

If, however, there is a legitimate reason why one would prefer an implementation that exhibits this erroneous rollover behavior, I would be interested to hear that.

No reasonable developer would set a tooltip delay larger than a couple of seconds. What would the purpose be of a tooltip that shows up, for example, a minute after you hover above something?

So, yes, if you set a tooltip delay of one hour, then after 48 hours of uptime the tooltip would fail to show up.

I’m not saying the current behavior is correct, but I’m saying that the bug can be ignored in favor of other fixes (development time is limited) because it’s unlikely to cause real world issues, and even if they happen, they are very temporary, and unserious.

See my previous post. I’ve given multiple examples. If you want your opinion to be taken seriously by serious people, then please read before you reply.

Incorrect. The failure will occur whenever the sum of the specified delay period and the up-time exceeds the rollover limit. Your example would be true if and only if uint32 was actually somewhere between 27 and 28 bits. But your example is, in any case, entirely irrelevant.

Your claims of potential likelihood, severity, and duration seem to be based on your lack of understanding. Again, see my previous post. I’m sorry that you don’t understand the issue, the remedy, and how developers might be more creative than you’d expect.

None of your three examples are serious use cases.

I won’t argue with you anymore as you are not discussing this in good faith.

I didn’t mean 48 hours, I meant 48 days,

2 Likes

A couple of examples of concrete cases that a developer would reasonably expect to function properly:

Example 1:

#if DEBUG_NO_TOOL_TIPS_EVER
constexpr int ToolTipDelayMillis = INT_MAX;
#else
constexpr int ToolTipDelayMillis = 700;
#endif

Example 2:

class ToolTipWindow final : public juce::TooltipWindow
{
public:
    ToolTipWindow(juce::Component* const parent)
      : juce::TooltipWindow(parent)
    {}

    void enable(const bool isEnabled = true)
    {
        this->setMillisecondsBeforeTipAppears(isEnabled ? 700 : INT_MAX);
    }
};

This ought to go without saying, but while it is clear to any experienced developer that it is impossible to state with certainty whether the goals of these examples could be accomplished using other means without knowing all of the relevant ramifications of every possible case (i.e. justifications for solutions are manifold and very often non-obvious), it is nevertheless not reasonable that a developer should discover erroneous behavior of such cases only after either nearly two months of up-time or deep source inspection.

In closing, the underlying bug is trivial to fix, the fix has no appreciably negative ramifications, and the expectation of correct functionality is entirely reasonable.

And for both cases you would get what you want. If the overflow happens before the tooltip shows up, the tooltip won’t show up. It’s effectively disabled.

I love how you repeat the “experienced” developer, implying that I’m not. Hilarious.

Yes, it’s trivial to fix. But it’s hard to test, and I’d rather the JUCE developers not waste their time on something that has no negative real world consequences.

Just because it’s technically incorrect doesn’t make it unusable. Practical considerations are often more important than technical pedantry.

2 Likes

Incorrect. What actually happens is the sum of the previous now and the delay rolls over, causing the greater-than comparison to the current now to return true, causing the tooltip to be erroneously displayed.

now > lastCompChangeTime + (uint32) millisecondsBeforeTipAppears

This is a very basic deadline rollover bug.

I’m honestly not sure why you continue to reply when you aren’t understanding the issue. But your opinions have been given the level of attention and consideration commensurate with the understanding you’ve displayed.

This reply was not addressed at you or anyone in particular, but you’re free to interpret what you read in whatever manner you prefer.

I don’t think there’s anything else substantive to say in this thread, and it’s going a little sideways, so I’m closing the topic.

2 Likes