search

acejump / AceJump

🅰️ single character search, select, and jump
https://plugins.jetbrains.com/plugin/7086-acejump
GNU General Public License v3.0
1.21k stars 91 forks source link

Remove only the highlighters added by AceJump when the jump session ends #407

Closed huoguangjin closed 2 years ago

huoguangjin commented 2 years ago

Fix #406

huoguangjin commented 2 years ago

I am not quite sure if the remove action should be surrounded with DocumentUtil.executeInBulk.

breandan commented 2 years ago

Hi @huoguangjin, thank you for the PR! Any chance we can add a test for this?

huoguangjin commented 2 years ago

uh.. I don't know much about how to test AceJump with intellij idea test suites. Should I add test case to ExternalUsageTest.kt?

breandan commented 2 years ago

Yes, that seems like a good place to put it. I would just create a mock editor instance, add some highlighters programmatically using the same technique your plugin uses and check to see they are restored after AceJump exits.

huoguangjin commented 2 years ago

Hi, I add a simple test case to check if existed highlighters are removed when the session ends. This test case passes successfully, and fails as expected without the PR commits.

But please note that I dispose the highlighter manually at last because BaseTest.resetEditor checks if markupModel.allHighlighters empty when tears down. It seems that BaseTest.kt takes markupModel.allHighlighters as the highlighters added by AceJump. Maybe It needs changes, String.search and String.assertCorrectNumberOfTags are the same.

breandan commented 2 years ago

Okay, thanks for the clarification. In that case, feel free to modify BaseTest.resetEditor to check that the correct highlighters are preserved. I think the invariant we want to check is markupModel.allHighlighters are the same before and after each AceJump action. Otherwise, LGTM!

huoguangjin commented 2 years ago

Sorry, I can't get your idea. you mean we should mark the number of highlighters before the AceJump session starts, and then check if the number is the same when test case tears down?

breandan commented 2 years ago

Ideally, everything that AceJump modifies (except the caret position) should be identical before and after the session is complete, e.g., you could store oldHighlighters and check assertEquals(oldHighlighters, markupModel.allHighlighters) afterwards, right?

huoguangjin commented 2 years ago

I don't think it is a good idea. Because all test cases call resetEditor from tearDown, but not all test cases store oldHighlighters, it may be initialized as an empty array in setUp. While in this new test case, I store existedHighlighter as oldHighlighters after setUp and before starting an AceJump session, which is a little bit unusual.

How about calling markupModel.allHighlighters with a filter comparing the layer is equal to TextHighlighter.LAYER to find out those highlighters added by AceJump. The only problem is that TextHighlighter.LAYER is private.