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

Show Replace history for Editor find widget #50583

Closed
sandy081 opened this issue May 28, 2018 · 10 comments
Closed

Show Replace history for Editor find widget #50583

sandy081 opened this issue May 28, 2018 · 10 comments
Assignees
Labels
debt Code quality issues editor-find Editor find operations feature-request Request for new features or functionality on-testplan
Milestone

Comments

@sandy081
Copy link
Member

Show Replace history for Editor find widget

@sandy081 sandy081 self-assigned this May 28, 2018
@sandy081 sandy081 added editor-find Editor find operations feature-request Request for new features or functionality labels May 28, 2018
@sandy081 sandy081 added this to the June 2018 milestone May 28, 2018
@sandy081
Copy link
Member Author

Follow up issue from #18233

@sandy081 sandy081 added the debt Code quality issues label May 28, 2018
sandy081 added a commit that referenced this issue Jun 7, 2018
sandy081 added a commit that referenced this issue Jun 7, 2018
sandy081 added a commit that referenced this issue Jun 7, 2018
- Implement Context scoped history input widget
- Adopt to context scoped history input widget
@sandy081
Copy link
Member Author

sandy081 commented Jun 7, 2018

@rebornix I saw that you fixed it in #50189. I am sorry I did not inform you that I was also looking into this. Actually, I have introduced history in Search & Find in the beginning, so I thought I will finish it by fixing this :).

Anyway, I implemented in generic way here. History navigation was implemented outside the inputBox. So every time we need this feature I am duplicating it. For example, similar code is there at following places

  • Search input
  • Search replace
  • Search pattern inputs
  • Find input
  • Find replace
  • Problems filter
  • Terminal find

I now have a HistoryInputBox widget which has history navigation. Only thing that has to be done from outside to call APIs add/show. I pushed my changes by overriding your implementation.

Let me also tell you another cool change I am doing here. At present, we have two commands (previous/next) per each input box for navigating history. Since history navigation is encapsulated in the widget, I can now have only two commands for all above input boxes. It is similar to the List widget with commands. So basically, I will remove all commands and replace them with following two commands

input.action.historyNext, input.action.historyPrevious.

This is a breaking change but worth to have.

@sandy081 sandy081 closed this as completed Jun 7, 2018
@rebornix
Copy link
Member

rebornix commented Jun 7, 2018

@sandy081 I remembered I saw this issue/task someday but when I was going through my debts I forgot about it. Your change is the right way to handle the history navigation and it's great that we have this HistoryInputBox. BTW, do you plan to adopt this component for Terminal and Webview Find box ?

@sandy081
Copy link
Member Author

sandy081 commented Jun 7, 2018

Yes, Terminal & Webview find widgets were also adopted.

@sandy081
Copy link
Member Author

sandy081 commented Jun 7, 2018

@rebornix @Tyriar @mjbvz

I see that you are using find history navigation feature in Editor, Terminal & Webview respectively. Currently each component defines its own commands for navigation. As an user if I want to customise, I have to update all these commands which is cumbersome. Hence, I have merged all these commands into two generic commands input.action.historyNext and input.action.historyPrevious. Please read comment for more information. Impact of this change is that the current history navigation commands will be deprecated in favour of above.

For one milestone, I would be showing following deprecating warning message when user triggers current commands and will remove them in the following milestone.

image

Feedback is appreciated. Thanks.

@roblourens
Copy link
Member

I like this change, thanks for doing this. Please just give me a FYI @mention for stuff like this in the future, I thought it was broken for a bit :)

@rebornix
Copy link
Member

rebornix commented Jun 8, 2018

cc @cleidigh .

@Tyriar
Copy link
Member

Tyriar commented Jun 8, 2018

LGTM 👍

@sandy081
Copy link
Member Author

sandy081 commented Jun 8, 2018

@roblourens Oops.. Sorry Rob.. How could I forget you... Not next time 👍

@sandy081
Copy link
Member Author

sandy081 commented Jun 8, 2018

@roblourens @rebornix @Tyriar @mjbvz

FYI: Removed following commands

[

		'search.history.showNextIncludePattern',
		'search.history.showPreviousIncludePattern',
		'search.history.showNextExcludePattern',
		'search.history.showPreviousExcludePattern',
		'search.history.showNext',
		'search.history.showPrevious',
		'search.replaceHistory.showNext',
		'search.replaceHistory.showPrevious',

		'find.history.showPrevious',
		'find.history.showNext',

		'workbench.action.terminal.findWidget.history.showNext',
		'workbench.action.terminal.findWidget.history.showPrevious',

		'editor.action.extensioneditor.showNextFindTerm',
		'editor.action.extensioneditor.showPreviousFindTerm',

		'editor.action.webvieweditor.showNextFindTerm',
		'editor.action.webvieweditor.showPreviousFindTerm'

	]

Warning is shown only if user has customised keybindings for any of these commands. For backward compatibility, new commands also support alt+up/down keybindings.

@sandy081 sandy081 mentioned this issue Jun 25, 2018
2 tasks
@vscodebot vscodebot bot locked and limited conversation to collaborators Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues editor-find Editor find operations feature-request Request for new features or functionality on-testplan
Projects
None yet
Development

No branches or pull requests

4 participants