feat(inventory-list): add a setting for the spacing between columns#175
feat(inventory-list): add a setting for the spacing between columns#175zndxcvbn wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a configurable column-margin setting and wires it into the ListLayout component so list column x-positions (and label positions) are offset by the configured margin; also exposes the new ActionScript file to two SWF build targets and defines a default of 0 in config. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
zndxcvbn
changed the title
zndxcvbn
changed the title
zndxcvbn
force-pushed
the
feat-column-margin
branch
from
e49fa89 to
76b03c0
Compare
zndxcvbn
force-pushed
the
feat-column-margin
branch
from
76b03c0 to
84955d8
Compare
zndxcvbn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
source/actionscript/Common/skyui/components/list/ListLayout.as (3)
173-174: Verifydefaults.columnMarginis always parsed as a Number.
config.txtprovides0(numeric), so this works today, but if a user mis-edits the config (e.g. quotes the value),_columnMargincould become a string and the laterthis.columnMargin * multiplierwould yieldNaN, propagating intolabelX/x. A defensiveNumber(...)coercion (or fallback to0) would harden the construction path.🛡️ Proposed defensive coercion
- if (this._columnMargin == undefined) - this._columnMargin = this._defaultsData.columnMargin; + if (this._columnMargin == undefined) { + var m = Number(this._defaultsData.columnMargin); + this._columnMargin = isNaN(m) ? 0 : m; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/actionscript/Common/skyui/components/list/ListLayout.as` around lines 173 - 174, Ensure _defaultsData.columnMargin is coerced to a numeric value when initializing _columnMargin in ListLayout.as: if this._columnMargin is undefined, assign this._columnMargin = Number(this._defaultsData.columnMargin) || 0 so that subsequent uses like columnMargin * multiplier and calculations for labelX/x never produce NaN; update the initialization that references _defaultsData.columnMargin (and any other places reading defaults.columnMargin) to use Number(...) with a fallback.
478-478: Minor:visibleColumnCountis reused later — fine, but consider naming.Just a readability note —
visibleColumnCountequals_columnLayoutData.lengthand is only consumed inside the loop below. No change requested; flagging only because the variable is introduced here without other use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/actionscript/Common/skyui/components/list/ListLayout.as` at line 478, The local var visibleColumnCount duplicates _columnLayoutData.length but is only used inside the loop; improve readability by either renaming visibleColumnCount to something clearer (e.g., columnLayoutCount) or by removing the temporary and using this._columnLayoutData.length directly where referenced; update references in the loop that currently use visibleColumnCount to match the chosen approach (affecting the variable visibleColumnCount and its use with this._columnLayoutData).
143-146: Setter does not trigger a relayout.If the intent (per
#153) is to drive this from the MCM at runtime, callingset columnMargin(...)alone won't visually update existing lists —updateLayout()is only invoked fromchangeFilterFlag,selectColumn, andrestoreColumnState. Consider invokingupdateLayout()(and dispatchinglayoutChange) inside the setter when the value actually changes, or document that callers must trigger a refresh.♻️ Proposed change
public function set columnMargin(a_margin: Number) { - this._columnMargin = a_margin; + if (this._columnMargin == a_margin) + return; + this._columnMargin = a_margin; + this.updateLayout(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/actionscript/Common/skyui/components/list/ListLayout.as` around lines 143 - 146, The setter for columnMargin in ListLayout.as updates _columnMargin but does not trigger a visual relayout; modify the public function set columnMargin(a_margin: Number) to compare a_margin with this._columnMargin and if it changed, assign the new value, call updateLayout() and dispatch the layout change event (same event used elsewhere, e.g. "layoutChange" or the class' existing layout-dispatch mechanism) so existing lists refresh immediately; reference the _columnMargin field, the columnMargin setter, and the updateLayout()/layoutChange dispatch pattern used by changeFilterFlag/selectColumn/restoreColumnState.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/actionscript/Common/skyui/components/list/ListLayout.as`:
- Line 484: The new columnMargin setting (defaults.columnMargin and the
ListLayout property) isn't exposed in the MCM UI; add a corresponding control in
configpanel.xml (e.g., slider or numeric input labeled "Column Margin") bound to
defaults.columnMargin, and update ModConfigPanel to read and write that field
when loading/saving config so the MCM reflects and persists the value; ensure
the control's id/key matches the config key used by ModConfigPanel so changes
update the defaults.columnMargin used by ListLayout.
---
Nitpick comments:
In `@source/actionscript/Common/skyui/components/list/ListLayout.as`:
- Around line 173-174: Ensure _defaultsData.columnMargin is coerced to a numeric
value when initializing _columnMargin in ListLayout.as: if this._columnMargin is
undefined, assign this._columnMargin = Number(this._defaultsData.columnMargin)
|| 0 so that subsequent uses like columnMargin * multiplier and calculations for
labelX/x never produce NaN; update the initialization that references
_defaultsData.columnMargin (and any other places reading defaults.columnMargin)
to use Number(...) with a fallback.
- Line 478: The local var visibleColumnCount duplicates _columnLayoutData.length
but is only used inside the loop; improve readability by either renaming
visibleColumnCount to something clearer (e.g., columnLayoutCount) or by removing
the temporary and using this._columnLayoutData.length directly where referenced;
update references in the loop that currently use visibleColumnCount to match the
chosen approach (affecting the variable visibleColumnCount and its use with
this._columnLayoutData).
- Around line 143-146: The setter for columnMargin in ListLayout.as updates
_columnMargin but does not trigger a visual relayout; modify the public function
set columnMargin(a_margin: Number) to compare a_margin with this._columnMargin
and if it changed, assign the new value, call updateLayout() and dispatch the
layout change event (same event used elsewhere, e.g. "layoutChange" or the
class' existing layout-dispatch mechanism) so existing lists refresh
immediately; reference the _columnMargin field, the columnMargin setter, and the
updateLayout()/layoutChange dispatch pattern used by
changeFilterFlag/selectColumn/restoreColumnState.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2615ad7b-a599-4714-870d-89bfd45b7835
📒 Files selected for processing (3)
data/interface/skyui/config.txtsource/actionscript/Common/skyui/components/list/ListLayout.assource/swfsources.cmake
| c = 0; | ||
| for (var i = 0; i < this._columnList.length; i++) { | ||
| var col = _columnList[i]; | ||
| var col = this._columnList[i]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any MCM wiring that would set columnMargin at runtime.
rg -nP -C3 '\bcolumnMargin\b'
# Look for new MCM entries referencing column spacing.
rg -nP -C3 '(column.?margin|column.?spacing|COLUMN_MARGIN)' --iglob '*.xml' --iglob '*.txt' --iglob '*.as'Repository: doodlum/SkyUI-Community
Length of output: 3144
🏁 Script executed:
# Find configpanel.xml file
fd -type f 'configpanel.xml' --ignore-caseRepository: doodlum/SkyUI-Community
Length of output: 238
🏁 Script executed:
# Check if there are any MCM-related configuration files that might reference columns or spacing
fd -type f -e xml | head -20Repository: doodlum/SkyUI-Community
Length of output: 238
🏁 Script executed:
# Search for any modconfigmenu or MCM references in the codebase
rg -i 'modconfigmenu|modconfig' --iglob '*.xml' --iglob '*.as' -lRepository: doodlum/SkyUI-Community
Length of output: 49
🏁 Script executed:
# Find configpanel.xml file
fd 'configpanel' -e xmlRepository: doodlum/SkyUI-Community
Length of output: 100
🏁 Script executed:
# List all XML files to understand structure
fd -e xml | head -30Repository: doodlum/SkyUI-Community
Length of output: 954
🏁 Script executed:
# Search for ModConfigPanel or modconfigmenu references
rg -i 'modconfigpanel|modconfig' -lRepository: doodlum/SkyUI-Community
Length of output: 128
🏁 Script executed:
# Read the configpanel.xml file
cat source/swf/skyui/configpanel.xmlRepository: doodlum/SkyUI-Community
Length of output: 50381
🏁 Script executed:
# Search configpanel.xml specifically for spacing or margin related entries
rg -i '(spacing|margin|column)' source/swf/skyui/configpanel.xmlRepository: doodlum/SkyUI-Community
Length of output: 18892
🏁 Script executed:
# Look for source XML files that might be used to configure MCM options
find source -type f -name "*.xml" | grep -i configRepository: doodlum/SkyUI-Community
Length of output: 100
🏁 Script executed:
# Search in data/interface for configuration XML files
find data -type f -name "*.xml" | head -20Repository: doodlum/SkyUI-Community
Length of output: 49
🏁 Script executed:
# Look for .swf source files or MXML that might define configuration
find source -type f \( -name "*.mxml" -o -name "*Config*" \)Repository: doodlum/SkyUI-Community
Length of output: 270
🏁 Script executed:
# Read the ConfigPanel.as file to see its structure
wc -l source/actionscript/ModConfigPanel/ConfigPanel.asRepository: doodlum/SkyUI-Community
Length of output: 121
🏁 Script executed:
# Search for column-related configuration in ConfigPanel.as
rg -i 'column|margin|spacing' source/actionscript/ModConfigPanel/ConfigPanel.as -nRepository: doodlum/SkyUI-Community
Length of output: 169
🏁 Script executed:
# Also check what options are currently defined in ConfigPanel.as
rg -i 'case|switch|_config\[' source/actionscript/ModConfigPanel/ConfigPanel.as -n | head -40Repository: doodlum/SkyUI-Community
Length of output: 1619
🏁 Script executed:
# Check if there are any additions to ConfigPanel.as related to columnMargin
rg 'columnMargin' source/actionscript/ModConfigPanel/ConfigPanel.asRepository: doodlum/SkyUI-Community
Length of output: 49
🏁 Script executed:
# Look at the git diff to see what was changed in this PR
git diff --stat HEAD~1 HEAD 2>/dev/null | head -20Repository: doodlum/SkyUI-Community
Length of output: 49
🏁 Script executed:
# Search for any reference to issue 153 or column spacing in commit messages or docs
rg -i '#153|column.*spacing|spacing.*column' . --type md --type txt 2>/dev/null | head -10Repository: doodlum/SkyUI-Community
Length of output: 496
MCM configuration missing for column margin setting. This PR exposes columnMargin via defaults.columnMargin in config.txt and a settable property on ListLayout, but doesn't wire it into the MCM UI in ModConfigPanel/configpanel.xml. Users can only customize it by editing the config file, not through the mod configuration menu.
Is the MCM hookup intentionally a follow-up PR? If so, please clarify this in the PR description so reviewers and users understand that this PR doesn't provide end-to-end resolution of #153.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/actionscript/Common/skyui/components/list/ListLayout.as` at line 484,
The new columnMargin setting (defaults.columnMargin and the ListLayout property)
isn't exposed in the MCM UI; add a corresponding control in configpanel.xml
(e.g., slider or numeric input labeled "Column Margin") bound to
defaults.columnMargin, and update ModConfigPanel to read and write that field
when loading/saving config so the MCM reflects and persists the value; ensure
the control's id/key matches the config key used by ModConfigPanel so changes
update the defaults.columnMargin used by ListLayout.
1 participant
Resolve #153.
Summary by CodeRabbit