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

Mark only those commands ran using quick menu as recent #7552

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

dsseng
Copy link
Contributor

@dsseng dsseng commented Apr 12, 2020

What it does

Fixes #7527

How to test

See #7527

Review checklist

Reminder for reviewers

@dsseng
Copy link
Contributor Author

dsseng commented Apr 12, 2020

This probably needs another fix...

@dsseng
Copy link
Contributor Author

dsseng commented Apr 12, 2020

Anything is okay, but originally recent commands got stored, now they don't. I need help from somebody to make it work.

@vince-fugnitto
Copy link
Member

@sh7dm you may see the quick poc with minimal changes to achieve the behavior:
https://github.com/eclipse-theia/theia/compare/vf/recent-commands?expand=1

Only commands triggered from the quick-command menu will be registered as recently used, and those triggered from other menus, keybindings, toolbar items will not.

@vince-fugnitto vince-fugnitto added the commands issues related to application commands label Apr 13, 2020
@dsseng
Copy link
Contributor Author

dsseng commented Apr 13, 2020

@vince-fugnitto yes, this's the thing I did in the first commit. The issue was that tests failed, I could fix them but I wasn't sure if it doesn't break anything else. If you're sure it won't, I can restore this solution and make tests pass.

@vince-fugnitto
Copy link
Member

@vince-fugnitto yes, this's the thing I did in the first commit. The issue was that tests failed, I could fix them but I wasn't sure if it doesn't break anything else. If you're sure it won't, I can restore this solution and make tests pass.

I see, you can retry, if there are test failures I can see as what the reason may be :)

@dsseng
Copy link
Contributor Author

dsseng commented Apr 13, 2020

@vince-fugnitto here's the test result, I'll restore to that point and fix failing tests.

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Apr 13, 2020

@vince-fugnitto here's the test result, I'll restore to that point and fix failing tests.

The test cases will need to be updated as well:


When verifying test cases you can run:


npx run build @theia/core
npx run test @theia/core

@vince-fugnitto
Copy link
Member

Please squash your commits into a single commit and ping me when you're ready for another review :)

@dsseng
Copy link
Contributor Author

dsseng commented Apr 13, 2020

@vince-fugnitto should be OK now, you can review if you don't mind.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@sh7dm thank you for your contribution, the changes look good! 👍
We have some more work to do with recent commands, I tested some things on master and subsequently this pull-request and there are still some issues that I'd like resolved.

Signed-off-by: Dmitry Sharshakov <d3dx12.xx@gmail.com>

Fix tests

Signed-off-by: Dmitry Sharshakov <d3dx12.xx@gmail.com>

Remove tests for recent commands

Signed-off-by: Dmitry Sharshakov <d3dx12.xx@gmail.com>

Revert "Fix tests"

This reverts commit 0554e1a.

Signed-off-by: Dmitry Sharshakov <d3dx12.xx@gmail.com>

Test recent command history

Signed-off-by: Dmitry Sharshakov <d3dx12.xx@gmail.com>

Apply changes from review

Signed-off-by: Dmitry Sharshakov <d3dx12.xx@gmail.com>
@vince-fugnitto vince-fugnitto merged commit a62a6b9 into eclipse-theia:master Apr 13, 2020
@dsseng dsseng deleted the fix-7527 branch April 13, 2020 16:02
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recently used commands include ones triggered by mouse
2 participants