Fixed accidental removal of tooltip when displaying text with message() #1651
Conversation
| label->SetName(name); | ||
| if (removeTooltip) | ||
| { | ||
| auto p = name.find("\\"); |
There was a problem hiding this comment.
Do we actually rely on the tooltip-removing ability of this method? Where? How? Why? If this only displays the "main" part of the text, what displays the tooltip?
If we don't need this, we can just remove the whole find&substr part and simply always call label->SetName(name, false);
There was a problem hiding this comment.
From my understanding, labels remove the tooltip part of the text by default. This might be to allow sharing of texts between labels and other controls that do display tooltips. So if you had a button with text "New player\\Create new player profile", a button would have "New player" text and "Create new player profile" tooltip. But if you set this to a label, it would only display "New player" and had no tooltip.
I don't know why it's done like this, I find this very clunky and confusing; tooltip should always be separate from the main text. But I wrote this code to preserve existing behavior, even if it's weird, in order to hopefully not introduce any new bugs.
There was a problem hiding this comment.
I found an example, where we do depend on tooltip removal
This appears to contain both the tooltip for the slider and the label:
colobot/colobot-base/src/common/restext.cpp
Line 243 in 03b2da8
colobot/colobot-base/src/ui/screen/screen_setup_game.cpp
Lines 104 to 110 in 03b2da8
There was a problem hiding this comment.
Adding boolean flags that make a subroutine do something different hurts readability.
- When reading
foo(..., false)one has to find the declaration of foo() before one can get any hints as to whatfalsedoes. - It is difficult to do a text search (grep) for all places where the subroutine is called with false.
It is often preferable to add a new subroutine instead of adding a boolean flag to an existing subroutine. I suggest adding a CreateLabelRaw() method instead of adding the removeTooltip flag. CreateLabelRaw() would not strip the tooltip, CreateLabel() would keep the existing behaviour.
There was a problem hiding this comment.
I agree. This is already confusing, no point in making this worse. I'll add a separate factory method for raw labels.
While at it, I think SetName() should be refactored into SetText(), SetTooltip() and SetTextTooltip(). It's not clear at all what SetName() does unless we analyze the code and GetName() only returns the text part which is even more confusing.
There was a problem hiding this comment.
Turns out that refactoring SetName() will take a lot more work than I anticipated. I'd leave it the way it is, it can be addressed in a separate issue. I did the other change though.
There was a problem hiding this comment.
I have tested the changes and found a bug.
When I run the following code at first everything works as expected:
extern void object::New()
{
message("foo\\bar");
wait(1);
message("foo\\bar");
wait(1);
message("foo\\bar");
}
But after one of the messages times-out the other two loose the part after the backslash:
I think this line is the culprit:
colobot/colobot-base/src/ui/displaytext.cpp
Line 393 in b5b6e45
|
Is this ready for review? |
|
I think current fix is enough for now, unless you found new bugs or something it's ready for review. Changing SetName() and its weirdness will require proper refactoring. I'll make a separate issue for that. |
Fixes #1649