Skip to content

Conversation

@falbrechtskirchinger
Copy link
Contributor

When a tooltip is currently being shown and its text is changed, update the displayed text.

Copy link
Member

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Triggered by the "mutable" I feel this is the wrong approach: WindowManager already knows the currently shown tooltip (used for hiding it)
So how about adding a "UpdateTooltip" method to WindowManager that works similarly to WindowManager::SetToolTip in checking if the currently shown tooltip is "THIS"? Or even better: Add an extra param to WindowManager::SetToolTip: bool updateCurrent = false to combine that into a single function. Maybe even use updateCurrent = updateCurrent || tooltip.empty() and do the check at

if(curTooltip && (!ttw || curTooltip->getShowingCtrl() == ttw))
only once (when updateCurrent (now) is set.)

@falbrechtskirchinger
Copy link
Contributor Author

Triggered by the "mutable" I feel this is the wrong approach: WindowManager already knows the currently shown tooltip (used for hiding it)

Great. I was unaware of this.

So how about adding a "UpdateTooltip" method to WindowManager that works similarly to WindowManager::SetToolTip in checking if the currently shown tooltip is "THIS"? Or even better: Add an extra param to WindowManager::SetToolTip: bool updateCurrent = false to combine that into a single function. Maybe even use updateCurrent = updateCurrent || tooltip.empty() and do the check at

I think tooltip.empty() should override updateCurrent. Unless I'm missing something, the conditions don't mix cleanly the way you're describing. You'll see in a second.

if(curTooltip && (!ttw || curTooltip->getShowingCtrl() == ttw))

only once (when updateCurrent (now) is set.)

Add a parameter (bool updateCurrent) to the WindowManager::SetToolTip()
function, to update the text of the currently displayed tooltip.
When a tooltip's text is changed, ask the WindowManager to update the
currently shown tooltip. The call is only effective if the tooltip is
actually being shown.
@Flamefire
Copy link
Member

I think tooltip.empty() should override updateCurrent. Unless I'm missing something, the conditions don't mix cleanly the way you're describing. You'll see in a second.

Can you elaborate? updateCurrent = updateCurrent || tooltip.empty() accomplishes the override, doesn't it?
I'd have kept the ToolTip instance (in WindowManager) immutable and recreated it on change.

Anyway your solution works too 👍

@falbrechtskirchinger
Copy link
Contributor Author

I think tooltip.empty() should override updateCurrent. Unless I'm missing something, the conditions don't mix cleanly the way you're describing. You'll see in a second.

Can you elaborate? updateCurrent = updateCurrent || tooltip.empty() accomplishes the override, doesn't it? I'd have kept the ToolTip instance (in WindowManager) immutable and recreated it on change.

Oh, right, because you would have just taken the branch re-creating the tooltip.
I'm not a fan of short-lived, frequent heap allocations, not because of the cost (which is often negligible), but because of heap fragmentation and resulting excessive memory consumption. glibc's malloc() really sucks in that regard and applications can end up reserving obscene amounts of completely unused memory because it's full of unused holes.

Of course, the lines vector is still bugging me for the same reason. 😉

@falbrechtskirchinger
Copy link
Contributor Author

FYI, I have just confirmed that this does in fact work by testing these commits against #1604.

@Flamefire Flamefire requested a review from Flow86 July 17, 2023 14:38
@Flow86 Flow86 merged commit 9dd260a into Return-To-The-Roots:master Jul 17, 2023
@falbrechtskirchinger falbrechtskirchinger deleted the update-tooltip branch July 18, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants