Skip to content

MACGUI: Null pointer and empty chunks tests in mactext , change cursor restoration in getmaxwidth. #6538

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dasbidyendu
Copy link

@dasbidyendu dasbidyendu commented Apr 5, 2025

  • Check for null pointer before handling any operations in getChunkNum function.

  • Check for empty chunks to avoid extra operations.

  • avoid signed/unsigned issue in chunk position.

  • avoid possible overflow in cursor restoration logic

@AndywinXp
Copy link
Contributor

The changes have nothing to do with the SCUMM engine.

@bluegr bluegr changed the title SCUMM: Null pointer and empty chunks tests in mactext , change cursor restoration in getmaxwidth. MACGUI: Null pointer and empty chunks tests in mactext , change cursor restoration in getmaxwidth. Apr 5, 2025
@dasbidyendu
Copy link
Author

You’re absolutely correct, I apologise for the blunder .

@sev- sev- added the GSoC Part of a Google Summer of Code project label Apr 5, 2025
@sev-
Copy link
Member

sev- commented Apr 5, 2025

Just as it was written in the previous PR, you must follow our Commit Guidelines. Please fix the commit log messages.

Also, you still must fix the excessive vertical space and in general follow our Commit Guidelines, particularly for indentation.

@dasbidyendu
Copy link
Author

I have made the fixes can you check now

@dasbidyendu
Copy link
Author

Can you check it now , i think i have removed most of the whitespaces

Copy link
Member

@sev- sev- left a comment

Choose a reason for hiding this comment

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

Added notes and questions

@@ -107,8 +120,7 @@ void MacTextCanvas::chopChunk(const Common::U32String &str, int *curLinePtr, int
return;

// Now add rest of the chunks
MacFontRun newchunk = _text[curLine].chunks[curChunk];

MacFontRun newchunk = *chunk;
Copy link
Member

Choose a reason for hiding this comment

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

Please explain this change

Copy link
Author

Choose a reason for hiding this comment

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

Earlier in the code the chunk was already defined as MacFontRun *chunk = &_text[curLine].chunks[curChunk]
So using *chunk instead of re-accessing _text[curLine].chunks[curChunk] avoids repeating the same lookup.
The new line makes it clear that we’re copying the same chunk we just created and worked with above. This makes the logic easier to follow. Since the function is already using the chunk pointer in multiple places, it's better to keep things consistent rather than mixing direct access and pointer usage.

ppos += _canvas.getLineCharWidth(i);
ppos += _cursorCol;

int absoluteCharOffset = 0;
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this renaming?

Copy link
Author

Choose a reason for hiding this comment

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

The new name better describes the actual role of the variable: it stores the absolute character offset in the text buffer, from the beginning up to the cursor's position (_cursorRow, _cursorCol).Since the value is used to restore cursor position after rewrapping (with setMaxWidth()), calling it absoluteCharOffset makes its purpose immediately clear to future readers and maintainers.

@dasbidyendu dasbidyendu requested a review from sev- April 9, 2025 06:03
@dasbidyendu
Copy link
Author

Hey I have made and explained all the changes , can you please review it

@sev-
Copy link
Member

sev- commented Apr 15, 2025

@dasbidyendu You still have to fix your commit log messages

@dasbidyendu
Copy link
Author

Hey , i am sorry for asking this but can i please get an example of how it should be , i have read the documentation but i can't figure out what to do , Sorry for asking.

@raziel-
Copy link
Contributor

raziel- commented Apr 15, 2025

@dasbidyendu

Hey , i am sorry for asking this but can i please get an example of how it should be , i have read the documentation but i can't figure out what to do , Sorry for asking.

Update mactext-canvas.cpp
is too generic and missing the prefix.
The rest of the commit messages should be fine, i think, unless i'm on the completely wrong track

@dasbidyendu
Copy link
Author

Ok I’ll fix that one

…t-canvas.cpp

Added some checks to make sure The line continuations are not empty before adding them to the current line's word continuation to ensure consistency.

Also added some comments on previous code.

Made sure to append the chunks if it is within bounds.

Added the newChunk as a pointer to the current chunk.
…r restoration issue in getmaxwidth.

- Check for null pointer before handling any operations in getChunkNum function.
- Check for empty chunks to avoid extra operations.
- avoid signed/unsigned issue in chunk position.
- avoid possible overflow in cursor restoration logic
Remove unnecessary vertical whitespaces from code.
@dasbidyendu
Copy link
Author

I think i have fixed all the commit logs , please check them

@dasbidyendu
Copy link
Author

Can anyone check this and tell if I need to make further changes @sev- @bluegr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GSoC Part of a Google Summer of Code project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants