search

Return-To-The-Roots / s25client

Return To The Roots (Settlers II(R) Clone)
http://www.rttr.info
GNU General Public License v2.0
477 stars 75 forks source link

Update text in shown tooltip #1603

Closed falbrechtskirchinger closed 1 year ago

falbrechtskirchinger commented 1 year ago

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

falbrechtskirchinger commented 1 year ago

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.

https://github.com/Return-To-The-Roots/s25client/blob/bdf4110d761ddf73f235a8bdbd56fca8cf7cf941/libs/s25main/WindowManager.cpp#L839 only once (when updateCurrent (now) is set.)

Flamefire commented 1 year ago

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 commented 1 year ago

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. :wink:

falbrechtskirchinger commented 1 year ago

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