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

VIM-1970 | Working solution of plugin vim-highlightedyank #245

Merged
merged 12 commits into from
Jul 27, 2020
Merged

VIM-1970 | Working solution of plugin vim-highlightedyank #245

merged 12 commits into from
Jul 27, 2020

Conversation

KostkaBrukowa
Copy link
Contributor

Hey!
Before i go in I'd like to mention, that I've never contributed to any project before so if i did something wrong things please let me know :)
So, I've found issue (VIM-1970) on plugin that is really simple but makes vim a little bit more pleasurable. This is vim-highlightedyank. I've been using it with Vim and it would be nice to have it in ideavim.
There was no response since that question and I decided to implement it myself.
Based on your advice (or create a draft PR to indicate that you've started working) I've created a draft PR to let you know that I've started working on this.

Right now I believe this draft is overall working version of the plugin. I was thinking if you have any thoughts on this solution.

It's still a draft and the next steps I would like to take are:

  1. make it an extension and move this code to a different package
  2. make this plugin optional (to make it possible to enable it via set vim-highlightedyank
  3. make a duration of the highlight customizable (e.g. let g:highlightedyank_highlight_duration = 1000)
  4. maybe make a highlight color customizable (don't know if it is possible)
  5. of course write some tests to it.

What do you think about it?

@AlexPl292
Copy link
Member

Wow, you're right on time! A few days ago I've looked at this ticket and thought that it would be nice to have this feature in one of the next releases.
I've checked your drafts and they look fine for me. The one important thing is that you embed this functionality right into the yank group, what makes it a core functionality and not the extension of IdeaVim :) (yeah, I see your first punkt, just wanted to make it clear). To convert it in IdeaVim extension, check out how other extensions are implemented (extension package in the project). If you'll do it this way, you'll automatically get the second punkt of your list.
However, you may still need to modify YankGroup because at the moment I don't know how an extension can get a "notification" that a yank was performed. That would be awesome if we could do it in the way as vim does it.

Don't hesitate to reach me out in case of questions. Another great plugin is on the way and this is awesome!

@KostkaBrukowa KostkaBrukowa changed the title VIM-1970 | Draft of working solution to plugin vim-highlightedyank VIM-1970 | Working solution of plugin vim-highlightedyank Jul 19, 2020
@KostkaBrukowa
Copy link
Contributor Author

Hey @AlexPl292 !
I think I've finished the plugin and you can take a look at it.
Sadly there are two things that I didn't know how to do:

  1. the fourth punkt from my list that is: customisable color. The highlight function takes the TextAttributes object and I haven't found the way to just create that kind of object with for example color #c3c3c3 as a parameter, so the highlight color is not customisable. Do you think that the default highlight coloring is OK?
  2. I haven't managed to test the removal of the highlight after some timeout. I've tried to do this with Thread.sleep(...) but is does not work within the test, and test fails (it works well on the Intelij instance).
  fun `test highlighting for a correct default amount of time`() {
    doTest("yiw", code, code, CommandState.Mode.COMMAND, CommandState.SubMode.NONE)

    Thread.sleep(100)
    assertAllHighlightersCount(1)
    Thread.sleep(300) // even value like 10000 does not work
    assertAllHighlightersCount(0) //says that there is still 1 highlighter
  }

  fun `test highlighting for a correct user provided amount of time`() {
    configureByJavaText(code)
    typeText(StringHelper.parseKeys(":let g:highlightedyank_highlight_duration = \"500\"<CR>"))
    typeText(StringHelper.parseKeys("yiw"))

    Thread.sleep(100)
    assertAllHighlightersCount(1)
    Thread.sleep(200)
    assertAllHighlightersCount(1)
    Thread.sleep(200)
    assertAllHighlightersCount(0)
  }

Do you have any thoughts on it?

If you have any comments or ideas please let me know :)

@KostkaBrukowa
Copy link
Contributor Author

Hey!
I've actually managed to modify background color of the highlight from .ideavimrc :)
but again I didn't know how to check highlight color in tests :(

@AlexPl292 AlexPl292 self-assigned this Jul 23, 2020
@AlexPl292 AlexPl292 self-requested a review July 23, 2020 07:46
Copy link
Member

@AlexPl292 AlexPl292 left a comment

Choose a reason for hiding this comment

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

Hey, this is nice work, I like it! I've added some comments, could you please check them?

Regarding tests:
Check out TestHelper.kt file with functions waitAndAssert and assertDoesntChange. I think you can test it using these functions. However, waitAndAssert function runs an assertion in a loop for timeInMillis milliseconds, what might be not really what we need. We can use assertDoesntChange for, let's say, 290ms to check that highlighting isn't removed for this time, and waitAndAssert for 20 ms to check that highlighting is removed in this time range.

You can even create your helper function that would check that something has happened at some moment.
Something like fun assertHappend(afterMillis: Int, precision: Int, assert: () -> Boolean)

And call it with assertHappend(300, 10) { /* Check highlighting is removed */ }

src/com/maddyhome/idea/vim/group/copy/YankGroup.kt Outdated Show resolved Hide resolved
}

private class HighlightHandler {
private val yankHighlighters: MutableSet<Pair<Editor, RangeHighlighter>> = mutableSetOf()
Copy link
Member

@AlexPl292 AlexPl292 Jul 23, 2020

Choose a reason for hiding this comment

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

Are there any cases with more that one highlighter? If not, I suggest just store one current highlighter in the fields:

private var highlighter: RangeHighligher? = null
private var editor: Editor? = null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm creating multiple highlighters when user is in multi cursor mode. I haven't found the way to create highlighter with multiple ranges so I'm creating multiple highlighters
On the other hand I don't need editor for every highlighter, I just need one, so I've removed editor from set

doc/emulated-plugins.md Outdated Show resolved Hide resolved
@AlexPl292
Copy link
Member

@kamolsrisan Hi! I understand that you like the results and want it to be merged, but could you please stop posting this message? This already looks like spam.

@JetBrains JetBrains deleted a comment Jul 24, 2020
@JetBrains JetBrains deleted a comment Jul 24, 2020
@JetBrains JetBrains deleted a comment Jul 24, 2020
@JetBrains JetBrains deleted a comment Jul 24, 2020
@JetBrains JetBrains deleted a comment Jul 24, 2020
@JetBrains JetBrains deleted a comment Jul 24, 2020
@JetBrains JetBrains deleted a comment Jul 24, 2020
@JetBrains JetBrains deleted a comment Jul 24, 2020
@JetBrains JetBrains deleted a comment Jul 24, 2020
@AlexPl292
Copy link
Member

Hey, I've added a commit with some small changes to speed up the process. Actually, I've just added insert listener and performed some small refactorings.

Thank you very much for the contribution. This is great stuff, nice to see it in IdeaVim!

@AlexPl292 AlexPl292 merged commit 3586358 into JetBrains:master Jul 27, 2020
@KostkaBrukowa KostkaBrukowa deleted the VIM-1970 branch July 27, 2020 17:49
@KostkaBrukowa
Copy link
Contributor Author

I'm glad that I could add something to this really great project. Thanks

@citizenmatt
Copy link
Member

I discovered highlightedyank through this PR, and it's now a firm favourite extension in both IdeaVim and my standard Vim config. Great PR! 🎉

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.

3 participants