-
Notifications
You must be signed in to change notification settings - Fork 7
Add manual "Add Pin" dialog and File→Add Pin menu; update player pin … #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Hey, thanks for the PR! Your implementation vastly differs from the current structure of the tool, though. |
Removed "Add Pin..." menu item from File menu Removed helper methods: AddPinToolStripMenuItem_Click, ShowAddPinDialog, AddPinToPlayer, PostAddPinUpdate Updated .csproj to exclude the deleted file
|
@tolik518 Made some chages please have a look at it !! |
|
@amnindersingh12 Looks good to me on the first glance :) If the user marks an item as equipped, it should be automatically be added as seen (you cant equip something you've never seen). Otherwise it looks good and I'll merge after our adjustment! |
|
Hey @amnindersingh12, can you fix that or should I close the PR? |
- Add a helper method to mark an item as seen - Add event handlers to equipment combo boxes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds functionality for manually adding pins to a player's save file through a new "Add Pin" dialog accessible via File → Add Pin menu, addressing issue #40. The changes also include automated tracking of equipped items as "seen" and proper initialization of pin-related UI controls.
Key Changes:
- Added new FrmAddPin form with menu integration for manually adding pins to player lists
- Implemented automatic marking of items as seen when equipped via combo box event handlers
- Initialized pin checklist data sources and added population/serialization logic for all four pin collections
Files not reviewed (1)
- SoG_Savegame_Editor/Forms/FrmMain.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| playerObject.PinsSeen?.Clear(); |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling Clear() on a potentially null list before reassigning defeats the purpose of the null-conditional operator. Either remove line 820 or change line 821 to only assign when null: playerObject.PinsSeen ??= new List<SogPin>(); followed by playerObject.PinsSeen.Clear();
| playerObject.PinsSeen?.Clear(); |
| } | ||
| playerObject.PinsSeenCount = (ushort)playerObject.PinsSeen.Count; | ||
|
|
||
| playerObject.PinsOnShelf?.Clear(); |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling Clear() on a potentially null list before reassigning defeats the purpose of the null-conditional operator. Either remove line 831 or change line 832 to only assign when null: playerObject.PinsOnShelf ??= new List<SogPin>(); followed by playerObject.PinsOnShelf.Clear();
| playerObject.PinsOnShelf?.Clear(); |
| } | ||
| playerObject.PinsOnShelfCount = (byte)playerObject.PinsOnShelf.Count; | ||
|
|
||
| playerObject.PinsEquipped?.Clear(); |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling Clear() on a potentially null list before reassigning defeats the purpose of the null-conditional operator. Either remove line 842 or change line 843 to only assign when null: playerObject.PinsEquipped ??= new List<SogPin>(); followed by playerObject.PinsEquipped.Clear();
| playerObject.PinsEquipped?.Clear(); |
| } | ||
| playerObject.PinsEquippedCount = (byte)playerObject.PinsEquipped.Count; | ||
|
|
||
| playerObject.PinsLatest?.Clear(); |
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling Clear() on a potentially null list before reassigning defeats the purpose of the null-conditional operator. Either remove line 853 or change line 854 to only assign when null: playerObject.PinsLatest ??= new List<SogPin>(); followed by playerObject.PinsLatest.Clear();
| playerObject.PinsLatest?.Clear(); |
| int itemIndex = -1; | ||
|
|
||
| for (int i = 0; i < cblstItemsSeen.Items.Count; i++) | ||
| { | ||
| if (cblstItemsSeen.Items[i].ToString() == itemName) | ||
| { | ||
| itemIndex = i; | ||
| break; | ||
| } | ||
| } | ||
|
|
Copilot
AI
Nov 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using cblstItemsSeen.Items.IndexOf(itemName) instead of manually iterating through the items collection, which is more efficient and concise.
| int itemIndex = -1; | |
| for (int i = 0; i < cblstItemsSeen.Items.Count; i++) | |
| { | |
| if (cblstItemsSeen.Items[i].ToString() == itemName) | |
| { | |
| itemIndex = i; | |
| break; | |
| } | |
| } | |
| int itemIndex = cblstItemsSeen.Items.IndexOf(itemName); |
Add manual "Add Pin" feature (Closes #40)
Behavior:
Fixes: #40