-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Add settings entry into titlebar context menu #11404
Conversation
void IslandWindow::AddToSystemMenu(const winrt::hstring& itemLabel, winrt::delegate<void()> callback) | ||
{ | ||
} | ||
|
||
void IslandWindow::RemoveFromSystemMenu(const winrt::hstring& itemLabel) | ||
{ | ||
} |
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.
Ah, right, these clearly haven't been implemented yet. Makes sense why I wasn't seeing anything when checking this branch out 😅
I'll leave this PR alone till it's marked "ready for review"
AH This is fun! @leonMSFT may be v interested here :D |
@check-spelling-bot ReportUnrecognized words, please review:
Previously acknowledged words that are now absentcarlos dpg sid SPACEBAR Unregister vcvarsall xIcon yIcon zamoraTo accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands... in a clone of the git@github.com:serd2011/terminal.git repository
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉 🗜️ If you see a bunch of garbageIf it relates to a ... well-formed patternSee if there's a pattern that would match it. If not, try writing one and adding it to a Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines. Note that patterns can't match multiline strings. binary-ish stringPlease add a file path to the File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
|
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.
This is really neat! Thanks for implementing it 😄
@@ -1717,5 +1729,72 @@ void IslandWindow::OpenSystemMenu(const std::optional<int> mouseX, const std::op | |||
} | |||
} | |||
|
|||
void IslandWindow::AddToSystemMenu(const winrt::hstring& itemLabel, winrt::delegate<void()> callback) |
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.
Is it possible to add multiple system menu items with the same itemLabel
? How would we differentiate between them if we want to remove them?
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.
Since user wouldn't be able to differentiate between items with the same label i thought it would make no sense to support it. Are you talking about some kind of a check in add function to prevent this?
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.
Ah that's true 😄, it should be fine as it is imo then
return; | ||
} | ||
|
||
if (LOG_LAST_ERROR_IF(!DeleteMenu(systemMenu, item.wID, MF_BYCOMMAND))) |
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.
Does DeleteMenu
crash if it can't find an item with the given wID
? If it doesn't do we still need to do the loop up above that checks if an item with the item label exists? We save info about the item when we add it right, so maybe we can use the _systemMenuItems
to pinpoint the item we need to remove 🤔
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.
From purely an architectural purity standpoint, I don't love the add/remove pattern with the menu. I'd probably do something like "set menu" with an entire map of Command
s, that we'd send back down to the app to dispatch however they'd get dispatched.
HOWEVER, that's wild overkill for this one case. Plus, we don't even really have a spec for what that feature would look like yet. So I'm not going to make an external contributor come up with it. This is goodness, it works, and it paves the way for more goodness in the future. Thanks!
Hello @zadjii-msft! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
#11404 changed `_OpenSettingsUI` to `OpenSettingsUI` in `TerminalPage`, but there is still one leftover reference to `_OpenSettingsUI`. This commit fixes that.
🎉 Handy links: |
Summary of the Pull Request
Adds ability for app to change system context menu
PR Checklist
Validation Steps Performed