Skip to content
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

Preserve recent commands for different locales #11336

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

msujew
Copy link
Member

@msujew msujew commented Jun 24, 2022

What it does

Closes #11335

Makes the internal list of recently used commands a simple string array containing the IDs of these commands. This allows us to have them independent from the selected locale.

How to test

  1. Run the Configure Display Language command
  2. Select another locale
  3. Open the command palette, the Configure Display Language command should still be in the list of recently used commands.

Review checklist

Reminder for reviewers

@msujew msujew added commands issues related to application commands localization issues related to localization/internalization/nls labels Jun 24, 2022
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

This worked for me, but with the caveat that the first time I ran it, I wound up with duplicates of several of my recent commands:

image

Note that Color Theme and Configure Display Language each appear twice.

}
}
return commands;
return Object.values(this._commands);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@msujew msujew force-pushed the msujew/preserve-recent-commands branch from 220f3d7 to 8f1d5ba Compare June 27, 2022 13:49
@msujew
Copy link
Member Author

msujew commented Jun 27, 2022

@colin-grant-work I couldn't reproduce the issue, but I'm using an intermediate Set now to avoid duplicates. Does that fix the issue?

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

Is working, and is now more robust against duplicates that have snuck into the state restoration system on application close.

@colin-grant-work
Copy link
Contributor

I think we should really not use Command.equals anywhere. The only source of truth about command identity is the id field, so checking anything else, for example here,

if (limit > 0) {
rCommands.push(...recentCommands.filter(r =>
!this.exemptedCommands.some(c => Command.equals(r, c)) &&
allCommands.some(c => Command.equals(r, c)))
);
if (rCommands.length > limit) {
rCommands = rCommands.slice(0, limit);
}
}
// Build the list of other commands.
const oCommands = allCommands.filter(c => !rCommands.some(r => Command.equals(r, c)));

is just misleading: it can only return false negatives and introduce duplicates.

@msujew msujew merged commit c9b9b43 into master Jun 28, 2022
@msujew msujew deleted the msujew/preserve-recent-commands branch June 28, 2022 14:40
@github-actions github-actions bot added this to the 1.27.0 milestone Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commands issues related to application commands localization issues related to localization/internalization/nls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing locale does not preserve the recently used commands
2 participants