Skip to content

Conversation

@amnindersingh12
Copy link

Add manual "Add Pin" feature (Closes #40)

  • Add modal dialog to add pins by hand: Forms/FrmAddPin.cs
  • Add "Add Pin..." menu item in File menu and wire click handler:
    • Forms/FrmMain.Designer.cs (add addPinToolStripMenuItem)
    • Forms/FrmMain.cs (AddPinToolStripMenuItem_Click to show dialog, update playerObject lists, update counts, avoid duplicates, enable Save)
  • Include new form in project file: SoG_Savegame_Editor.csproj
  • Fix enum name collision by qualifying System.Enum usage in FrmAddPin.cs

Behavior:

  • User can open File → Add Pin..., select a SogPin and target list (PinsSeen, PinsOnShelf, PinsEquipped, PinsLatest).
  • Selected pin is added to the chosen in-memory player list (lazy-init if null), duplicates prevented, counts updated, Save enabled.

Fixes: #40

@tolik518
Copy link
Owner

tolik518 commented Oct 4, 2025

Hey, thanks for the PR!

Your implementation vastly differs from the current structure of the tool, though.
Please add a new tab for pins, like for every other setting instead of using a dedicated Form.

Removed "Add Pin..." menu item from File menu
Removed helper methods: AddPinToolStripMenuItem_Click, ShowAddPinDialog, AddPinToPlayer, PostAddPinUpdate
Updated .csproj to exclude the deleted file
@amnindersingh12
Copy link
Author

@tolik518 Made some chages please have a look at it !!

@tolik518
Copy link
Owner

tolik518 commented Oct 6, 2025

@amnindersingh12 Looks good to me on the first glance :)
The only thing that's now missing is the following:

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!

@tolik518
Copy link
Owner

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
@tolik518 tolik518 requested a review from Copilot November 7, 2025 12:30
Copy link

Copilot AI left a 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();
Copy link

Copilot AI Nov 7, 2025

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();

Suggested change
playerObject.PinsSeen?.Clear();

Copilot uses AI. Check for mistakes.
}
playerObject.PinsSeenCount = (ushort)playerObject.PinsSeen.Count;

playerObject.PinsOnShelf?.Clear();
Copy link

Copilot AI Nov 7, 2025

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();

Suggested change
playerObject.PinsOnShelf?.Clear();

Copilot uses AI. Check for mistakes.
}
playerObject.PinsOnShelfCount = (byte)playerObject.PinsOnShelf.Count;

playerObject.PinsEquipped?.Clear();
Copy link

Copilot AI Nov 7, 2025

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();

Suggested change
playerObject.PinsEquipped?.Clear();

Copilot uses AI. Check for mistakes.
}
playerObject.PinsEquippedCount = (byte)playerObject.PinsEquipped.Count;

playerObject.PinsLatest?.Clear();
Copy link

Copilot AI Nov 7, 2025

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();

Suggested change
playerObject.PinsLatest?.Clear();

Copilot uses AI. Check for mistakes.
Comment on lines +1577 to +1587
int itemIndex = -1;

for (int i = 0; i < cblstItemsSeen.Items.Count; i++)
{
if (cblstItemsSeen.Items[i].ToString() == itemName)
{
itemIndex = i;
break;
}
}

Copy link

Copilot AI Nov 7, 2025

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add feature to add pins

2 participants