Skip to content

Move default_index to DeckBuilder#3126

Closed
salix5 wants to merge 5 commits into
Fluorohydride:masterfrom
salix5:patch-index
Closed

Move default_index to DeckBuilder#3126
salix5 wants to merge 5 commits into
Fluorohydride:masterfrom
salix5:patch-index

Conversation

@salix5

@salix5 salix5 commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

default_lflist in system.conf
It is the index of _lfList and lflist combobox, and it should be validated before passing to combobox.

default_index
The validated value.
Now it is managed by DeckBuilder.

Comment thread gframe/deck_con.h Outdated
@salix5 salix5 requested a review from mercury233 June 29, 2026 09:56

@mercury233 mercury233 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't think it is a good idea to add coupling

@salix5

salix5 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

don't think it is a good idea to add coupling

談耦合之前還是應該先談正確性
目前版本直接拿用戶輸入的default_lflist傳給ComboBox
這個做法明顯有問題
irrlicht的文件有提到index必須是正常index或-1
傳入錯誤的index沒有立刻崩潰只是因為底層的實作有檢查
但依賴實作的方法既不可靠 也無必要

ygopro應該要保存驗證後的default_lflist_index
由於default_lflist_index主要是由DeckBuilder使用
保存在DeckBuilder是合理的選項

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 moves the “default LFList index” (from system.conf’s default_lflist) into DeckBuilder, ensuring the value used to select the LFList combobox is validated before being applied.

Changes:

  • Validate gameConf.default_lflist against deckManager._lfList and store the validated value in deckBuilder.default_lflist_index.
  • Switch LFList combobox selection (system/settings + host dialog) to use deckBuilder.default_lflist_index.
  • Persist the selected LFList index via DeckBuilder and use it to initialize DeckBuilder::filterList.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
gframe/game.cpp Validates config LFList index, uses DeckBuilder-managed index for UI selection, and saves it back to config.
gframe/event_handler.cpp Updates LFList toggle/selection logic to read/write DeckBuilder-managed LFList index.
gframe/deck_con.h Adds default_lflist_index state to DeckBuilder.
gframe/deck_con.cpp Initializes filterList from the DeckBuilder-managed LFList index.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gframe/game.cpp
Comment thread gframe/game.cpp
Comment thread gframe/game.cpp Outdated
Comment thread gframe/event_handler.cpp
Comment thread gframe/event_handler.cpp Outdated
@salix5 salix5 requested a review from mercury233 July 7, 2026 07:57

@mercury233 mercury233 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, gameConf.default_lflist should be removed as part of this change.

But, I don't think we should proceed with this change, since I don't think deckBuilder.default_lflist_index should be added in the first place.

Comment thread gframe/event_handler.cpp
mainGame->cbHostLFlist->setSelected(mainGame->gameConf.default_lflist);
mainGame->deckBuilder.filterList = &deckManager._lfList[mainGame->gameConf.default_lflist];
int sel = mainGame->cbLFlist->getSelected();
if (sel != -1) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently not necessary. One empty lflist is always be pushed to deckManager._lfList, also we never clear cbLFlist, nor setSelected to -1.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing -1 to index is not acceptable.
I don't think ygopro should fight against every combobox like:
Does this combobox return -1?
Surely not.
Oh wait...

I suppose that checking selected value of any combobox is a good practice.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice, only the deck list combo box can be empty. The other combo boxes should never be empty in theory, so it's fine to use them directly.

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.

3 participants