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

Supporting line wise yanks, including paste and undo. #811

Merged
merged 8 commits into from Nov 8, 2019

Conversation

springcomp
Copy link
Contributor

This is my take at #333.

I thought best to create a ViRegister class to replace the current _clipboard String because I wanted to support pasting "line wise" irrespective of where the cursor is at when pasting occurs. I created an event that gets raised each time the buffer is about to change as part of a paste operation, to hook into undo/redo without too much coupling.

Please, let me know your thoughts and suggestions for improvements.

@springcomp
Copy link
Contributor Author

I updated this pull request via force push using the accepted answer from this question.

@msftclas
Copy link

msftclas commented Oct 19, 2019

CLA assistant check
All CLA requirements met.

Copy link
Member

@lzybkr lzybkr left a comment

Choose a reason for hiding this comment

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

Sorry it took forever for me to take a look
I have some general comments that need to be addressed and I'll try to take a closer look then, or maybe @daxian-dbw will beat me to it.

PSReadLine/StringBuilderLinewiseExtensions.cs Outdated Show resolved Hide resolved
PSReadLine/StringBuilderLinewiseExtensions.cs Outdated Show resolved Hide resolved
PSReadLine/StringBuilderLinewiseExtensions.cs Outdated Show resolved Hide resolved
PSReadLine/StringBuilderLinewiseExtensions.cs Outdated Show resolved Hide resolved
PSReadLine/StringBuilderLinewiseExtensions.cs Outdated Show resolved Hide resolved
PSReadLine/ViRegister.cs Outdated Show resolved Hide resolved
PSReadLine/ViRegister.cs Outdated Show resolved Hide resolved
test/StringBuilderLinewiseExtensionsTests.cs Outdated Show resolved Hide resolved
PSReadLine/ViRegister.cs Outdated Show resolved Hide resolved
PSReadLine/ViRegister.cs Outdated Show resolved Hide resolved
@springcomp
Copy link
Contributor Author

Sorry, I force-pushed again because there was structural changes to address your general comments.
You can now take a closer look.

From now on I will address your feedback with additional commits.

@springcomp
Copy link
Contributor Author

This PR still shows changes requested even though I have carried out the changes.
@lzybkr can you please re-review?

@lzybkr
Copy link
Member

lzybkr commented Oct 28, 2019

It looks like not all of the new tests were run in CI - presumably because test discovery isn't searching Microsoft.PowerShell.PSReadLine2.dll.

I'd like @daxian-dbw to approve including tests like this before merging.

@springcomp
Copy link
Contributor Author

springcomp commented Nov 6, 2019

The latest commit adds discovery and execution of thoses unit-tests.

For this purpose, the main module is compiled with an alternate set of options and to a target assembly named Microsoft.PowerShell.PSReadLine2.UnitTests.dll so that it does not conflict with the release assembly.

@daxian-dbw, are you OK with these changes?

@daxian-dbw
Copy link
Member

Can we move the test code to PSReadLine.Tests (under PSReadLine\test)? We can make Microsoft.PowerShell.PSReadLine2 internal visible to PSReadLine.Tests.

@springcomp
Copy link
Contributor Author

springcomp commented Nov 8, 2019

I thought about doing this but was deterred as per @lzybkr’s comment.

I would be happy to go ahead, but I do not know how to handle InternalsVisibleTo and assembly delay-signing?

For this to work, there would need to be a strong-name snk file in the repository for signing both assemblies. The way I handled this in the past was for the owner of the repository to include an encrypted version of the release strong-name snk file in the repository, and use AppVeyor secure strings to decrypt the file during the build.

Would that work for you @daxian-dbw?

@daxian-dbw
Copy link
Member

I don't think InternalsVisibleTo requires the involved assemblies to be strong-name signed. Quoted the following from the doc of this attribute

If both assemblies are unsigned, the assemblyName argument consists of the name of the friend assembly, specified without a directory path or file name extension.

I think you can just add [assembly: InternalsVisibleTo("PSReadLine.Tests")] to PSReadLine\PSReadLine\AssemblyInfo.cs and it should work.

@springcomp
Copy link
Contributor Author

I can’t believe I just learned about that fact today 😱😱!

@daxian-dbw
Copy link
Member

Thanks @springcomp!
I made a minor change to move the InternalsVisibleTo declaration to AssemblyInfo.cs, where other assembly scope attributes are put. That change breaks the build and it seems that file gets excluded somehow. I will fix it.

@daxian-dbw
Copy link
Member

Build fixed. @springcomp Could you please review my changes? Thanks!

Copy link
Contributor Author

@springcomp springcomp left a comment

Choose a reason for hiding this comment

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

Yes, that is the reason why I created a separate file but this solution is certainly better.

@springcomp
Copy link
Contributor Author

With this PR, I intend to work next on implementing dd (delete line) and, possibly S (substitute line) as natural extensions, so am looking forward to seeing this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants