Skip to content

Fix rendering of highlighted URLs #16342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 0 commits into from
Closed

Conversation

asdf-a11
Copy link

@asdf-a11 asdf-a11 commented Jul 16, 2025

Add compact, short information about your PR for easier understanding:

To do

This PR Ready for Review

How to test

To test use infomation provided in #16246 issue.

@Zughy Zughy changed the title FixedHighlightingUrl #16246 Fix rendering of highlighted URLs Jul 16, 2025
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

I don't think that throwing away the color info can produce a clean solution.

The cleanest solution is probably to utilize EnrichedString properly.

@@ -176,21 +177,36 @@ void GUIEditBoxWithScrollBar::draw()
start_pos = ml ? m_broken_text_positions[i] : 0;
}

//Create copy of string removing highlighting specifier
Copy link
Contributor

Choose a reason for hiding this comment

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

You're (incorrectly) reimplementing unescape_enriched (found in src/util/string.h) here.


// draw normal text
font->draw(txt_line->c_str(), m_current_text_rect,
font->draw(plainString.c_str(), m_current_text_rect,
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't just throw away the color here. (And if we wanted to throw away the color, which we don't, we would do that way earlier, during construction or when the text is set.)

Screenshot From 2025-07-16 14-22-14

@appgurueu appgurueu added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jul 16, 2025
@asdf-a11
Copy link
Author

Yes I would agree. (I am still getting familiar with the code base) So is there not already a string class that handles colour information and you are proposing to replace core::stringw with a new rich text class for all text rendering?

@appgurueu
Copy link
Contributor

So is there not already a string class that handles colour information

Yes, there is EnrichedString. (Initially I had misremembered what exactly that did, but it does pretty much what we need: Separate a string with color (and translation) escapes into plain text and color info.)

and you are proposing to replace core::stringw with a new rich text class for all text rendering?

Yes, replacing core::stringw with EnrichedString for the text. This could become a bit nasty due to the inheritance (requiring you to touch more classes than you'd like to).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Bugfix 🐛 PRs that fix a bug @ Client / Audiovisuals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highlighting the URL text in the "Open URL" window causes buggy text rendering
3 participants