Mapgen tweaks#1161
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a CheckableComboBox multi-select widget, refactors MapGen options to typed generics, integrates GitHub releases/version switching into the MapGen dialog, and updates manager/process/UI and supporting infra (API authorize flag, Zig build, replay UI, config) for the end-to-end flow. ChangesMap Generator UI and Release Management Enhancement
Sequence DiagramssequenceDiagram
participant User
participant MapGenDialog
participant JsonApiBase
participant GitHub as GitHub API
participant MapGenManager
User->>MapGenDialog: Click fetch versions
MapGenDialog->>MapGenDialog: get_all_versions()
MapGenDialog->>JsonApiBase: get(releases_url, authorize=True)
JsonApiBase->>GitHub: GET /repos/*/releases
GitHub-->>JsonApiBase: releases + Link header
JsonApiBase-->>MapGenDialog: process_releases()
MapGenDialog->>MapGenDialog: populate_versions_combo()
MapGenDialog->>MapGenDialog: save_release_tags()
User->>MapGenDialog: Select version
MapGenDialog->>MapGenManager: switch_version()/generateMap()
MapGenManager->>MapGenManager: set_latest_version()
MapGenManager->>MapGenDialog: emit new_available
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/games/mapgenoptions.py`:
- Around line 124-131: The save and as_cmd_arg methods must guard against an
empty multi-select state: in save(), call self.ui_elem.currentData() into a
local variable (e.g., data) and if data is empty, avoid persisting an empty
string (either skip calling config.Settings.set or remove/clear the setting)
otherwise persist with self.ui_elem.delimiter().join(data); in as_cmd_arg(),
similarly inspect data and return an empty list when data is empty instead of
calling random.choice, otherwise return [f"--{self.name}", random.choice(data)].
Ensure you reference the existing methods save, as_cmd_arg and property
ui_elem.currentData() when making the changes.
In `@src/games/mapgenoptionsdialog.py`:
- Line 510: Fix the user-facing typo in the error dialog call: update the string
passed to message_dialog in mapgenoptionsdialog (the call using
message_dialog(self, "Error", "Process ouptut:",
self.mapgen_manager.process_stdout)) to read "Process output:" so the label
correctly spells "output"; keep the rest of the call and the reference to
mapgen_manager.process_stdout unchanged.
- Around line 280-283: The handler on_version_selection_changed currently
replaces "latest" with self.mapgen_manager.currentVersion, which makes the
comparison always false; instead determine the target version first (e.g., if
text == "latest" use self.mapgen_manager.latestVersion, otherwise use text) and
then call self.buttonSwitchVersion.setEnabled(target_version !=
self.mapgen_manager.currentVersion) so selecting "latest" enables the switch
when it differs from currentVersion; update references in
on_version_selection_changed accordingly.
- Around line 379-381: The call to semantic_version.Version is unguarded in
load_cmd_options and will raise for the default
MapGeneratorManager.currentVersion == "0"; wrap the Version(...) usage in a safe
parse (e.g., try/except ValueError around
Version(self.mapgen_manager.currentVersion)) or use
semantic_version.Version.coerce/parsing with validation and fall back to a safe
minimum like Version("0.0.0"); then compare that safe Version to
Version("1.12.0") and only call self.set_cmd_options({})/return when the parsed
version is less than 1.12.0. Ensure the change targets load_cmd_options and
references self.mapgen_manager.currentVersion so first-run initialization does
not crash.
In `@src/mapGenerator/mapgenManager.py`:
- Around line 48-53: The comparison in set_latest_version uses lexicographic
string comparison; change it to a semantic version comparison by parsing both
the incoming version and current self.latestVersion into proper Version objects
(e.g., using packaging.version.Version or another semantic parser) and compare
those (handle empty or missing self.latestVersion by treating it as Version("0")
or similar). Update set_latest_version to compute new = Version(version) >
Version(self.latestVersion_or_default) before assigning self.latestVersion, keep
Settings.set("mapGenerator/latest_version", version) and emit new_available only
when new is true.
In `@src/mapGenerator/mapgenProcess.py`:
- Around line 86-89: Remove the use of strip() so chunk boundaries aren't lost:
decode the bytes with standard_output.data().decode() (no .strip()), append that
raw decoded chunk to self.stdout, and iterate using data.splitlines() (not
split(os.linesep)) to correctly handle any newline style; keep the existing
empty-line check (or skip empty lines via if not line: continue) so lines are
parsed reliably. Ensure you reference the standard_output.data(), the local
variable data, and the instance field self.stdout when making the change in
mapgenProcess.py.
In `@src/qt/widgets/checkablecombobox.py`:
- Around line 111-113: currentData() can return non-string values so joining
texts may raise TypeError; update the logic around texts = self.currentData() in
the checkable combobox so that each element is converted to a string before
calling self.delimiter().join(...), and still fall back to self.no_choice_text
when texts is empty. Locate the block using currentData(), delimiter(),
no_choice_text and replace the join operand with a stringified sequence (e.g.,
map(str, ...) or list comprehension over texts) so mixed-type selections are
safely joined.
- Around line 129-132: The addItems method currently returns early when datalist
is None, dropping all texts; change it so datalist being omitted still adds
every text by treating datalist as optional—e.g., if datalist is None create a
sequence of None values of the same length as texts or otherwise guard access to
datalist[i] when iterating—and then call the existing per-item add logic for
each (refer to addItems and the per-item add method used inside the loop) so all
texts are added even when no datalist is provided.
- Around line 151-154: The restore-from-text logic in CheckableComboBox
currently only checks items present in choices but never unchecks items removed,
causing stale selections; update the method (where the loop over
range(self.model().rowCount()) and self.model().item(i).setCheckState(...)
appears) to first set each item's check state to Qt.CheckState.Unchecked when
its text is not in choices, and only set Qt.CheckState.Checked when it is in
choices, so every item is explicitly toggled based on membership in choices.
- Around line 80-83: The popup mouse-release handler in CheckableComboBox calls
self.view().indexAt(event.pos()) and then self.model().item(index.row()) without
validating the index, which can be invalid when clicking empty space; update the
mouse event handling (the branch checking event.type() ==
QEvent.Type.MouseButtonRelease) to first verify the QModelIndex is valid (e.g.,
index.isValid()) and/or that index.row() is within range and that item is not
None before accessing item.checkState() or toggling it, and simply ignore or
return when the index is invalid to prevent null dereference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4339b445-9e58-4c6c-95db-b2a4d2ad586e
📒 Files selected for processing (18)
.github/workflows/release.ymlres/client/client.cssres/dialogs/information.uires/games/mapgen.uires/themes/Dark Blue/client/client.csssrc/api/ApiBase.pysrc/config/production.pysrc/games/gamepanelwidget.pysrc/games/hostgamewidget.pysrc/games/mapgenoptions.pysrc/games/mapgenoptionsdialog.pysrc/mapGenerator/mapgenManager.pysrc/mapGenerator/mapgenProcess.pysrc/qt/widgets/checkablecombobox.pysrc/replays/replayitem.pysrc/replays/zigparser/decompressor.zigsrc/replays/zigparser/zigfafreplay.zigsrc/ui/information_dialog.py
it complains about its own generated code with ReleaseSafe optimize option, therefore we use ReleaseFast -- YOLO
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release.yml:
- Line 70: The build uses -O ReleaseFast which disables runtime safety; update
the parser/decompression entrypoints in src/replays/zigparser/zigfafreplay.zig
(e.g., the public main/parse/decompress functions such as main, parseReplay,
decompressData or whatever export functions accept untrusted replay bytes) to
explicitly enable runtime safety by adding `@setRuntimeSafety`(true) at their
start or wrap their logic in a function annotated with `@setRuntimeSafety`(true);
alternatively, add explicit validation+error-handling around all unsafe
operations in those functions so parsing of untrusted replay files does not rely
on ReleaseFast default checks.
In `@src/games/mapgenoptionsdialog.py`:
- Around line 351-356: In load_release_tags, f.readlines() yields lines with
trailing newlines causing Version("1.12.0\n") to raise ValueError; change the
parsing to strip whitespace and skip empties (e.g., iterate over the file lines
and for each do line = line.strip() and only call Version(line) when line is
non-empty) so self.releases = {Version(line.strip()) for line in f if
line.strip()} (keep the existing try/except and use self.release_tags and
self.releases names).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6596ee22-5ff0-4e66-ba78-4727c7f91242
📒 Files selected for processing (15)
.github/workflows/release.ymlres/client/client.cssres/dialogs/information.uires/games/mapgen.uisrc/api/ApiBase.pysrc/config/production.pysrc/games/hostgamewidget.pysrc/games/mapgenoptions.pysrc/games/mapgenoptionsdialog.pysrc/mapGenerator/mapgenManager.pysrc/mapGenerator/mapgenProcess.pysrc/replays/replayitem.pysrc/replays/zigparser/decompressor.zigsrc/replays/zigparser/zigfafreplay.zigsrc/ui/information_dialog.py
✅ Files skipped from review due to trivial changes (2)
- res/dialogs/information.ui
- src/config/production.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI Improvements