SCUMM: Last Crusade/Loom Macintosh GUI, phase 2 #5411
Conversation
This will be used for Last Crusade and Loom. Loom is currently the more broken of the two, though.
Mac Loom and Last Crusade use bold for their "About" menu item, and that made the menu too narrow.
My goal right now is to move all font drawing that isn't explicitly handled by scripts into the Mac GUI. The Indy 3 speech boxes will probably be moved into the Mac GUI as well, since they're a bit special. This means the charset class will only have to keep track of the game font, which will remove some odd special cases.
Correctly exclude non-sticky keyboard modifiers.
It was trying to figure out the size of the largest glyph, but there is now only one font to consider.
This is already done in keyEvent(), which is the only place where processMenuShortCut() is called. This allows using Alt-<key> as shortcut, which is consistent with how the Mac emulators I've tried does it.
I don't think I'll be able to use the pre-existing Mac dialog class because it seems too tailored to the Wage engine. So at least for now I'm making my own.
At least until I know exactly how tiny a subset of PICT v1 I need, it's easier to keep the code here. It needs to be cleaned up, but it works well enough for the one picture I've tried it on.
That should be enough to then draw the animations and text on top of.
This will make it possible to skip to the next part, like the original does, while cutting down on code duplication.
The first text box is now drawn almost almost correctly, and the whole text box is handled by a single function. Something's off about the outline font, but that's a separate issue.
Added the final button, which is actually just another text box.
Add, don't set, the extra bitmap offset for italics to keep glyphs from overlapping in memory.
The cursor regression was something I introduced when moving Mac cursor handling into MacGui. Hopefully it works more like it used to again now.
Something isn't right about the way it draws the borders. It works for the normal cases, but not when it draws over other things. I need to think about that, but there are other thigns I need to fix first.
This doesn't match the original perfectly. At this point, I don't think I care enough to try and fix it, as long as it looks decent.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- engines/scumm/gfx_mac.h (6 hunks)
Additional comments: 26
engines/scumm/gfx_mac.h (26)
22-23: The header guards are correctly defined to prevent multiple inclusions of the header file.
39-50: The
MacGuiclass is well structured with member variables initialized in the constructor initializer list, which is good for performance.51-53: The use of a hash map for fonts is appropriate for quick access, but ensure that memory management for the fonts is handled correctly to avoid leaks.
68-653: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [54-77]
The
Colorenum is well-defined and includes custom color support. Ensure that the color values match the expected game palette.
79-96: The
FontIdenum is well-defined and seems to cover various font styles used in the game. Ensure that these IDs are correctly mapped to the actual font resources.99-103: The
TextStyleenum is clear and concise. It's good practice to define such enums for readability and maintainability of text styling.106-112: The
TextLinestruct is a good abstraction for handling text lines with style and alignment. Ensure that thestrpointer is managed properly to avoid dangling pointers.114-116: The
MacDialogWindowStyleenum provides a clear distinction between different window styles.119-131: The
delayfunction and virtual functions for dialog handling are well declared. Ensure that the implementations of these virtual functions are consistent across derived classes.137-202: The
MacGuiObjectandMacWidgetclasses are well-structured with clear inheritance. The use of virtual destructors is good practice for base classes.204-216: The
MacButtonclass is well-defined. Ensure that thedrawCornersfunction is implemented correctly to handle the drawing of button corners.218-229: The
MacCheckboxclass has a clear implementation with mouse event handling. Ensure that the_isCheckedstate is managed correctly throughout the GUI lifecycle.235-261: The
MacStaticTextclass is a simple and effective way to handle static text in the GUI. ThesetColorandsetTextmethods are good for dynamic updates.264-306: The
MacEditTextclass handles editable text fields. Ensure that text selection and editing functionality are thoroughly tested, especially the mouse and keyboard event handling.308-319: The
MacPictureclass is a simple wrapper for displaying images. Ensure that the_picturepointer is managed properly to avoid memory leaks.321-343: The
MacSliderBaseclass provides a good foundation for slider widgets. Ensure that the value and position calculations are accurate and that the slider behaves correctly with mouse events.345-384: The
MacSliderclass extendsMacSliderBasewith specific drawing and event handling. Ensure that paging and arrow button functionality are working as expected.386-412: The
MacPictureSliderclass is a specialized slider with a background and handle picture. Ensure that the drawing and event handling are correctly implemented, especially the handle drawing and mouse interaction.414-452: The
MacListBoxclass handles list box widgets. Ensure that the text update and selection functionality are working correctly and that mouse wheel support is implemented as expected.454-554: The
MacDialogWindowclass is a complex class handling dialog windows. Ensure that the event handling, widget management, and drawing functions are thoroughly tested. Pay special attention to the management of the_beamCursorand the various surfaces to avoid memory leaks.556-594: The
MacGuiclass constructor and destructor are well-defined. Ensure that all resources are properly released in the destructor to avoid memory leaks. The virtual functions provide a good interface for derived classes.596-626: The
MacLoomGuiclass is a derived class ofMacGuispecific to the "Loom" game. Ensure that the overridden methods are correctly implemented and that the_practiceBoxis managed properly.628-650: The
MacIndy3Guiclass is another derived class ofMacGuispecific to the "Indiana Jones and the Last Crusade" game. Ensure that the text area and verb GUI functionality are correctly implemented.659-678: The
isVerbGuiActivemethod and related functionality are important for the game's GUI state management. Ensure that the verb GUI state is correctly synchronized with the game's state.689-708: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [689-758]
The
WidgetandVerbWidgetclasses are well-structured with clear responsibilities. The use of a timer and redraw flag is good for managing state and drawing optimization. Ensure that the_killflag inVerbWidgetis used correctly and that widgets are properly destroyed when no longer needed.
- 871-876: The
hidemethod and related functionality for managing the GUI visibility and dirty screen regions are important for the GUI's visual updates. Ensure that these methods are correctly implemented to maintain the GUI's visual integrity.
The worst one was that the scroll wheel could crash ScummVM. The rest is mostly cosmetical.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- engines/scumm/gfx_mac.h (6 hunks)
Additional comments: 25
engines/scumm/gfx_mac.h (25)
22-32: The inclusion guards and necessary includes are correctly set up. The forward declarations for
OSystem,Surface, andMacWindowManagerare also in place, ensuring that the header file doesn't include unnecessary dependencies.39-50: The
MacGuiclass is well-structured with protected member variables that are initialized to safe default values. This is good practice to avoid uninitialized variables that could lead to undefined behavior.68-657: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [54-77]
The
Colorenum is well-defined and includes a comment about custom colors, which is helpful for maintainability. However, ensure that the custom color indices do not conflict with other parts of the codebase that might use the same range of values.
79-96: The
FontIdenum is clearly defined, which will make it easier to manage fonts across the GUI code. It's good to see that different font styles for different parts of the game's GUI are considered.99-112: The
TextStyleenum andTextLinestruct are well-defined. TheTextLinestruct includes all necessary fields for rendering text, which should simplify the text drawing code elsewhere.114-117: The
MacDialogWindowStyleenum is simple and clear, but ensure that the styles it defines are sufficient for all dialog window types that the GUI might need to display.119-131: The
delayfunction and virtual functions for handling dialogs are declared but not defined here. Ensure that their implementations handle errors gracefully, especially when dealing with file I/O or user input.140-154: The
MacGuiObjectclass is a good example of a base class with common properties for GUI objects. It uses a constructor initializer list, which is efficient.156-202: The
MacWidgetclass is well-structured and provides a comprehensive set of virtual functions for event handling and drawing. The separation ofsetRedrawandsetEnabledfunctions is good for controlling widget state.204-216: The
MacButtonclass inherits fromMacWidgetand seems to be well-defined. However, thedrawCornersfunction takes a pointer toCornerLinewithout checking if it'snullptr. Ensure that the pointer is always valid when passed to this function.218-229: The
MacCheckboxclass has a private_hitBoundsmember that should be used for more accurate click detection. Ensure that_hitBoundsis correctly calculated and updated if the checkbox moves or changes size.235-261: The
MacStaticTextclass is a simple widget for displaying static text. It overridesgetFocusandloseFocuswith empty implementations, which is appropriate since static text does not need to handle focus events.264-306: The
MacEditTextclass is more complex due to the need to handle text input and cursor management. Ensure that the text editing logic is robust against invalid input and edge cases, such as extremely long text or rapid input.308-319: The
MacPictureclass is straightforward, but ensure that the_picturepointer is managed properly to avoid memory leaks or dangling pointers.321-343: The
MacSliderBaseclass provides a base for slider widgets. It includes functions for calculating the slider value from its position and vice versa, which is a common requirement for sliders. Ensure that these calculations are accurate and handle edge cases, such as when the slider is at its minimum or maximum position.345-387: The
MacSliderclass extendsMacSliderBaseand includes additional logic for handling user interaction with the slider. It's important to ensure that the mouse event handling is smooth and responsive.389-415: The
MacPictureSliderclass is a specialized slider that uses pictures for its background and handle. Ensure that the pictures are loaded and displayed correctly, and that the slider functionality is not compromised by the use of images.417-456: The
MacListBoxclass handles a list of selectable items. It's important to ensure that the list box can handle a variable number of items and that the scrolling and selection logic is intuitive and error-free.458-558: The
MacDialogWindowclass is a complex class responsible for managing dialog windows. It includes a lot of functionality, such as cursor management, widget handling, and drawing. Ensure that the class correctly manages resources, such as surfaces and cursors, to avoid memory leaks or graphical glitches.560-598: The
MacGuiclass constructor and destructor are declared, along with several virtual functions. Ensure that all derived classes properly implement these virtual functions and that the destructor cleans up any allocated resources.600-630: The
MacLoomGuiclass is a derived class ofMacGuispecific to the game "Loom". It includes overrides for several virtual functions and manages game-specific GUI elements like the practice box. Ensure that the overrides are correctly implemented and that game-specific resources are managed properly.632-654: The
MacIndy3Guiclass is another derived class ofMacGuifor the game "Indiana Jones and the Last Crusade". It includes a text area for the game's GUI and overrides several virtual functions. As withMacLoomGui, ensure that the overrides are correctly implemented and that game-specific resources are managed properly.663-686: The
isVerbGuiActivefunction and other methods related to verb GUI management are important for the game's interaction. Ensure that the verb GUI is correctly shown or hidden based on game state and that verbs are updated as needed.693-712: The
Widgetclass within theMacIndy3Guinamespace is a base class for GUI widgets specific to "Indiana Jones and the Last Crusade". It includes a timer for handling timed events. Ensure that the timer logic is accurate and that widgets correctly respond to timeout events.875-880: The
show,hide,fill, andmarkScreenAsDirtyfunctions are important for managing the visibility and redrawing of the GUI. Ensure that these functions correctly update the screen and that any dirty regions are properly redrawn to maintain a smooth user experience.Overall, the code is well-structured and seems to follow good object-oriented design principles. However, due to the complexity and the number of features being added or modified, thorough testing is essential to ensure that all new functionality works as expected and that existing functionality is not broken.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- engines/scumm/script_v4.cpp (1 hunks)
- engines/scumm/scumm_v4.h (1 hunks)
Files skipped from review due to trivial changes (1)
- engines/scumm/scumm_v4.h
engines/scumm/script_v4.cpp
Outdated
| void ScummEngine_v4::clearSeriesIQPoints() { | ||
| // Stub | ||
| } |
There was a problem hiding this comment.
The clearSeriesIQPoints function is defined as a stub with no implementation. If this function is intended to be used, it should be properly implemented to clear the series IQ points as the name suggests. If it's not going to be used, it should be removed to avoid confusion and maintain clean code.
engines/scumm/script_v4.cpp
Outdated
| saveIQPoints(); | ||
| } | ||
|
|
||
| void ScummEngine_v4::clearSeriesIQPoints() { | ||
| // Stub | ||
| } | ||
|
|
||
| void ScummEngine_v4::saveIQPoints() { | ||
| // save Indy3 IQ-points | ||
| Common::OutSaveFile *file; |
There was a problem hiding this comment.
The saveIQPoints function is responsible for saving the IQ points to a file. The filename is constructed using _targetName + ".iq", which is a good approach to ensure unique filenames for different targets. However, there is no error handling if the file cannot be opened for saving. It would be beneficial to add error handling to inform the user or take corrective action if the file cannot be opened.
Here's a proposed change to add basic error handling:
Common::OutSaveFile *file;
Common::String filename = _targetName + ".iq";
+ bool saveSuccessful = false;
file = _saveFileMan->openForSaving(filename);
if (file != nullptr) {
byte *ptr = getResourceAddress(rtString, STRINGID_IQ_EPISODE);
if (ptr) {
int size = getResourceSize(rtString, STRINGID_IQ_EPISODE);
file->write(ptr, size);
+ saveSuccessful = true;
}
delete file;
+ } else {
+ warning("Could not open file '%s' for saving IQ points", filename.c_str());
+ }
+ // Additional logic could be added here to handle the failure caseCommitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| saveIQPoints(); | |
| } | |
| void ScummEngine_v4::clearSeriesIQPoints() { | |
| // Stub | |
| } | |
| void ScummEngine_v4::saveIQPoints() { | |
| // save Indy3 IQ-points | |
| Common::OutSaveFile *file; | |
| saveIQPoints(); | |
| } | |
| void ScummEngine_v4::clearSeriesIQPoints() { | |
| // Stub | |
| } | |
| void ScummEngine_v4::saveIQPoints() { | |
| // save Indy3 IQ-points | |
| Common::OutSaveFile *file; | |
| Common::String filename = _targetName + ".iq"; | |
| bool saveSuccessful = false; | |
| file = _saveFileMan->openForSaving(filename); | |
| if (file != nullptr) { | |
| byte *ptr = getResourceAddress(rtString, STRINGID_IQ_EPISODE); | |
| if (ptr) { | |
| int size = getResourceSize(rtString, STRINGID_IQ_EPISODE); | |
| file->write(ptr, size); | |
| saveSuccessful = true; | |
| } | |
| delete file; | |
| } else { | |
| warning("Could not open file '%s' for saving IQ points", filename.c_str()); | |
| } | |
| // Additional logic could be added here to handle the failure case | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- engines/scumm/gfx_mac.h (6 hunks)
Additional comments: 23
engines/scumm/gfx_mac.h (23)
22-23: The use of include guards is correct and follows the standard convention for C++ header files.
39-50: The
MacGuiclass is well-structured with clear member variable declarations. The use of default member initializers (nullptrandfalse) is a good practice in modern C++ to ensure that members are well-defined.51-53: The use of a hash map for fonts is appropriate for quick access, but ensure that memory management for the fonts is handled correctly elsewhere to avoid memory leaks.
68-658: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [54-77]
The
Colorenum is well-defined and includes comments for custom colors, which is good for maintainability. However, ensure that the values used forkCustomColor,kBackground, andkTransparencydo not conflict with actual color values that might be used in the game.
79-96: The
FontIdenum is clearly defined and seems to cover various font styles needed for the game. Ensure that these IDs are consistently used throughout the codebase.99-112: The
TextStyleenum andTextLinestruct are well-defined. TheTextLinestruct provides a clear way to represent a line of text with its style and alignment, which is good for text rendering tasks.114-117: The
MacDialogWindowStyleenum is simple and clear, but ensure that the styles it represents are sufficient for all dialog window types needed in the game.119-131: The presence of virtual functions indicates that this class is designed to be subclassed. Ensure that all subclasses properly implement these functions. The
prepareSaveLoadfunction is a utility that should be carefully reviewed to ensure it handles savegame names and slots correctly.137-202: The nested
MacGuiObjectandMacWidgetclasses are well-structured and provide a clear hierarchy for GUI objects. The use of virtual destructors is good practice for classes intended to be subclassed. ThedrawTextanddrawBitmapprotected methods inMacWidgetare utility functions that should be reviewed for correct rendering behavior.204-216: The
MacButtonclass inherits fromMacWidgetand seems to be specialized for button widgets. ThedrawCornersmethod suggests custom drawing logic for buttons, which should be reviewed for correct visual appearance.218-229: The
MacCheckboxclass includes a_hitBoundsmember for hit detection, which is important for user interaction. Ensure that the hit detection logic is accurate and that thehandleMouseUpmethod correctly toggles the checkbox state.235-261: The
MacStaticTextclass is a simple widget for displaying static text. ThesetColormethod allows for changing the text and background colors, which should be tested to ensure that the colors are updated correctly on the screen.264-306: The
MacEditTextclass is more complex due to text editing capabilities. Pay special attention to the text selection and caret handling logic, as these are common sources of bugs in text editing components.308-319: The
MacPictureclass manages a picture widget, which involves handling aGraphics::Surface. Ensure that the surface is managed correctly to avoid memory leaks or dangling pointers.321-343: The
MacSliderBaseclass and its derivedMacSliderclass handle slider widgets. The value-to-position conversion methods (calculateValueFromPosandcalculatePosFromValue) are critical and should be carefully reviewed to ensure that the slider behaves correctly across its range.389-415: The
MacPictureSliderclass combines a slider with pictures for the background and handle. Ensure that the drawing and event handling logic correctly updates the slider's visual state and value.417-456: The
MacListBoxclass manages a list box widget, which includes handling for scrolling and selection. The interaction between the list box and its associated slider (_slider) is particularly important to review for correct scrolling behavior.458-558: The
MacDialogWindowclass is a complex component that manages a dialog window, including its widgets, drawing, and event handling. The methodsrunDialog,updateCursor, andupdateare key functions that should be thoroughly tested to ensure the dialog behaves correctly.560-598: The
MacGuiclass constructor and destructor should be reviewed to ensure that resources are initialized and released correctly. The virtual methods likehandleEvent,initialize, andupdateindicate that this class is designed for inheritance, so check that subclasses properly implement these methods.600-630: The
MacLoomGuisubclass provides specific implementations for the Loom game. Ensure that the overridden methods likehandleEventandgetFontByScummIdare correctly tailored to the game's requirements.68-658: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [632-687]
The
MacIndy3Guisubclass provides specific implementations for the Indiana Jones game. The overridden methods and additional private members like_visibleand_leftButtonIsPressedshould be reviewed for correct functionality within the context of this game.
- 664-687: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [664-702]
The
isVerbGuiActivemethod and related methods in theMacIndy3Guisubclass should be reviewed to ensure that the verb GUI is managed correctly, especially during game states where the GUI should not be active.
- 876-881: The
hide,fill, andmarkScreenAsDirtymethods in theMacIndy3Guisubclass should be reviewed to ensure that they correctly manage the visibility and drawing of the GUI elements.
This avoids glitches when scrolling (once scrolling is fixed).
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- engines/scumm/script_v4.cpp (1 hunks)
- engines/scumm/scumm_v4.h (2 hunks)
Additional comments: 1
engines/scumm/scumm_v4.h (1)
- 37-42: The addition of
clearSeriesIQPoints()andupdateIQPoints()functions to theScummEngine_v4class is intended to support the new Mac GUI functionality. These methods likely relate to game-specific features for "Indiana Jones and the Last Crusade" and "Loom". It's important to ensure that these methods are implemented with proper error handling and are called in the correct contexts within the engine. Additionally, the comment "Used by MacGui" provides clarity on why these methods are introduced, which is good for maintainability.
| void loadVars(); | ||
| void saveIQPoints(); | ||
| void loadIQPoints(byte *ptr, int size); |
There was a problem hiding this comment.
The removal of updateIQPoints() function from the class indicates a refactoring or a design change. It's crucial to verify that this change does not leave any orphaned calls to the now-removed updateIQPoints() method elsewhere in the codebase. If the functionality of this method has been replaced or is no longer needed, this change is justified. However, if the method is still required and has been moved to another class or part of the code, it's important to ensure that all references to it have been updated accordingly.
engines/scumm/script_v4.cpp
Outdated
| void ScummEngine_v4::clearSeriesIQPoints() { | ||
| Common::OutSaveFile *file; | ||
| Common::String filename = _targetName + ".iq"; | ||
|
|
||
| file = _saveFileMan->openForSaving(filename); | ||
| if (file != nullptr) { | ||
| int size = getResourceSize(rtString, STRINGID_IQ_EPISODE); | ||
|
|
||
| for (int i = 0; i < size; i++) | ||
| file->writeByte(0); | ||
|
|
||
| _scummVars[245] = 0; | ||
|
|
||
| delete file; | ||
| } | ||
| } |
There was a problem hiding this comment.
This new function clearSeriesIQPoints is introduced to clear IQ points. It opens a file with a .iq extension and writes zero bytes to it. There are a few points to consider:
- The function assumes that the file will be successfully opened and does not handle the case where
openForSavingmight fail (other than checking fornullptr). It would be safer to handle the potential error case where the file cannot be opened. - The loop that writes zero bytes to the file does not check the return value of
writeByte. While this might not be critical for most file systems, it's generally good practice to check for write errors. - The function directly manipulates
_scummVars[245]without any checks. This is potentially risky if the index 245 is not guaranteed to be associated with the series IQ points in all contexts where this function might be called. - There is no check to ensure that the size obtained from
getResourceSizeis the expected size for the IQ points data structure. This could lead to writing an incorrect number of bytes to the file. - The function does not return any value or status to indicate success or failure, which might be useful for callers to know whether the operation succeeded.
Here's a proposed change that adds error handling and status reporting:
void ScummEngine_v4::clearSeriesIQPoints() {
Common::OutSaveFile *file;
Common::String filename = _targetName + ".iq";
+ bool success = false;
file = _saveFileMan->openForSaving(filename);
if (file != nullptr) {
int size = getResourceSize(rtString, STRINGID_IQ_EPISODE);
+ if (size != EXPECTED_IQ_DATA_SIZE) {
+ warning("Unexpected IQ data size: %d", size);
+ } else {
for (int i = 0; i < size; i++) {
- file->writeByte(0);
+ if (file->writeByte(0) != 1) {
+ warning("Failed to write zero byte to IQ file");
+ break;
+ }
}
+ success = true;
_scummVars[245] = 0;
delete file;
}
+ } else {
+ warning("Failed to open IQ file for clearing: %s", filename.c_str());
}
+ return success;
}This change adds a success flag that is set to true only if the file is opened successfully, the size is as expected, and all bytes are written without error. It also adds warnings to the log in case of unexpected conditions.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| void ScummEngine_v4::clearSeriesIQPoints() { | |
| Common::OutSaveFile *file; | |
| Common::String filename = _targetName + ".iq"; | |
| file = _saveFileMan->openForSaving(filename); | |
| if (file != nullptr) { | |
| int size = getResourceSize(rtString, STRINGID_IQ_EPISODE); | |
| for (int i = 0; i < size; i++) | |
| file->writeByte(0); | |
| _scummVars[245] = 0; | |
| delete file; | |
| } | |
| } | |
| void ScummEngine_v4::clearSeriesIQPoints() { | |
| Common::OutSaveFile *file; | |
| Common::String filename = _targetName + ".iq"; | |
| bool success = false; | |
| file = _saveFileMan->openForSaving(filename); | |
| if (file != nullptr) { | |
| int size = getResourceSize(rtString, STRINGID_IQ_EPISODE); | |
| if (size != EXPECTED_IQ_DATA_SIZE) { | |
| warning("Unexpected IQ data size: %d", size); | |
| } else { | |
| for (int i = 0; i < size; i++) { | |
| if (file->writeByte(0) != 1) { | |
| warning("Failed to write zero byte to IQ file"); | |
| break; | |
| } | |
| } | |
| success = true; | |
| _scummVars[245] = 0; | |
| delete file; | |
| } | |
| } else { | |
| warning("Failed to open IQ file for clearing: %s", filename.c_str()); | |
| } | |
| return success; | |
| } |
| saveIQPoints(); | ||
| } | ||
|
|
||
| void ScummEngine_v4::clearSeriesIQPoints() { | ||
| Common::OutSaveFile *file; | ||
| Common::String filename = _targetName + ".iq"; | ||
|
|
||
| file = _saveFileMan->openForSaving(filename); | ||
| if (file != nullptr) { | ||
| int size = getResourceSize(rtString, STRINGID_IQ_EPISODE); | ||
|
|
||
| for (int i = 0; i < size; i++) | ||
| file->writeByte(0); | ||
|
|
||
| updateIQPoints(); | ||
|
|
||
| delete file; | ||
| } |
There was a problem hiding this comment.
The clearSeriesIQPoints function is designed to reset the IQ points by writing zero bytes to a save file. However, there are a few issues and improvements that can be made:
-
Error Handling: There is no error handling if the file cannot be opened for saving. If
openForSavingfails, it will returnnullptr, and the current code does not handle this case. It's important to handle this gracefully. -
Resource Management: The use of
deleteto close the file is correct, but it should be in afinally-like block to ensure it is executed even if an exception occurs during the write operations. -
Performance: Writing byte by byte in a loop is not efficient. It would be better to write the entire buffer at once if possible.
-
Magic Number: The
.iqfile extension is a magic string that appears multiple times in the code. It would be better to define it as a constant.
Here's a proposed fix:
- The buffer is allocated and zero-initialized before the try block.
- The entire buffer is written at once instead of byte by byte.
- A catch block is added to handle any exceptions that might occur during the write operation.
- The buffer is freed and the file is closed in a way that ensures these operations are always executed, even if an exception occurs.
- A warning is logged if the file cannot be opened.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| saveIQPoints(); | |
| } | |
| void ScummEngine_v4::clearSeriesIQPoints() { | |
| Common::OutSaveFile *file; | |
| Common::String filename = _targetName + ".iq"; | |
| file = _saveFileMan->openForSaving(filename); | |
| if (file != nullptr) { | |
| int size = getResourceSize(rtString, STRINGID_IQ_EPISODE); | |
| for (int i = 0; i < size; i++) | |
| file->writeByte(0); | |
| updateIQPoints(); | |
| delete file; | |
| } | |
| saveIQPoints(); | |
| } | |
| void ScummEngine_v4::clearSeriesIQPoints() { | |
| Common::OutSaveFile *file; | |
| Common::String filename = _targetName + ".iq"; | |
| file = _saveFileMan->openForSaving(filename); | |
| if (file == nullptr) { | |
| warning("Could not open file %s for saving IQ points", filename.c_str()); | |
| return; | |
| } | |
| try { | |
| int size = getResourceSize(rtString, STRINGID_IQ_EPISODE); | |
| byte *buffer = new byte[size]; | |
| memset(buffer, 0, size); | |
| file->write(buffer, size); | |
| updateIQPoints(); | |
| delete[] buffer; | |
| } catch (...) { | |
| delete file; | |
| throw; | |
| } | |
| delete file; | |
| } |
This means we won't have to set their color, and that clicking on them automatically does nothing. (I hope I didn't accidentally disable anything else in the process.)
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- engines/scumm/gfx_mac.h (6 hunks)
Additional comments: 25
engines/scumm/gfx_mac.h (25)
22-23: The include guards are correctly defined to prevent multiple inclusions of the header file.
39-50: The
MacGuiclass is well-structured with clear member variable declarations. It's good to see that pointers are initialized tonullptrto avoid undefined behavior.51-52: The use of a
HashMapfor fonts is appropriate for quick access, but ensure that memory management for the fonts is handled correctly elsewhere to avoid memory leaks.68-657: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [54-76]
The
Colorenum is well-defined and includes comments for custom colors, which is a good practice for maintainability.
79-96: The
FontIdenum is clearly defined, which should help with readability and maintainability when dealing with different font types.99-112: The
TextStyleandTextLinestructures are well-defined and should facilitate text rendering.114-117: The
MacDialogWindowStyleenum provides a clear distinction between different window styles.121-131: Virtual functions for dialog handling are declared, which is good for extensibility. However, ensure that all derived classes implement these methods as they are pure virtual.
140-154: The
MacGuiObjectclass provides a good base for GUI objects with essential properties like visibility and bounds.156-202: The
MacWidgetclass is well-structured with virtual functions for event handling and drawing. The separation of concerns is evident here.204-216: The
MacButtonclass inherits fromMacWidgetand provides additional functionality specific to buttons, such as drawing corners.218-229: The
MacCheckboxclass has a private member_isCheckedto track the state, which is good practice. ThefindWidgetanddrawmethods are overridden to handle checkbox-specific logic.235-261: The
MacStaticTextclass is a simple widget for displaying static text. It overrides only the necessary functions without adding unnecessary complexity.264-306: The
MacEditTextclass is more complex due to the need to handle text input, selection, and caret management. Ensure that the text editing logic is thoroughly tested, especially edge cases like selection and cursor movement.308-319: The
MacPictureclass is straightforward, providing functionality to handle picture widgets.321-343: The
MacSliderBaseclass provides a base for slider widgets, encapsulating common slider functionality.345-387: The
MacSliderclass extendsMacSliderBaseand adds functionality for arrow buttons and handle drawing. Ensure that the event handling logic is robust and well-tested.389-415: The
MacPictureSliderclass is a specialized slider that uses pictures for the background and handle. It's good to see specialized widgets for different UI needs.417-455: The
MacListBoxclass handles a list box widget with associated text and slider for scrolling. The complexity is higher due to the interaction between list items and the slider, so thorough testing is recommended.457-557: The
MacDialogWindowclass is a complex class that manages dialog windows, including widget management and event handling. It's crucial to ensure that the event loop and widget interactions are well-tested to prevent UI bugs.559-596: The
MacGuiclass constructor and destructor are declared, along with essential functions for GUI management. The virtual functions provide a good extension point for game-specific GUI implementations.599-629: The
MacLoomGuiclass is a game-specific GUI implementation for "Loom". It overrides necessary methods and provides additional functionality like a practice box for musical notes.68-657: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [631-686]
The
MacIndy3Guiclass is another game-specific GUI implementation for "Indiana Jones and the Last Crusade". It includes additional logic for verb GUI management and inventory handling.
693-699: The
Widgetclass within theMacIndy3Guiclass is a nested class that provides base functionality for widgets in this context. The timer logic is included, which is important for widgets that need to update or animate over time.875-880: The
hidemethod and other utility functions likefillandmarkScreenAsDirtyare well-defined, providing essential functionality for GUI management.
Word-wrapping is used for message dialogs, but not e.g. for strings in the save/load dialogs.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- engines/scumm/gfx_mac.h (6 hunks)
Additional comments: 10
engines/scumm/gfx_mac.h (10)
22-23: The use of include guards is correct and follows the standard convention for preventing multiple inclusions of the same header file.
68-661: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [39-602]
The
MacGuiclass is well-structured with clear separation of concerns, using protected members for internal state and public methods for interaction. The use of virtual functions for handling menus and dialogs is good for extensibility and allows derived classes to provide specific implementations. The use of enums for colors and fonts provides a clear and maintainable way to reference these resources throughout the code.
140-202: The
MacGuiObjectandMacWidgetclasses provide a good foundation for GUI elements with common properties and methods. The use of inheritance and virtual functions for drawing and event handling is appropriate. However, ensure that all widgets properly manage resources, especially if they allocate memory or acquire other resources.204-216: The
MacButtonclass correctly inherits fromMacWidgetand provides a specialized implementation for buttons. Thedrawmethod anddrawCornershelper function seem to be well-defined for the button's visual representation.267-309: The
MacEditTextclass is complex and handles text input, selection, and caret management. Ensure that text input and editing operations are correctly implemented and tested, especially edge cases like selection deletion and caret movement.460-561: The
MacDialogWindowclass is a comprehensive implementation of a dialog window with support for various widgets and custom drawing methods. The use of aGraphics::Surfacefor rendering and an array ofMacWidgetpointers for managing widgets is appropriate. Ensure that memory management is handled correctly, especially in the destructor, to prevent memory leaks.563-601: The
MacGuiclass constructor and destructor seem to be correctly defined. The virtual methods for handling events and GUI updates are well-placed for derived classes to override. ThecreateWindow,createDialog, anddrawBannermethods provide useful utilities for GUI management.603-633: The
MacLoomGuiclass is a specialized GUI for the game "Loom". It correctly inherits fromMacGuiand overrides necessary methods. The presence of a_practiceBoxand related methods suggest additional game-specific functionality, which should be thoroughly tested.68-661: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [635-688]
The
MacIndy3Guiclass is another specialized GUI for the game "Indiana Jones and the Last Crusade". It extendsMacGuiand adds specific elements like a text area and save game edit text. The overridden methods and additional private members indicate extended functionality that should be carefully reviewed for correctness.
- 879-884: The
showandhidemethods along with thefillandmarkScreenAsDirtyfunctions in theMacIndy3Guiclass are essential for managing the visibility and redrawing of the GUI. ThecopyDirtyRectsToScreenmethod is presumably used to optimize drawing by only updating parts of the screen that have changed. Ensure that the dirty rectangle system is robust and does not lead to drawing artifacts.
… empty editText string This prevents users from ever saving games with empty names, like in the original and also removes my previous ugly hack for fetching the savegame name string.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- engines/scumm/gfx_mac.h (6 hunks)
Additional comments: 25
engines/scumm/gfx_mac.h (25)
22-32: The inclusion guards and necessary includes are correctly set up. The forward declarations for
OSystemand theGraphicsnamespace classes are also in place, ensuring that the header file has the necessary references without including the entire definitions, which is good for compilation times.39-50: The
MacGuiclass is well-structured with protected member variables that are initialized to sensible defaults. This is good practice as it ensures that the class has a valid state upon instantiation.51-53: The use of a
HashMapfor fonts is appropriate for quick access to fonts by their IDs. However, ensure that there is proper management of the font resources to avoid memory leaks, especially since pointers toGraphics::Fontare being stored.68-662: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [54-77]
The
Colorenum is well-defined with clear color names and reserved values for custom colors. This is a good practice as it makes the code more readable and maintainable.
79-96: The
FontIdenum is well-defined and follows a similar pattern to theColorenum, which is good for consistency.99-112: The
TextStyleenum andTextLinestruct are clear and well-defined, which will be helpful for managing text styles and lines in the GUI.119-131: The presence of virtual functions for dialog handling (
runAboutDialog,runOpenDialog,runSaveDialog,runOptionsDialog) indicates that this class is meant to be subclassed, with specific implementations provided in the subclasses. This is a good use of polymorphism.140-154: The
MacGuiObjectclass is a good base class for GUI elements, encapsulating common properties like redraw state, enabled state, visibility, and bounds. This promotes code reuse and consistency across GUI components.156-205: The
MacWidgetclass is a well-structured subclass ofMacGuiObject, adding more specific properties and methods for widgets. The presence of pure virtual methods (draw) indicates that this class is also meant to be subclassed.207-219: The
MacButtonclass is a concrete implementation of a widget, providing adrawmethod and additional functionality specific to buttons. This is a good example of object-oriented design.221-232: The
MacCheckboxclass correctly overrides thefindWidgetmethod to provide specific hit-testing logic for checkboxes, which is a good practice for interactive GUI elements.238-267: The
MacStaticTextclass provides functionality for static text display in the GUI, including word wrapping and color setting. The separation of this functionality into its own class is good for modularity.270-311: The
MacEditTextclass is a complex widget that handles text editing, including cursor management and text selection. The complexity is justified given the functionality it provides, but it's important to ensure that all edge cases are handled, such as text overflow and cursor positioning.313-324: The
MacPictureclass encapsulates the functionality for displaying a picture within the GUI. It's good to see that it properly manages the lifecycle of theGraphics::Surfaceit uses.326-348: The
MacSliderBaseclass provides a base for slider widgets, encapsulating common slider functionality. This is a good use of inheritance to avoid code duplication between different types of sliders.350-392: The
MacSliderclass extendsMacSliderBaseand adds functionality for a standard slider with arrows and a draggable handle. The code is well-organized and seems to cover the necessary functionality for a slider widget.394-420: The
MacPictureSliderclass is another specialized slider that uses pictures for the background and handle. This class demonstrates good use of inheritance and encapsulation of specialized behavior.422-460: The
MacListBoxclass is a complex widget that manages a list of selectable items. It correctly encapsulates the logic for item selection, scrolling, and drawing. Ensure that the list management logic is robust, especially when handling a large number of items or dynamic content changes.462-563: The
MacDialogWindowclass is a comprehensive implementation of a dialog window, with methods for managing visibility, widgets, and drawing. The use of an array for widgets and the ability to set a default widget are good practices for dialog management.565-603: The
MacGuiclass constructor and destructor are correctly defined. The virtual methods indicate that this class is designed to be subclassed for specific game GUIs. The use of pure virtual methods enforces that subclasses must provide implementations for these methods.605-635: The
MacLoomGuiclass is a subclass ofMacGuithat provides specific implementations for the Loom game. It correctly overrides the necessary methods and provides additional functionality specific to Loom.68-662: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [637-690]
The
MacIndy3Guiclass is another subclass ofMacGuifor the Indiana Jones game. It adds specific functionality and overrides methods as needed. The presence of game-specific logic within this class is appropriate.
668-675: The comment explaining the distinction between the GUI being allowed and being active is insightful and helps clarify the intended behavior of the GUI in different contexts.
698-705: The
Widgetclass within theMacIndy3Guiclass is a specialized widget for handling verbs in the game. The use of a_timervariable suggests that widgets may have time-based behavior, which is a common requirement for interactive elements.880-885: The methods for filling a rectangle and marking the screen as dirty are essential for GUI rendering. The
hidemethod is also important for managing the visibility of the GUI.Overall, the code is well-structured and demonstrates good object-oriented design principles. The use of inheritance and encapsulation is consistent, and the code is modular, which should make it easier to maintain and extend. The GUI classes are designed to be subclassed, which is appropriate for a system that needs to support different game-specific GUIs. The comments are helpful and provide context where necessary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- engines/scumm/gfx_mac.h (6 hunks)
- engines/scumm/input.cpp (4 hunks)
Additional comments: 26
engines/scumm/gfx_mac.h (26)
22-23: The header guards are correctly defined to prevent multiple inclusions of the header file.
39-50: The
MacGuiclass is well-structured with clear member variable declarations. It's good to see that member variables are initialized with default values.51-53: The use of a
HashMapfor fonts is appropriate for quick access, but ensure that memory management for the fonts is handled correctly to avoid memory leaks.68-663: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [54-76]
The
Colorenum is well-defined and includes comments for custom colors and special cases like background and transparency, which is good for maintainability.
79-96: The
FontIdenum is clearly defined with distinct identifiers for different font styles and game-specific fonts. This is good for readability and maintainability.99-103: The
TextStyleenum is concise and covers the necessary styles. It's good to see that the styles are clearly named.106-112: The
TextLinestruct is a simple and effective way to encapsulate text drawing parameters. It's good to see that alignment is considered.114-116: The
MacDialogWindowStyleenum provides a clear distinction between different window styles.121-131: Virtual functions for handling GUI-specific behavior are declared, which is a good use of polymorphism to allow for game-specific GUI implementations.
135-152: The
MacGuiObjectclass serves as a base class for GUI elements, which is a good design choice for code reuse and maintainability.154-202: The
MacWidgetclass is well-designed with a clear separation of concerns and virtual functions for handling events and drawing. Ensure that derived classes properly override these functions as needed.219-230: The
MacCheckboxclass is well-implemented with clear handling of mouse events and drawing logic.236-266: The
MacStaticTextclass is a good example of a simple widget that handles text display. The use of word wrapping and color settings is a nice touch.268-309: The
MacEditTextclass is a complex widget with text editing capabilities. It handles mouse and keyboard events, text selection, and caret management. Ensure that the text surface is properly managed to avoid memory leaks.311-322: The
MacPictureclass is a simple widget for displaying images. Ensure that the_picturepointer is managed correctly to prevent memory leaks.324-346: The
MacSliderBaseclass provides a base for slider widgets, encapsulating common slider functionality. It's good to see that value and position calculations are separated into their own functions.348-390: The
MacSliderclass extendsMacSliderBaseand adds functionality for drawing and handling events specific to a slider with arrows. The implementation is detailed and considers user interaction extensively.392-418: The
MacPictureSliderclass is a specialized slider that uses pictures for the background and handle. It's a good example of extending base functionality for a more complex widget.420-458: The
MacListBoxclass is a complex widget that manages a list of selectable items. It includes a slider for scrolling and handles mouse and wheel events. Ensure that the_textWidgetsand_sliderare managed correctly to prevent memory leaks.460-561: The
MacDialogWindowclass is a comprehensive implementation of a dialog window with support for various widgets, cursor management, and drawing primitives. It's well-structured and provides a solid foundation for dialog handling in the GUI.563-604: The
MacGuiclass constructor, destructor, and virtual functions are declared, providing a framework for game-specific GUI implementations. The use of pure virtual functions enforces the implementation of essential methods in derived classes.606-636: The
MacLoomGuiclass is a game-specific GUI implementation for "Loom". It extendsMacGuiand overrides necessary methods. The presence of a_practiceBoxsuggests additional game-specific functionality.68-663: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [638-690]
The
MacIndy3Guiclass is another game-specific GUI implementation for "Indiana Jones and the Last Crusade". It includes additional functionality such as verb handling and inventory management.
669-690: The
isVerbGuiActivemethod and related methods provide functionality for verb GUI management. Ensure that the state of_visibleand_leftButtonIsPressedis managed correctly across different game states.699-718: The
Widgetclass within theMacIndy3Guiclass is a base class for GUI elements specific to "Indiana Jones and the Last Crusade". It includes timer management and drawing primitives. Ensure that the static members_vm,_gui, and_surfaceare initialized and used correctly.881-886: The
hidemethod and related drawing functions are well-implemented, providing essential functionality for GUI visibility and screen updates.
| class MacButton : public MacWidget { | ||
| private: | ||
| struct CornerLine { | ||
| int start; | ||
| int length; | ||
| }; | ||
|
|
||
| public: | ||
| MacButton(MacGui::MacDialogWindow *window, Common::Rect bounds, Common::String text, bool enabled) : MacWidget(window, bounds, text, enabled) {} | ||
|
|
||
| void draw(bool drawFocused = false); | ||
| void drawCorners(Common::Rect r, CornerLine *corner); | ||
| }; |
There was a problem hiding this comment.
The MacButton class extends MacWidget and provides functionality specific to buttons. The private CornerLine struct is not used and could be removed if it's not needed.
- struct CornerLine {
- int start;
- int length;
- };Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| class MacButton : public MacWidget { | |
| private: | |
| struct CornerLine { | |
| int start; | |
| int length; | |
| }; | |
| public: | |
| MacButton(MacGui::MacDialogWindow *window, Common::Rect bounds, Common::String text, bool enabled) : MacWidget(window, bounds, text, enabled) {} | |
| void draw(bool drawFocused = false); | |
| void drawCorners(Common::Rect r, CornerLine *corner); | |
| }; | |
| class MacButton : public MacWidget { | |
| public: | |
| MacButton(MacGui::MacDialogWindow *window, Common::Rect bounds, Common::String text, bool enabled) : MacWidget(window, bounds, text, enabled) {} | |
| void draw(bool drawFocused = false); | |
| void drawCorners(Common::Rect r, CornerLine *corner); | |
| }; |
|
|
||
| class VerbWidget : public Widget { | ||
| protected: | ||
| bool _visible = false; | ||
| int _verbid = 0; | ||
| int _verbslot = -1; | ||
| bool _kill = false; | ||
|
|
||
| public: | ||
|
|
||
| VerbWidget(int x, int y, int width, int height) : Widget(x, y, width, height) {} | ||
|
|
||
| void setVerbid(int n) { _verbid = n; } | ||
| bool hasVerb() const { return _verbslot != -1; } | ||
| bool isVisible() const { return _visible; } | ||
| void threaten() { _kill = true; } | ||
| bool isDying() const { return _kill; } | ||
|
|
There was a problem hiding this comment.
The VerbWidget class is a specialized widget for handling verbs in the game. The use of a _kill flag to manage the widget's lifecycle is a bit unclear and could potentially be replaced with a more explicit state management system.
- bool _kill = false;
+ enum class State { Active, Dying, Dead };
+ State _state = State::Active;Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| class VerbWidget : public Widget { | |
| protected: | |
| bool _visible = false; | |
| int _verbid = 0; | |
| int _verbslot = -1; | |
| bool _kill = false; | |
| public: | |
| VerbWidget(int x, int y, int width, int height) : Widget(x, y, width, height) {} | |
| void setVerbid(int n) { _verbid = n; } | |
| bool hasVerb() const { return _verbslot != -1; } | |
| bool isVisible() const { return _visible; } | |
| void threaten() { _kill = true; } | |
| bool isDying() const { return _kill; } | |
| class VerbWidget : public Widget { | |
| protected: | |
| int _verbid = 0; | |
| int _verbslot = -1; | |
| enum class State { Active, Dying, Dead }; | |
| State _state = State::Active; | |
| public: | |
| VerbWidget(int x, int y, int width, int height) : Widget(x, y, width, height) {} | |
| void setVerbid(int n) { _verbid = n; } | |
| bool hasVerb() const { return _verbslot != -1; } | |
| void threaten() { _state = State::Dying; } | |
| bool isDying() const { return _state == State::Dying; } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- engines/scumm/input.cpp (5 hunks)
Additional comments: 4
engines/scumm/input.cpp (4)
107-114: The handling of Macintosh events is correctly placed before other event processing to ensure that Mac GUI events have priority. This is important for the correct functioning of the Mac-specific GUI elements and should be maintained unless there is a specific reason to change the event processing order.
279-285: The clearing of the key status upon receiving a
Common::EVENT_RETURN_TO_LAUNCHERorCommon::EVENT_QUITis a good practice to prevent any unintended behavior that might occur if the key status were to persist after such events.294-314: The logic for handling quit events is well-structured. It checks for the presence of a Mac GUI and whether the user has disabled the exit confirmation. If the exit confirmation is enabled, it runs the quit dialog. If the user confirms the exit, it either pushes a
Common::EVENT_RETURN_TO_LAUNCHERevent or callsquitGame()depending on theexitType. This is a good example of handling platform-specific behavior while respecting user preferences.993-1005: The restart dialog logic is correctly checking for the presence of a Mac GUI and, if present, displays a restart dialog. If the user confirms the restart, the game is restarted using the
restart()function. This is a good implementation of a conditional feature that provides a consistent user experience across different platforms.
| } | ||
| } | ||
|
|
||
| // Word-wrap text |
There was a problem hiding this comment.
The algo suspiciously similar to Font::wordWrapText(), but perhaps you have your own for better control
|
Thank you! |
Actually, I didn't even realize there was one there. I saw there was one in base-str.cpp that I couldn't use, and stopped looking there. It's only needed to wrap the text in a few dialog boxes (Restart and such) so if I can get the one for free in Font that'd be great. |
(This text is a bit of a mess. There was a lot of things I wanted to get in there, but I'll see if I can clean it up a bit later.)
This is phase 2 of implementing the Macintosh GUI for Loom and Indiana Jones and the Last Crusade. Now also for Loom, though it's much less noticeable. I have not spent as much time polishing it as the first part, but I still think it's time to make the pull request.
I've played through both Loom and Indiana Jones and the Last Crusade, and made a few bugfixes along the way. Apart from the missing features, it should be in better shape than what's currently in the development version.
There is now a common MacGui class, with derived classes for the Last Crusade and Loom GUIs. One of my goals has been to move any "special purpose" text drawing (e.g. the Last Crusade speech boxes) out of the charset renderer class, leaving it to handle only regular in-game text.
Another goal is to provide a foundation for the next part of the road, which is to get the Mac dialogs fully integrated with the game. @AndywinXp is working on that.
New features
There is a menu bar, thanks to the ScummVM Mac window manager class. (This is the only aspect that is controlled by the "original GUI" setting. I figured some may want to turn it off, but the Last Crusade verb gui is non-negotiable. There are too many hacks that would have to be reintroduced in order to get the old Franken-GUI back.) The menu items are generated from the games's menu resources. However:
The draggable practice mode box in Loom has been implemented. I always said it was too silly a feature to add to ScummVM, but with all the work being done on handling the Mac GUI it seemed even sillier not to do it. If enhancements are enabled, you can drag it freely. If not, X coordinates have to be multiples of 16.
The About dialogs work for both Last Crusade and Loom. I've made a few changes to our Mac font rendering to get the font rendering (as far as I can tell) pixel perfect.
The rest of the games's dialogs are created from the appropriate DITL resources. I expect most of them to be usable. Some dialogs exist in one version for color and one for black and white, and this is handled. If enhancements are enabled, you can use the mouse scroll wheel on slider widgets.
Images for the dialogs are loaded from the PICT resource.
Old bugs
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Style
Documentation
Chores