Skip to content
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

Qt: honor control characters in ScriptingTextBuffer::print #3350

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ahigerd
Copy link
Contributor

@ahigerd ahigerd commented Nov 19, 2024

My previous PR fixed line wrapping for strings with embedded newlines, but in the process it completely broke newline handling.

This PR makes sure that the cursor moves to the next block / a new block after each line.

@ahigerd
Copy link
Contributor Author

ahigerd commented Nov 19, 2024

After some further discussion in Discord, I've decided I don't want to submit this PR as-is -- it fixes the bug, but the strategy for how to fix it is inelegant and leaves bugs pertaining to \r lingering.

@ahigerd ahigerd changed the title Qt: fix newline handling in TextBuffer print() Qt: honor control characters in ScriptingTextBuffer::print Nov 19, 2024
@ahigerd
Copy link
Contributor Author

ahigerd commented Nov 20, 2024

Rewritten -- and added support for \t while I was at it.

Copy link
Member

@endrift endrift left a comment

Choose a reason for hiding this comment

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

Nothing major looks wrong, but I have a few concerns.

src/platform/qt/scripting/ScriptingTextBuffer.cpp Outdated Show resolved Hide resolved
int lineWidth = m_dims.width();

for (const QChar& ch : text) {
if (ch == '\t') {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to do this with a switch statement? (Can we do this with a switch statement?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It COULD be done with a switch statement but I'm not sure it would make the code look any better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... After refactoring to fix the performance issue, maybe it would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think?

src/platform/qt/scripting/ScriptingTextBuffer.cpp Outdated Show resolved Hide resolved
@ahigerd ahigerd force-pushed the alh/fix-textbuffer-newlines branch 2 times, most recently from 058774e to 10ec5b3 Compare November 24, 2024 00:50
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.

2 participants