Skip to content

ShortcutMapper: fix active shortcut index#16492

Closed
pryrt wants to merge 2 commits intonotepad-plus-plus:masterfrom
pryrt:fixShortcutOffBy1
Closed

ShortcutMapper: fix active shortcut index#16492
pryrt wants to merge 2 commits intonotepad-plus-plus:masterfrom
pryrt:fixShortcutOffBy1

Conversation

@pryrt
Copy link
Copy Markdown
Contributor

@pryrt pryrt commented May 1, 2025

  • there was off-by-1 when building the oemVkUsedIDs vector, which was caused by using the index based on .size() instead of .size()-1 to get the most-recently-added

  • also cleared the vector each launch of the dialog, because the .push_back() was causing the vector to grow without bounds, messing up the indexing on subsequent runs of the dialog, causing it to pick none of the entries

fixes #16491

- there was off-by-1 when building the oemVkUsedIDs vector, which was caused by using the index based on .size() instead of .size()-1 to get the most-recently-added

- also cleared the vector each launch of the dialog, because the .push_back() was causing the vector to grow without bounds, messing up the indexing on subsequent runs of the dialog, causing it to pick _none_ of the entries
@xomx
Copy link
Copy Markdown
Contributor

xomx commented May 1, 2025

Tested - ok.

@pryrt
I read (and agree with) your comment in the Community v8.8 Release thread. Lately, we've been having trouble finding a N++ version that could be triggered for auto-updating, huh?

@pryrt
Copy link
Copy Markdown
Contributor Author

pryrt commented May 1, 2025

@xomx,

Tested - ok.

Thanks.

I read (and agree with) your comment in the Community v8.8 Release thread. Lately, we've been having trouble finding a N++ version that could be triggered for auto-updating, huh?

I was really hoping that v8.8 would be the next auto-update. Sorry for this bug.

@donho donho self-assigned this May 2, 2025
Copy link
Copy Markdown
Member

@donho donho left a comment

Choose a reason for hiding this comment

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

Please follow the suggestion.


if (_keyCombo._key == namedKeyArray[i].id)
iFound = static_cast<int32_t>(oemVkUsedIDs.size());
iFound = static_cast<int32_t>(oemVkUsedIDs.size()-1); // -1 because it's already been added, so the most recent index of oemVkUsedIDs is 1 less than the length
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

iFound = static_cast<int32_t>(oemVkUsedIDs.size() - 1); should be iFound = static_cast<int32_t>(oemVkUsedIDs.size()) - 1; to convert size_t to int32_t before, then do the substraction.


if (_keyCombo._key == namedKeyArray[i].id)
iFound = static_cast<int32_t>(oemVkUsedIDs.size()-1); // -1 because it's already been added, so the most recent index of oemVkUsedIDs is 1 less than the length
iFound = static_cast<int32_t>(oemVkUsedIDs.size())-1; // -1 because it's already been added, so the most recent index of oemVkUsedIDs is 1 less than the length
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's always better to keep the code readability: Write static_cast<int32_t>(oemVkUsedIDs.size()) - 1 instead of static_cast<int32_t>(oemVkUsedIDs.size())-1. But it's OK for this time.

@donho
Copy link
Copy Markdown
Member

donho commented May 2, 2025

The regression was introduced by: 6dfbc1f

@donho donho closed this in 30976ec May 2, 2025
@pryrt pryrt deleted the fixShortcutOffBy1 branch May 12, 2025 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[REGRESSION] Shortcut mapper dialog box is displaying the wrong keyboard character

3 participants