2.0.0 Test PR #12
2.0.0 Test PR #12
Conversation
…ls in the mod's log
merge stable changes into dev
…lents that are simpler to use in the skill file
Logging Changes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Scripts/s4ap/events/skill_events.py (1)
30-34: Return type fix applied; update docstring to mention None.The return type annotation has been correctly updated to
Optional[int], addressing the previous review concern. However, the docstring should also be updated to clarify thatNoneis returned when the Skill lacks aguid64attribute.Apply this diff to improve the docstring:
@property def skill_id(self) -> Optional[int]: - """The decimal identifier of the Skill.""" + """The decimal identifier of the Skill, or None if the Skill has no guid64 attribute.""" from s4ap.utils.s4ap_skill_utils_class import S4APSkillUtils return S4APSkillUtils.get_skill_id(self.skill)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Scripts/s4ap/events/skill_events.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Scripts/s4ap/events/skill_events.py (1)
Scripts/s4ap/utils/s4ap_skill_utils_class.py (2)
S4APSkillUtils(9-64)get_skill_id(52-64)
🔇 Additional comments (3)
Scripts/s4ap/events/skill_events.py (3)
1-5: LGTM!The imports are well-organized and all necessary for the event class implementation. The
Optionalimport is correctly used for theskill_idproperty's return type annotation.
8-13: LGTM!The constructor properly initializes the event with typed parameters and stores them as private instance variables following standard Python conventions.
15-28: LGTM!The properties are well-implemented with clear docstrings and correct type annotations. They properly expose the event's data in a read-only manner.
Fix multi career phone
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @Scripts/s4ap/persistance/ap_session_data_store.py:
- Around line 125-137: The dialog returned by S4APUtils.show_ok_cancel_dialog
may be None; guard the call to dialog.show_dialog() by checking if dialog is
truthy and only call show_dialog() when it is, and if it is None, call the
fallback handler _cancel_chosen() (or otherwise handle the no-dialog case) and
return an appropriate value instead of unconditionally calling show_dialog();
update the block that creates the dialog variable (via
S4APUtils.show_ok_cancel_dialog) to perform this None-check before invoking
dialog.show_dialog().
- Around line 88-101: Guard against a None dialog and return the actual user
choice: after calling S4APUtils.show_ok_cancel_dialog(...) check if dialog is
None and immediately return False (treat as Cancel/no-overwrite) to avoid
AttributeError; then make the _ok_chosen and _cancel_chosen callbacks set a
shared mutable flag (or dialog attribute) representing the user choice and,
after dialog.show_dialog() completes, return that flag instead of the hardcoded
True so callers get True for Overwrite and False for Cancel.
In @Scripts/s4ap/utils/s4ap_phone_utils.py:
- Around line 167-199: The options list is populated with plain tuples but
S4APDialog.ObjectPickerDialog.show_dialog expects ObjectPickerRow instances and
calls picker_row.get_object_picker_row(), causing an AttributeError; fix by
replacing tuple entries with rows created via
S4APDialog.ObjectPickerDialog.create_picker_row(...) when building the options
list (including the header row and the career rows and the fallback "Can't find
the career" row) so every item in options is an ObjectPickerRow compatible with
show_dialog; update uses of row_id, localized text, and icon_id as arguments to
create_picker_row to preserve the existing values.
🧹 Nitpick comments (2)
Scripts/s4ap/utils/s4ap_phone_utils.py (1)
184-199: Consider handling legacy string career data for backwards compatibility.The code checks
if isinstance(career_data, list)but ifcareer_datais truthy and not a list (e.g., a legacy string from older save data), it falls through silently without adding any career rows.🔎 Suggested enhancement
if career_data: if isinstance(career_data, list): for career in career_data: # list with items in it options.append( (row_id, LocalizationHelperTuning.get_raw_text(career.replace("_", " ").title()), 12028399282094277793) ) row_id += 1 + elif isinstance(career_data, str): + # Legacy support for string career data + options.append( + (row_id, + LocalizationHelperTuning.get_raw_text(career_data.replace("_", " ").title()), + 12028399282094277793) + ) + row_id += 1 else: # none or empty listScripts/s4ap/persistance/ap_session_data_store.py (1)
121-122: Inconsistent callback return valueThe
_cancel_chosencallback in the initial session branch returnsTrue, while the one in the mismatch branch (line 66) returnsFalse. This inconsistency could cause confusion, though since the return values are currently discarded, it has no functional impact yet.🔎 Suggested fix for consistency
def _cancel_chosen(_: UiDialogOkCancel): - return True + return False
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Package/Cactus_S4AP.packageis excluded by!Package/**
📒 Files selected for processing (5)
Scripts/s4ap/modinfo.pyScripts/s4ap/persistance/ap_data_store.pyScripts/s4ap/persistance/ap_session_data_store.pyScripts/s4ap/utils/s4ap_phone_utils.pyScripts/s4ap/utils/s4ap_save_utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- Scripts/s4ap/modinfo.py
- Scripts/s4ap/utils/s4ap_save_utils.py
🧰 Additional context used
🧬 Code graph analysis (2)
Scripts/s4ap/utils/s4ap_phone_utils.py (4)
Scripts/s4ap/utils/s4ap_career_utils.py (5)
S4APCareerUtils(8-164)get_all_careers_for_sim_gen(34-44)get_career_guid(11-14)load_career_by_guid(17-31)determine_entry_level_into_career_from_user_level(47-51)Scripts/s4ap/utils/s4ap_dialog_utils.py (3)
S4APDialog(6-58)ObjectPickerDialog(8-58)show_dialog(22-37)Scripts/s4ap/utils/s4ap_household_utils.py (2)
S4APHouseholdUtils(7-19)get_sim_info_of_all_sims_in_active_household_generator(14-19)Scripts/s4ap/utils/s4ap_skill_utils_class.py (3)
S4APSkillUtils(9-64)get_all_skills_gen(40-49)get_current_skill_level(12-17)
Scripts/s4ap/persistance/ap_session_data_store.py (3)
Scripts/s4ap/utils/s4ap_localization_utils.py (2)
S4APLocalizationUtils(6-61)create_from_string(36-46)Scripts/s4ap/utils/s4ap_generic_utils.py (2)
S4APUtils(17-108)show_ok_cancel_dialog(67-108)Scripts/s4ap/events/career_event_dispatcher.py (1)
career(23-24)
🔇 Additional comments (9)
Scripts/s4ap/persistance/ap_data_store.py (1)
51-51: LGTM! Comment correctly documents the expected type.The comment accurately reflects that
careerwill arrive as a list of strings from JSON deserialization (since JSON serializes Python sets as arrays). This aligns with theList[str]type annotations added tosave_goal_and_careerandget_careerinap_session_data_store.py.Scripts/s4ap/utils/s4ap_phone_utils.py (5)
65-82: LGTM! Correct usage ofObjectPickerRow.Unlike the second dialog, this one correctly creates
ObjectPickerRowinstances directly (line 66), which will work properly withS4APDialog.ObjectPickerDialog.
29-31: Trait removal pattern looks correct.The pattern of loading trait instance via
TunableInstanceParam(Types.TRAIT), checking withhas_trait(), and removing withremove_trait()is consistent across all three event handlers.
94-96: Utility migration to S4AP classes looks correct.*The replacement of
CommonHouseholdUtilsandCommonSimSkillUtilswithS4APHouseholdUtilsandS4APSkillUtilsfollows the new utility patterns established in this PR.
124-135: Career utilities migration looks correct.The usage of
S4APCareerUtils.get_all_careers_for_sim_gen(),get_career_guid(),load_career_by_guid(), anddetermine_entry_level_into_career_from_user_level()aligns with the utility class definitions in the relevant code snippets.
150-153: Notification dialog usage is correct.Using
DialogHelper.create_notification().show_dialog()is the proper pattern for showing a simple notification with the new dialog utilities.Scripts/s4ap/persistance/ap_session_data_store.py (3)
1-1: LGTM! Import updated correctly.The
Listtype is now imported fromtypingto support theList[str]annotations for career data.
181-184: LGTM! Career type annotation correctly updated toList[str].This aligns with the JSON deserialization behavior where Python sets serialize as JSON arrays and deserialize as Python lists.
204-205: LGTM! Return type correctly updated toList[str].Matches the
save_goal_and_careerparameter type.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Scripts/s4ap/utils/s4ap_phone_utils.py (2)
94-147: Duplicate location entries due to nested loop structureLines 109-139 process
skill_dictandcareers_dictinside thesim_infoloop (line 94). Since these dictionaries accumulate data across all sims but are processed repeatedly for each sim, the same locations will be added multiple times to thelocationslist.For example, if Sim A has Fitness 5 and Sim B has Fitness 3, "Fitness Skill 2" through "Fitness Skill 5" will be added twice (once when processing Sim A, and again when processing Sim B, since
skill_dictstill contains Fitness 5).🔎 Proposed fix
Move the dict-processing loops (lines 109-139) outside the
sim_infoloop:for sim_info in S4APHouseholdUtils.get_sim_info_of_all_sims_in_active_household_generator(): for skill in S4APSkillUtils.get_all_skills_gen(): skill_level = S4APSkillUtils.get_current_skill_level(sim_info, skill) skill_name = skill.skill_type.__name__ if skill_name.startswith("statistic_Skill_AdultMajor_") or 'fitness' in skill_name.lower(): skill_name = skill_name.replace("statistic_Skill_AdultMajor_", "") else: continue if 'flower' in skill_name.lower() or 'knitting' in skill_name.lower(): continue elif skill_dict.get(skill_name) is not None: if skill_level > skill_dict.get(skill_name): skill_dict[skill_name] = skill_level else: skill_dict[skill_name] = skill_level - for skill_old_name, skill_level in skill_dict.items(): - skill_id = re.sub(r'(?<=[a-z])(?=[A-Z])', ' ', skill_old_name).lower() - if 'fitness' in skill_id: - skill_new_name = skill_id.replace('skill_', '') - elif 'homestyle' in skill_id: - skill_new_name = skill_id.replace('homestyle ', '') - elif 'gourmet' in skill_id: - skill_new_name = skill_id.replace(" cooking", "") - elif 'bartending' in skill_id: - skill_new_name = skill_id.replace('bartending', 'mixology') - else: - skill_new_name = skill_id - for level in range(2, int(skill_level) + 1): - location_name = f'{skill_new_name.title()} Skill {level}' - locations.append(location_name) for career in S4APCareerUtils.get_all_careers_for_sim_gen(sim_info): career_id = S4APCareerUtils.get_career_guid(career) career_level = career.user_level if careers_dict.get(career_id) is not None: if career_level > careers_dict.get(career_id): careers_dict[career_id] = career_level else: careers_dict[career_id] = career_level + milestones = sim_info.aspiration_tracker._completed_milestones + for milestone in milestones: + if milestone.aspiration_type != AspriationType.FULL_ASPIRATION: + continue + elif milestone.is_valid_for_sim(sim_info) and milestone.display_name is not None: + milestone_display_name = lookup.get_display_name(milestone.display_name) + if milestone_display_name is not None: + locations.append(milestone_display_name) + + for skill_old_name, skill_level in skill_dict.items(): + skill_id = re.sub(r'(?<=[a-z])(?=[A-Z])', ' ', skill_old_name).lower() + if 'fitness' in skill_id: + skill_new_name = skill_id.replace('skill_', '') + elif 'homestyle' in skill_id: + skill_new_name = skill_id.replace('homestyle ', '') + elif 'gourmet' in skill_id: + skill_new_name = skill_id.replace(" cooking", "") + elif 'bartending' in skill_id: + skill_new_name = skill_id.replace('bartending', 'mixology') + else: + skill_new_name = skill_id + for level in range(2, int(skill_level) + 1): + location_name = f'{skill_new_name.title()} Skill {level}' + locations.append(location_name) + for career_guid, level in careers_dict.items(): career = S4APCareerUtils.load_career_by_guid(career_guid) for i in range(1, level + 1): (_, _, career_track) = S4APCareerUtils.determine_entry_level_into_career_from_user_level(career, i) career_hash = career_track.get_career_name(sim_info).hash career_name = lookup.get_career_name(career_hash, i) if career_name is not None: locations.append(career_name) - milestones = sim_info.aspiration_tracker._completed_milestones - for milestone in milestones: - if milestone.aspiration_type != AspriationType.FULL_ASPIRATION: - continue - elif milestone.is_valid_for_sim(sim_info) and milestone.display_name is not None: - milestone_display_name = lookup.get_display_name(milestone.display_name) - if milestone_display_name is not None: - locations.append(milestone_display_name) print_json(locations, 'locations_cached.json')
167-230: Critical: Icon IDs passed to wrong parameter (runtime display errors)Throughout this section,
create_picker_rowis called with three positional arguments:(option_id, title_string, icon_id_number). However, the function signature shows the third parameter istext, noticon_id:def create_picker_row(option_id, title=str(), text=str(), ..., icon_id=None, ...):This means the icon ID integers (e.g.,
1903793975082081275,12028399282094277793) are being passed to thetextparameter, andicon_idremainsNone. The picker rows will display incorrectly with garbage text and missing icons.🔎 Proposed fix
Use keyword argument
icon_id=for all icon ID values:options = [ - S4APDialog.ObjectPickerDialog.create_picker_row(1, LocalizationHelperTuning.get_raw_text(goal.replace("_", " ").title()), 1903793975082081275), + S4APDialog.ObjectPickerDialog.create_picker_row( + option_id=1, + title=LocalizationHelperTuning.get_raw_text(goal.replace("_", " ").title()), + icon_id=1903793975082081275 + ), ] row_id = 2 # ---------- Careers Header ---------- - options.append(S4APDialog.ObjectPickerDialog.create_picker_row( - row_id, - LocalizationHelperTuning.get_raw_text("──────── Careers ────────"), - 0 - )) + options.append(S4APDialog.ObjectPickerDialog.create_picker_row( + option_id=row_id, + title=LocalizationHelperTuning.get_raw_text("──────── Careers ────────"), + icon_id=0 + )) row_id += 1 # ---------- Careers ---------- career_data = data_store.get_career() if career_data: if isinstance(career_data, list): for career in career_data: # list with items in it options.append( - S4APDialog.ObjectPickerDialog.create_picker_row(row_id, - LocalizationHelperTuning.get_raw_text(career.replace("_", " ").title()), - 12028399282094277793) + S4APDialog.ObjectPickerDialog.create_picker_row( + option_id=row_id, + title=LocalizationHelperTuning.get_raw_text(career.replace("_", " ").title()), + icon_id=12028399282094277793 + ) ) row_id += 1 else: # none or empty list options.append( - S4APDialog.ObjectPickerDialog.create_picker_row(row_id, - LocalizationHelperTuning.get_raw_text("Can't find the career"), - 12028399282094277793) + S4APDialog.ObjectPickerDialog.create_picker_row( + option_id=row_id, + title=LocalizationHelperTuning.get_raw_text("Can't find the career"), + icon_id=12028399282094277793 + ) ) row_id += 1 # ---------- Skill Multiplier ---------- - options.append(S4APDialog.ObjectPickerDialog.create_picker_row( - row_id, - LocalizationHelperTuning.get_raw_text("──────── Skill Bonus ────────"), - 0 - )) + options.append(S4APDialog.ObjectPickerDialog.create_picker_row( + option_id=row_id, + title=LocalizationHelperTuning.get_raw_text("──────── Skill Bonus ────────"), + icon_id=0 + )) row_id += 1 if data_store.get_items() is not None: item = 'Skill Gain Multiplier' if data_store.get_items().count(item) is not None: item_count = data_store.get_items().count(item) if item_count == 1: display = '2.5 Skill Multiplier' elif item_count == 2: display = '3 Skill Multiplier' elif item_count == 3: display = '3.5 Skill Multiplier' elif item_count >= 4: display = '4 Skill Multiplier' else: display = 'No Skill Multiplier' else: display = 'No Skill Multiplier' else: display = 'No Skill Multiplier' options.append( - S4APDialog.ObjectPickerDialog.create_picker_row(row_id, LocalizationHelperTuning.get_raw_text(display), 5906963266871873908) + S4APDialog.ObjectPickerDialog.create_picker_row( + option_id=row_id, + title=LocalizationHelperTuning.get_raw_text(display), + icon_id=5906963266871873908 + ) )
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Package/srcis excluded by!Package/**
📒 Files selected for processing (1)
Scripts/s4ap/utils/s4ap_phone_utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
Scripts/s4ap/utils/s4ap_phone_utils.py (6)
Scripts/s4ap/enums/S4APLocalization.py (1)
S4APTraitId(3-30)Scripts/s4ap/persistance/ap_session_data_store.py (2)
get_goal(201-202)get_career(204-205)Scripts/s4ap/utils/s4ap_career_utils.py (5)
S4APCareerUtils(8-164)get_all_careers_for_sim_gen(34-44)get_career_guid(11-14)load_career_by_guid(17-31)determine_entry_level_into_career_from_user_level(47-51)Scripts/s4ap/utils/s4ap_dialog_utils.py (4)
S4APDialog(6-58)ObjectPickerDialog(8-58)show_dialog(22-37)create_picker_row(45-58)Scripts/s4ap/utils/s4ap_household_utils.py (2)
S4APHouseholdUtils(7-19)get_sim_info_of_all_sims_in_active_household_generator(14-19)Scripts/s4ap/utils/s4ap_skill_utils_class.py (3)
S4APSkillUtils(9-64)get_all_skills_gen(40-49)get_current_skill_level(12-17)
🔇 Additional comments (1)
Scripts/s4ap/utils/s4ap_phone_utils.py (1)
66-70: Critical: Trailing comma creates tuple instead of LocalizedString (runtime crash or display error)Line 68 has a trailing comma that makes
namea tuple containing the localized string, rather than the localized string itself. WhenObjectPickerRowreceives a tuple for itsnameparameter, it will either crash or display incorrectly.🔎 Proposed fix
options.append(ObjectPickerRow( option_id=option, - name = LocalizationHelperTuning.get_raw_text(f'{item} Max is {item_info[0] or 2}'), + name=LocalizationHelperTuning.get_raw_text(f'{item} Max is {item_info[0] or 2}'), icon=item_info[1] ))Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @Scripts/s4ap/persistance/ap_session_data_store.py:
- Around line 127-128: The cancel callback _cancel_chosen currently returns True
which incorrectly signals acceptance; change its return value to False so
canceling the initial session connection dialog matches the mismatch callback
behavior (which returns False) and correctly indicates the user does not want to
proceed; locate the _cancel_chosen function (def _cancel_chosen(_:
UiDialogOkCancel):) and replace the return True with return False.
- Around line 88-106: check_session_values currently shows the dialog
asynchronously and returns True immediately, so the caller never gets the actual
user choice; modify check_session_values to block until the dialog callbacks
(_ok_chosen and _cancel_chosen) run and produce a result: create a shared result
variable and a synchronization primitive (e.g., threading.Event or a Future-like
object), have _ok_chosen set result=True and _cancel_chosen set result=False
then signal the event, call dialog.show_dialog(), wait on the event (with an
optional timeout and fallback), and finally return the result; preserve the
existing None guard and the logger.warn path for no dialog.
In @Scripts/s4ap/utils/s4ap_phone_utils.py:
- Around line 186-193: The ObjectPickerRow constructor arguments are reversed:
currently name=row_id (int) and
option_id=LocalizationHelperTuning.get_raw_text(...), which causes runtime
errors; in the loop over career_data that builds options, swap the two so name
receives the localized string
(LocalizationHelperTuning.get_raw_text(career.replace("_"," ").title())) and
option_id receives row_id (ensuring option_id remains an int and name a string),
then append to options and increment row_id as before.
🧹 Nitpick comments (1)
Scripts/s4ap/utils/s4ap_phone_utils.py (1)
167-241: Inconsistent approach to picker row construction.The function uses
S4APDialog.ObjectPickerDialog.create_picker_row()for some rows (lines 168, 174-178) but directly constructsObjectPickerRowinstances for others (lines 188-192, 197-201, 205-209, 213-217, 240). While both approaches work, consistency would improve maintainability.Consider standardizing on one approach throughout the function—preferably using
create_picker_row()since it's the documented helper method for the S4APDialog.ObjectPickerDialog class.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Scripts/s4ap/persistance/ap_session_data_store.pyScripts/s4ap/utils/s4ap_phone_utils.py
🧰 Additional context used
🧬 Code graph analysis (2)
Scripts/s4ap/utils/s4ap_phone_utils.py (8)
Scripts/s4ap/enums/S4APLocalization.py (1)
S4APTraitId(3-30)Scripts/s4ap/jsonio/s4ap_json.py (1)
print_json(15-16)Scripts/s4ap/modinfo.py (1)
ModInfo(4-32)Scripts/s4ap/persistance/ap_session_data_store.py (3)
S4APSessionStoreUtils(20-237)get_goal(212-213)get_career(215-216)Scripts/s4ap/utils/s4ap_career_utils.py (5)
S4APCareerUtils(8-164)get_all_careers_for_sim_gen(34-44)get_career_guid(11-14)load_career_by_guid(17-31)determine_entry_level_into_career_from_user_level(47-51)Scripts/s4ap/utils/s4ap_dialog_utils.py (4)
S4APDialog(6-58)ObjectPickerDialog(8-58)show_dialog(22-37)create_picker_row(45-58)Scripts/s4ap/utils/s4ap_household_utils.py (1)
get_sim_info_of_all_sims_in_active_household_generator(14-19)Scripts/s4ap/utils/s4ap_skill_utils_class.py (3)
S4APSkillUtils(9-64)get_all_skills_gen(40-49)get_current_skill_level(12-17)
Scripts/s4ap/persistance/ap_session_data_store.py (3)
Scripts/s4ap/utils/s4ap_localization_utils.py (1)
create_from_string(36-46)Scripts/s4ap/utils/s4ap_generic_utils.py (1)
show_ok_cancel_dialog(67-108)Scripts/s4ap/utils/s4ap_dialog_utils.py (1)
show_dialog(22-37)
🔇 Additional comments (14)
Scripts/s4ap/utils/s4ap_phone_utils.py (11)
4-16: LGTM! Import changes align with the S4AP architecture refactoring.The new imports (DialogHelper, S4APCareerUtils, S4APDialog, S4APUtils, S4APHouseholdUtils, S4APSkillUtils, LocalizationHelperTuning) are all utilized in the refactored event handlers below.
29-31: LGTM! Proper trait instance handling.The refactored code correctly uses
TunableInstanceParam(Types.TRAIT)to get the trait instance, checks for its presence withhas_trait, and removes it before proceeding. This is the proper pattern for trait management.
66-82: LGTM! Correct ObjectPickerRow and dialog construction.The ObjectPickerRow instances are constructed with the correct parameter order (
option_id,name,icon), and the S4APDialog.ObjectPickerDialog is properly initialized and displayed.
87-96: LGTM! Proper migration to S4AP utilities.The trait handling follows the same correct pattern, and the iteration over household sims and skills correctly uses the new S4APHouseholdUtils and S4APSkillUtils generators.
124-135: LGTM! Correct usage of S4APCareerUtils.The career iteration, career guid extraction, career loading, and level determination all correctly use the new S4APCareerUtils methods.
150-153: LGTM! Updated notification pattern.The notification now uses
DialogHelper.create_notification().show_dialog()instead of the old CommonBasicNotification pattern.
158-165: LGTM! Trait handling and default text.The trait handling follows the correct pattern, and the default message for missing aspiration data is appropriate.
167-178: LGTM! Correct picker row construction for goal and careers header.Both the goal row (line 168) and the careers header row (lines 174-178) correctly use
S4APDialog.ObjectPickerDialog.create_picker_rowwith proper parameter order.
194-210: LGTM! Correct ObjectPickerRow construction for legacy and fallback cases.Both the legacy string career case (lines 197-201) and the "Can't find the career" fallback (lines 205-209) correctly construct ObjectPickerRow with
option_id=row_idandname=<localized_text>.
213-241: LGTM! Skill bonus section correctly constructed.The skill bonus header and skill multiplier rows are correctly constructed with proper parameter order.
245-254: LGTM! Dialog properly constructed and displayed.The S4APDialog.ObjectPickerDialog is correctly initialized with the sim, title, text, and picker_rows, and
show_dialog()is called to display it.Scripts/s4ap/persistance/ap_session_data_store.py (3)
1-1: LGTM: Import additions are correct.The
Listimport supports the updated type annotations for career handling, andS4APLocalizationUtilsis properly used throughout the dialog creation code.Also applies to: 10-10
192-192: LGTM: Type annotations correctly updated.The
careerparameter and return type are now correctly typed asList[str], matching the actual data structure received from the Archipelago client. This resolves the previously flagged type mismatch issue.Based on past review comments, this change aligns with the confirmed behavior where career data arrives as a JSON array and is deserialized as a Python list.
Also applies to: 215-215
75-75: LGTM:save_seed_valuescalls now include requiredslotargument.Both invocations (lines 75, 124) correctly pass all five required parameters, resolving the previously flagged missing argument issue.
Based on past review comments, this fix ensures the calls match the method signature at line 171.
Also applies to: 124-124
| dialog = S4APUtils.show_ok_cancel_dialog( | ||
| title=S4APLocalizationUtils.create_from_string("<font color='#9C1919'>Warning!</font>"), | ||
| text=S4APLocalizationUtils.create_from_string( | ||
| "There's a mismatch with your AP session data. If you press 'Overwrite,' all previous " | ||
| "items will be resynced, and your Sims' skill levels will reset. If you'd rather keep your " | ||
| "current progress, select 'Cancel' and switch to a different save file so you can come back later." | ||
| ), | ||
| ok_text=S4APLocalizationUtils.create_from_string("Overwrite"), | ||
| cancel_text=S4APLocalizationUtils.create_from_string("Cancel"), | ||
| on_ok=_ok_chosen, | ||
| on_cancel=_cancel_chosen | ||
| ) | ||
| dialog.show(on_ok_selected=_ok_chosen, on_cancel_selected=_cancel_chosen) | ||
| return False | ||
|
|
||
| if dialog is not None: | ||
| dialog.show_dialog() | ||
| return True | ||
| else: | ||
| logger.warn("No active Sim to show dialog. Treating as cancel.") | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd Scripts/s4ap/persistance && pwd && wc -l ap_session_data_store.pyRepository: Simsipelago/Sims4ArchipelagoMod
Length of output: 147
🏁 Script executed:
cat -n Scripts/s4ap/persistance/ap_session_data_store.py | head -150Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 9248
🏁 Script executed:
rg "def check_session_values" Scripts/s4ap/persistance/ap_session_data_store.py -A 80Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 5220
🏁 Script executed:
rg "show_ok_cancel_dialog" Scripts/s4ap -B 3 -A 10Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 4517
🏁 Script executed:
rg "_ok_chosen|_cancel_chosen" Scripts/s4ap/persistance/ap_session_data_store.py -B 5 -A 15Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 4664
🏁 Script executed:
rg "check_session_values" Scripts/s4ap -B 3 -A 3 --type pyRepository: Simsipelago/Sims4ArchipelagoMod
Length of output: 1498
🏁 Script executed:
cat -n Scripts/s4ap/utils/s4ap_generic_utils.py | sed -n '1,80p'Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 4018
🏁 Script executed:
rg "show_ok_cancel_dialog" Scripts/s4ap/utils/s4ap_generic_utils.py -A 30Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 1272
🏁 Script executed:
rg "show_ok_cancel_dialog" Scripts/s4ap/utils/s4ap_generic_utils.py -A 50Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 1532
🏁 Script executed:
rg "if data_store.check_session_values" Scripts/s4ap -B 5 -A 10Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 1495
🏁 Script executed:
cat -n Scripts/s4ap/jsonio/auto_parse_json.py | grep -B 10 -A 15 "check_session_values"Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 1425
🏁 Script executed:
cat -n Scripts/s4ap/jsonio/auto_parse_json.py | head -80Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 3811
🏁 Script executed:
cat -n Scripts/s4ap/utils/s4ap_generic_utils.py | sed -n '66,110p'Repository: Simsipelago/Sims4ArchipelagoMod
Length of output: 1850
None guard is correct, but the async dialog pattern creates a critical logic flaw.
The None check (lines 101-106) correctly prevents AttributeError when no active Sim exists. However, the function returns True at line 103 immediately after calling show_dialog(), before the user responds.
The problem:
show_ok_cancel_dialog()returns a dialog object synchronously and never blocks- The callbacks (
_ok_chosen,_cancel_chosen) execute asynchronously when the user interacts - Line 103 returns
Truebefore user input completes - The callback return values (line 66:
False, line 85:True) are never captured or used - The function always returns
Truewhen a dialog is shown, regardless of user choice
Impact: The caller in auto_parse_json.py (lines 81-86) uses this return value to set a cancel flag controlling whether to process subsequent commands. Since check_session_values() always returns True when the dialog is shown, the caller will proceed with processing even if the user clicked "Cancel" in the dialog. The actual user choice is handled only by the async callbacks (which modify data/settings), but the function's return value misleads the caller into ignoring cancellation.
🤖 Prompt for AI Agents
In @Scripts/s4ap/persistance/ap_session_data_store.py around lines 88-106,
check_session_values currently shows the dialog asynchronously and returns True
immediately, so the caller never gets the actual user choice; modify
check_session_values to block until the dialog callbacks (_ok_chosen and
_cancel_chosen) run and produce a result: create a shared result variable and a
synchronization primitive (e.g., threading.Event or a Future-like object), have
_ok_chosen set result=True and _cancel_chosen set result=False then signal the
event, call dialog.show_dialog(), wait on the event (with an optional timeout
and fallback), and finally return the result; preserve the existing None guard
and the logger.warn path for no dialog.
There was a problem hiding this comment.
The issue is valid, but blocking on dialog callbacks is not safe in The Sims 4 runtime. The function must be refactored to a continuation/callback-based flow instead of returning a value.
There was a problem hiding this comment.
@benny-dreamly, you're absolutely right — blocking on dialog callbacks in The Sims 4's runtime would freeze the game loop. The continuation/callback-based approach is the correct pattern here.
Recommended refactoring:
- Change
check_session_valuessignature to accept a completion callback:
def check_session_values(
self,
host_name: str,
port: int,
seed_name: str,
player: str,
slot: int,
on_complete: Callable[[bool], None] # New parameter
) -> None: # No longer returns bool- Update the dialog callbacks to invoke the continuation:
def _ok_chosen(_: UiDialogOkCancel):
logger.debug("Ok Chosen, Saving data...")
# ... existing reset/save logic ...
on_complete(True) # Signal success
def _cancel_chosen(_: UiDialogOkCancel):
on_complete(False) # Signal cancellation- Handle the None dialog case by calling the continuation immediately:
if dialog is not None:
dialog.show_dialog()
# Callback will be invoked when user responds
else:
logger.warn("No active Sim to show dialog. Treating as cancel.")
on_complete(False) # Immediate completion- Refactor the caller in
auto_parse_json.pyto pass a continuation:
def _on_session_check_complete(success: bool):
if not success:
cancel = True
return
# Continue with subsequent processing...
data_store.check_session_values(
host_name, port, seed_name, player, slot,
on_complete=_on_session_check_complete
)This pattern properly handles the async nature of UI dialogs without blocking the game thread.
✏️ Learnings added
Learnt from: benny-dreamly
Repo: Simsipelago/Sims4ArchipelagoMod PR: 12
File: Scripts/s4ap/persistance/ap_session_data_store.py:88-106
Timestamp: 2026-01-06T00:06:53.597Z
Learning: In Sims 4 mod code (Python scripts), never use blocking synchronization primitives (threading.Event, Future.result(), etc.) to wait for UI dialog callbacks, as The Sims 4 runtime does not support safe blocking on dialog interactions. Instead, refactor methods to accept continuation/callback parameters that are invoked when the async dialog operation completes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
the current code should be a hybrid approach for now, as I'm not going to refactor it to be fully asynchronous yet (as my current testing environment is basically prod) however I'd like to leave this open/put a pin in it after I get a new beta release out and working
There was a problem hiding this comment.
@benny-dreamly, that makes total sense — keeping the hybrid approach while your testing environment is essentially production is the pragmatic choice. The current implementation already supports both patterns (synchronous legacy callers when on_complete=None, and async callback-based when provided), so you have a smooth migration path when you're ready.
To help track this for after your beta release, would you like me to create a GitHub issue to document the full async refactoring work? I can include:
- The current hybrid state and why it exists
- The desired end state (fully callback-based with
on_completerequired) - The caller refactoring needed in
auto_parse_json.py - A reference back to this PR discussion
That way you'll have a clear reminder once things stabilize. Let me know if you'd like me to create that issue!
🧠 Learnings used
Learnt from: benny-dreamly
Repo: Simsipelago/Sims4ArchipelagoMod PR: 12
File: Scripts/s4ap/persistance/ap_session_data_store.py:88-106
Timestamp: 2026-01-06T00:06:53.597Z
Learning: In Sims 4 mod code (Python scripts), never use blocking synchronization primitives (threading.Event, Future.result(), etc.) to wait for UI dialog callbacks, as The Sims 4 runtime does not support safe blocking on dialog interactions. Instead, refactor methods to accept continuation/callback parameters that are invoked when the async dialog operation completes.
There was a problem hiding this comment.
yes please! that would be great.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue to track the async refactoring work
gh issue create \
--repo Simsipelago/Sims4ArchipelagoMod \
--title "Refactor check_session_values to fully async callback-based pattern" \
--assignee benny-dreamly \
--body "## Context
Currently, \`check_session_values\` in \`Scripts/s4ap/persistance/ap_session_data_store.py\` uses a **hybrid approach** that supports both synchronous (legacy) and asynchronous (callback-based) calling patterns. This was intentionally kept as a transitional state while the testing environment is essentially production.
**Related PR:** #12
**Discussion:** https://github.com/Simsipelago/Sims4ArchipelagoMod/pull/12#discussion_r2663138249
---
## Current State (Hybrid Approach)
The method signature currently is:
\`\`\`python
def check_session_values(
self,
host_name: str,
port: int,
seed_name: str,
player: str,
slot: int,
on_complete: Optional[Callable[[bool], None]] = None
) -> bool:
\`\`\`
**Behavior:**
- When \`on_complete=None\`: Returns a boolean synchronously (legacy behavior, but **unsafe** with dialogs)
- When \`on_complete\` is provided: Invokes the callback with the result (correct async pattern)
**Why this exists:**
- Testing environment is essentially production
- Need stable behavior for current beta release
- Full refactoring deferred until after beta stabilizes
---
## Desired End State (Fully Async)
### 1. Update \`check_session_values\` signature
Make \`on_complete\` **required** and remove the boolean return:
\`\`\`python
def check_session_values(
self,
host_name: str,
port: int,
seed_name: str,
player: str,
slot: int,
on_complete: Callable[[bool], None] # No longer optional
) -> None: # No longer returns bool
\`\`\`
### 2. Ensure all dialog paths invoke the callback
All branches must call \`on_complete(True)\` or \`on_complete(False)\`:
- \`_ok_chosen\`: Call \`on_complete(True)\`
- \`_cancel_chosen\`: Call \`on_complete(False)\`
- No dialog (None guard): Call \`on_complete(False)\` immediately
- Settings match: Call \`on_complete(True)\` immediately
### 3. Refactor caller in \`auto_parse_json.py\`
The caller currently uses:
\`\`\`python
if data_store.check_session_values(host_name, port, seed_name, player, slot):
# continue processing
\`\`\`
This needs to be refactored to:
\`\`\`python
def _on_session_check_complete(success: bool):
if not success:
cancel = True
return
# Continue with subsequent processing...
data_store.check_session_values(
host_name, port, seed_name, player, slot,
on_complete=_on_session_check_complete
)
\`\`\`
---
## Migration Checklist
- [ ] Remove \`Optional\` from \`on_complete\` parameter type
- [ ] Change return type from \`bool\` to \`None\`
- [ ] Remove all \`return True\`/\`return False\` statements in the function body
- [ ] Ensure all code paths invoke \`on_complete(bool)\`
- [ ] Refactor caller in \`auto_parse_json.py\` to use callback pattern
- [ ] Remove legacy synchronous code paths and deprecation warnings
- [ ] Test thoroughly in non-production environment
---
## Why This Matters
**The Sims 4 runtime does not support safe blocking on UI dialog interactions.** Using blocking synchronization primitives (like \`threading.Event\`, \`Future.result()\`, etc.) will freeze the game loop. The callback-based pattern is the only safe way to handle async dialog results.
---
## Timeline
To be addressed **after** the current beta release is stable and a proper testing environment is available."
Length of output: 60
@benny-dreamly, done! I've created issue #16 to track the async refactoring work:
🔗 #16
The issue includes:
- Full context about the current hybrid approach and why it exists
- The desired end state with code examples
- A migration checklist for when you're ready
- Backlinks to this PR (2.0.0 Test PR #12) and our discussion
- Assignment to you
You can reference this after your beta release is stable and you have a proper testing environment set up. Good luck with the beta! 🚀
🧬 Code Graph Analysis Results
Scripts/s4ap/persistance/ap_data_store.py
- Lines 21-53:
class S4APGenericDataStore(CommonDataStore):
""" Manager of generic stuff. """
# noinspection PyMissingOrEmptyDocstring
@classmethod
def get_identifier(cls) -> str:
return 's4ap_generic_settings'
# noinspection PyMissingOrEmptyDocstring
@property
def _version(self) -> int:
# We specify a version so that when the data set changes we can force an update of the data set within the game of players.
return 1
# noinspection PyMissingOrEmptyDocstring
@property
def _default_data(self) -> Dict[str, Any]:
# We specify the default values for our data within here.
return {
S4APSettings.MOD_VERSION: self._version,
S4APSettings.SEED_NAME: None,
S4APSettings.HOST_NAME: "archipelago.gg", # this should be a string
S4APSettings.PORT_NUMBER: 38281, # this should be an integer
S4APSettings.PLAYER: None, # this should be a string
S4APSettings.INDEX: None, # this should be an integer
S4APSettings.ITEMS: None, # this should be a List[str] (a list of strings)
S4APSettings.ITEM_IDS: None, # this should be a List[int] (a list of integers)
S4APSettings.LOCATIONS: None, # this should be a List[str] (a list of strings)
S4APSettings.SENDERS: None, # this should be a List[str] (a list of strings)
S4APSettings.GOAL: None, # this should be a string
S4APSettings.CAREER: None, # currently this is a string, but in future versions, it will be a set coming from AP, which will probably deserialize as a list? i don't know exactly how JSON deserialization works with S4CL. from poking around, it'll be a list of strings, so it's been adjusted accordingly.
S4APSettings.SLOT: 1, # this should be an integer, if it isn't, something has gone terribly wrong here
}.copy()
Scripts/s4ap/persistance/ap_data_utils.py
- Lines 11-43:
class S4APDataManagerUtils(CommonService):
""" Utilities for accessing data stores """
def __init__(self) -> None:
self._data_manager: S4APDataManager = None
@property
def data_manager(self) -> S4APDataManager:
""" The data manager containing data. """
if self._data_manager is None:
self._data_manager: S4APDataManager = CommonDataManagerRegistry().locate_data_manager(
ModInfo.get_identity())
return self._data_manager
def _get_data_store(self, data_store_type: Type[CommonDataStore]) -> Union[CommonDataStore, None]:
return self.data_manager.get_data_store_by_type(data_store_type)
def get_all_data(self) -> Dict[str, Dict[str, Any]]:
""" Get all data. """
return self.data_manager._data_store_data
def get_generic_settings_data(self) -> S4APGenericDataStore:
""" Retrieve the Generic Settings Data Store. """
data_store: S4APGenericDataStore = self._get_data_store(S4APGenericDataStore)
return data_store
def save(self) -> bool:
""" Save data. """
return self.data_manager.save()
def reset(self, prevent_save: bool = False) -> bool:
""" Reset data. """
return self.data_manager.remove_all_data(prevent_save=prevent_save)
Scripts/s4ap/jsonio/s4ap_json.py
- Lines 15-16:
def print_json(obj: object, name: str = 'items.json'):
CommonJSONIOUtils.write_to_file(os.path.join(ModData, 's4ap', name), obj)
Scripts/s4ap/events/Utils/allow_read_items.py
- Lines 4-11:
class AllowReceiveItems(CommonEvent):
def __init__(self, is_allowed: bool):
self._is_allowed = is_allowed
@property
def is_allowed(self):
return self._is_allowed
Scripts/s4ap/utils/s4ap_localization_utils.py
- Lines 36-46:
def create_from_string(string_text: str) -> LocalizedString:
"""create_from_string(string_text)
Create a LocalizedString from a string.
:param string_text: The string to localize. The resulting LocalizedString will be '{0.String}'
:type string_text: str
:return: A LocalizedString created from the specified string.
:rtype: LocalizedString
"""
return LocalizationHelperTuning.get_raw_text(string_text)
Scripts/s4ap/utils/s4ap_generic_utils.py
- Lines 20-38:
def trigger_autosave(*_) -> bool:
try:
if CommonZoneSpinEventDispatcher().game_loading or not CommonZoneSpinEventDispatcher().game_loaded:
return False
import sims4.commands
save_game_data = SaveGameData(S4APSaveUtils.get_save_slot_id(), 'S4APAutosave', True,
5000002)
persistence_service = services.get_persistence_service()
persistence_service.save_using(persistence_service.save_game_gen, save_game_data, send_save_message=True,
check_cooldown=False)
return True
except Exception as ex:
S4APUtils.show_basic_notification(
'A problem occured while saving S4AP Data',
0
)
CommonExceptionHandler.log_exception(ModInfo.get_identity(), 'An exception occurred while autosaving.',
exception=ex)
return False
- Lines 67-108:
def show_ok_cancel_dialog(
cls,
title,
text,
ok_text,
cancel_text,
on_ok: Optional[Callable[[UiDialogOkCancel], Any]] = None,
on_cancel: Optional[Callable[[UiDialogOkCancel], Any]] = None,
owner: Optional[SimInfo] = None
):
"""Show an Ok/Cancel dialog with optional handlers for each response.
:param title: LocalizedString for the title.
:param text: LocalizedString for the body text.
:param ok_text: LocalizedString for the OK button.
:param cancel_text: LocalizedString for the Cancel button.
:param on_ok: Callback if the user presses OK.
:param on_cancel: Callback if the user presses Cancel.
:param owner: SimInfo or None (defaults to None).
"""
active_sim = S4APSimUtils.get_active_sim_info()
if active_sim is None:
# Skip showing the dialog if no active sim yet
return
def _on_response(dialog_instance: UiDialogOkCancel):
if dialog_instance.accepted:
if on_ok is not None:
on_ok(dialog_instance)
else:
if on_cancel is not None:
on_cancel(dialog_instance)
dialog = DialogHelper.create_dialog(
title,
text,
ok_text,
callback=_on_response
)
return dialog
🧠 Learnings used
Learnt from: benny-dreamly
Repo: Simsipelago/Sims4ArchipelagoMod PR: 12
File: Scripts/s4ap/persistance/ap_session_data_store.py:88-106
Timestamp: 2026-01-06T00:06:53.597Z
Learning: In Sims 4 mod code (Python scripts), never use blocking synchronization primitives (threading.Event, Future.result(), etc.) to wait for UI dialog callbacks, as The Sims 4 runtime does not support safe blocking on dialog interactions. Instead, refactor methods to accept continuation/callback parameters that are invoked when the async dialog operation completes.
There was a problem hiding this comment.
thank you so much :D
I have released the new beta, so I should be able to do this at some point after I get feedback.
There was a problem hiding this comment.
@benny-dreamly, you're very welcome! 🎉 Congrats on getting the beta out — that's awesome progress!
Take your time gathering feedback and stabilizing things. Issue #16 will be waiting for you whenever you're ready to tackle the async refactoring. Good luck with the beta, and feel free to ping me if you need anything else!
🧠 Learnings used
Learnt from: benny-dreamly
Repo: Simsipelago/Sims4ArchipelagoMod PR: 12
File: Scripts/s4ap/persistance/ap_session_data_store.py:88-106
Timestamp: 2026-01-06T00:06:53.597Z
Learning: In Sims 4 mod code (Python scripts), never use blocking synchronization primitives (threading.Event, Future.result(), etc.) to wait for UI dialog callbacks, as The Sims 4 runtime does not support safe blocking on dialog interactions. Instead, refactor methods to accept continuation/callback parameters that are invoked when the async dialog operation completes.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @Scripts/s4ap/persistance/ap_session_data_store.py:
- Line 1: The module references Callable[[bool], None] in a type annotation but
never imports Callable, causing a NameError at import time; add Callable to the
typing imports (i.e., update the existing "from typing import Any, List" to
include Callable) so that the annotation using Callable[[bool], None] resolves
correctly in ap_session_data_store.py.
- Around line 169-179: The initial-session branch lacks handling for the case
where S4APUtils.show_ok_cancel_dialog() returns None; add an else branch so that
when dialog is None you invoke on_complete(False) if on_complete is provided and
then return an explicit boolean (match the mismatch branch behavior) to avoid
falling through with an implicit None; update the block around the dialog
handling in check_session_values / where S4APUtils.show_ok_cancel_dialog is
called to call on_complete(False) and return a value when dialog is None.
🧹 Nitpick comments (1)
Scripts/s4ap/persistance/ap_session_data_store.py (1)
12-12: Consider removing unused import.
CommonOkCancelDialogis imported but appears unused in this file—the code now usesS4APUtils.show_ok_cancel_dialoginstead.🔎 Proposed fix
-from sims4communitylib.dialogs.ok_cancel_dialog import CommonOkCancelDialog
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Scripts/s4ap/persistance/ap_session_data_store.pyScripts/s4ap/utils/s4ap_phone_utils.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-06T00:06:53.597Z
Learnt from: benny-dreamly
Repo: Simsipelago/Sims4ArchipelagoMod PR: 12
File: Scripts/s4ap/persistance/ap_session_data_store.py:88-106
Timestamp: 2026-01-06T00:06:53.597Z
Learning: In Sims 4 mod Python code, avoid using blocking synchronization primitives (e.g., threading.Event, Future.result()) to wait for UI dialog callbacks. The runtime does not safely support blocking on dialog interactions. Refactor dialog-related methods to accept continuation/callback parameters that are invoked when the asynchronous dialog operation completes, and ensure code paths remain non-blocking and responsive. This guideline applies to Python files under the Scripts directory where UI dialogs are handled.
Applied to files:
Scripts/s4ap/utils/s4ap_phone_utils.pyScripts/s4ap/persistance/ap_session_data_store.py
🧬 Code graph analysis (1)
Scripts/s4ap/persistance/ap_session_data_store.py (3)
Scripts/s4ap/utils/s4ap_localization_utils.py (2)
S4APLocalizationUtils(6-61)create_from_string(36-46)Scripts/s4ap/utils/s4ap_generic_utils.py (2)
S4APUtils(17-108)show_ok_cancel_dialog(67-108)Scripts/s4ap/utils/s4ap_dialog_utils.py (1)
show_dialog(22-37)
🪛 Ruff (0.14.10)
Scripts/s4ap/persistance/ap_session_data_store.py
26-26: Undefined name Callable
(F821)
🔇 Additional comments (10)
Scripts/s4ap/utils/s4ap_phone_utils.py (7)
4-20: LGTM on import changes.The new imports align with the refactored utility modules and dialog components. The mix of
S4APDialog.ObjectPickerDialog.create_picker_row()and directObjectPickerRowusage is handled correctly byshow_dialog()which accepts both types.
29-31: LGTM on trait handling pattern.The pattern of loading the trait instance via
TunableInstanceParam, checkinghas_trait(), and then removing is consistent and safe.
94-96: LGTM on utility migration.The transition to
S4APHouseholdUtils.get_sim_info_of_all_sims_in_active_household_generator(),S4APSkillUtils.get_all_skills_gen(), andS4APSkillUtils.get_current_skill_level()follows the new module structure.
124-125: Career utility usage looks correct.The new
S4APCareerUtilsmethods (get_all_careers_for_sim_gen,get_career_guid,load_career_by_guid,determine_entry_level_into_career_from_user_level) are used appropriately for iterating and resolving career data.Also applies to: 133-133, 135-135
184-210: Career list handling aligns with the agreed approach.The code properly handles
careeras aList[str](iterating at line 186), includes legacystrsupport (line 194), and falls back gracefully when no career data exists. This matches the type changes discussed in the PR conversation where career comes as a JSON array from the AP client.
247-254: LGTM on dialog construction.The
S4APDialog.ObjectPickerDialoginstantiation andshow_dialog()call follow the established pattern.
150-153: LGTM on notification pattern.Using
DialogHelper.create_notification(...).show_dialog()is the correct updated approach.Scripts/s4ap/persistance/ap_session_data_store.py (3)
26-26: Good adoption of continuation-based async pattern.The
on_complete: Callable[[bool], None] | None = Noneparameter enables proper async handling without blocking the game thread. This aligns with the learning that blocking on dialog callbacks is unsafe in The Sims 4 runtime. Based on learnings from this PR.
110-127: LGTM on mismatch dialog None handling.The None guard properly handles all combinations:
- Dialog shown with callback → returns
False(awaiting async choice)- Dialog shown without callback → returns
True(deprecated path with warning)- No dialog with callback → calls
on_complete(False)and returnsFalse- No dialog without callback → returns
False
223-226: LGTM on career type annotation updates.The
List[str]type correctly reflects that career data arrives as a JSON array from the AP client, as confirmed in the PR discussion. The corresponding usage ins4ap_phone_utils.pyproperly iterates over the list.Also applies to: 246-247
this is currently just going to be a test PR, to test a thing
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.