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

New extension: Indentation based text objects #261

Merged
merged 12 commits into from May 24, 2021

Conversation

sharat87
Copy link
Contributor

Hello team! Thank you for the amazing work on IdeaVim.

Indentation based text objects are something I've been missing quite a bit in IdeaVim and, learning that that extension is not already available, I made an implementation of this idea.

Original Vim extension of this idea: https://github.com/michaeljsmith/vim-indent-object.

I'm very new to IdeaVim's code base and I did just enough to get this feature working (since I needed it). In that context, majority of the code is copy-paste from the textobj-entire plugin. If there's anything you'd like to be changed to make it ready for merging, please let me know.

Also, I'm trying to add a test class to test this plugin, but the test fails for some weird reason I'm not able to comprehend yet. I could use some help there (if you guys are okay to merge this in now and do the test separately, I'm fine with that as well). This is probably due to my lack of knowledge in Kotlin though.

Again, thank you very much for your work and your time. I hope this extension will be available as part of IdeaVim in the future.

@AlexPl292 AlexPl292 self-requested a review December 25, 2020 06:47
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, glad to hear that you like the project :D
Thank you for your interest in IdeaVim. I even think that support for this plugin was already requested on some of YouTrack tickets. I've checked your code and the overall approach looks good to me. However, I've left some comments, could you please check them?
Regarding the falling test, I would actually prefer they'll be merged along with this PR. Could you maybe send them as well and I'll check why they don't work?

TextRange getRange(@NotNull Editor editor, @NotNull Caret caret, @NotNull DataContext context,
int count, int rawCount, @Nullable Argument argument) {
final int caretLineNum = caret.getCaretModel().getVisualPosition().getLine();
String content = editor.getDocument().getText();
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to operate on CharSequence instead of the string? IJ uses a custom implementation of the CharSequence to store the code and converting it to the String may be a heavy operation.

int count, int rawCount, @Nullable Argument argument) {
final int caretLineNum = caret.getCaretModel().getVisualPosition().getLine();
String content = editor.getDocument().getText();
String[] lines = content.split("\n");
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I know that vim stores the text as a list of lines, so getting this "lines array" is a zero-weight operation for vim, but this might be heavy for IJ for long files :(
What do you think, would it be possible not to analyze the whole text, but kind of "go up from the caret until the indent is the same, then go down from the caret"?

final int caretLineNum = caret.getCaretModel().getVisualPosition().getLine();
String content = editor.getDocument().getText();
String[] lines = content.split("\n");
String caretLine = lines[caretLineNum];
Copy link
Member

Choose a reason for hiding this comment

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

caretLineNum is a line taken from the visual position, but here you take the index for all lines. This may work incorrect if you'll have folded lines above your current line. I think there should be a logicalPosition for lines indexing (if you'll use this approach, see the comments above).

I think this post may explain the difference between the visual and logical positions in IJ: https://jetbrains.org/intellij/sdk/docs/tutorials/editor_basics/coordinates_system.html#caret-logical-position
In short, a logical position is a position in the text while a visual position is a position that is rendered for the user. If you have a 10 lines file and 3-6 lines are folded, this file has 10 logical lines, but 7 visual lines.

@sharat87
Copy link
Contributor Author

Thank you for the quick response @AlexPl292. This was an initial MVP implementation that I did in an evening. Now that I've confirmed your interest in this, I will invest more time in improving the implementation and adding tests. I'll get back to you folks after those changes.

@AlexPl292
Copy link
Member

@sharat87
Copy link
Contributor Author

Hey @blaquezzz, thank you for approving this PR. However, I'd request you to hold up on merging this. I'm working on an implementation that works on characters as opposed to lines currently. Give me a couple of weeks more I should be able to spend some time again. (Life and holidays happened 😄)

@sharat87
Copy link
Contributor Author

Hey @AlexPl292, sorry for the long delay on this. Had to prioritize other things. So, I've made an initial implementation that just works for now. I intend to refactor and make the code readable, but only after I have tests to ensure I don't break it during refactoring. But with the test class I've added (copy pasted from the textobj-entire plugin), the tests don't run. They fail in the setup phase when I enable the plugin. Can you advise what I'm missing to run the tests please?

Here's the error I'm seeing:

Screenshot 2021-03-21 at 11 17 09

Stack trace:

java.lang.AssertionError
	at org.jetbrains.plugins.ideavim.JavaVimTestCase.enableExtensions(JavaVimTestCase.java:76)
	at org.jetbrains.plugins.ideavim.extension.textobjindent.VimIndentObjectTest.setUp(VimIndentObjectTest.kt:31)
	at com.intellij.testFramework.UsefulTestCase.invokeSetUp(UsefulTestCase.java:471)
	at com.intellij.testFramework.UsefulTestCase.defaultRunBare(UsefulTestCase.java:463)
	at com.intellij.testFramework.UsefulTestCase.lambda$runBare$11(UsefulTestCase.java:525)
	at com.intellij.testFramework.EdtTestUtil.lambda$runInEdtAndWait$1(EdtTestUtil.java:40)
	at java.desktop/java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:303)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:770)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:721)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:715)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:740)
	at com.intellij.ide.IdeEventQueue.dispatchEvent(IdeEventQueue.java:419)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

Thanks!

@sharat87 sharat87 requested a review from AlexPl292 April 7, 2021 09:20
@AlexPl292
Copy link
Member

Hi! I apologize for the delay, because of some reason I skipped the notifications. The problem with tests is bacause you have a different name in VimExternsions.xml file.

@sharat87
Copy link
Contributor Author

sharat87 commented May 9, 2021

@AlexPl292 and others, I've added a few tests for scenarios I could think of. Please suggest/add more cases to this so we can get this to be even more robust.

Thanks!

# Conflicts:
#	resources/META-INF/includes/VimExtensions.xml
@AlexPl292 AlexPl292 merged commit 86296e4 into JetBrains:master May 24, 2021
@AlexPl292
Copy link
Member

Hi! I think this extension is already pretty good. Thank you for your contribution! :D

@sharat87 sharat87 deleted the extension/textobjindent branch May 24, 2021 08:20
@haolly
Copy link

haolly commented Jun 9, 2021

@sharat87 I just tried this in EAP, and it behavior bugs.
When I use command vii in a python function, vim version of this plugin will only select the function body, but this version will select the whole class sadly 😞

@sharat87
Copy link
Contributor Author

sharat87 commented Jun 9, 2021

@haolly woow, that shouldn't be happening. Could you open a new issue for this with minimal piece of code where vii fails as an example please? It'll help me reproduce quickly and get to a fix. Thanks! :)

PS: @AlexPl292, I'm hoping opening an issue for a plugin inside Ideavim against the Ideavim repo is okay 😄

@haolly
Copy link

haolly commented Jun 10, 2021

class TestCls(object):
	VAR_NAME = "SomeThingMagic"

	def __init__(self):
		self.loadedCnt = 0
		self.StartOffset = None

@sharat87 Just use this code as a file to test

@haolly
Copy link

haolly commented Jun 22, 2021

@sharat87 Do you have plan to fix this ?

@sharat87
Copy link
Contributor Author

Hey @haolly, I do intend to fix this, but haven't found the bandwidth recently. I'm hoping this weekend will be different.

@koutselakismanos
Copy link

koutselakismanos commented Jul 22, 2021

Hey @sharat87 , thanks for the contribution, I have been missing this feature! I also found a bug in golang code.
Doing gcii in line 11


import (
	"time"
)

type Game struct {
	ID                 int
	Title              string
	ContainerImageName string
	CpuLimit           int
	MemoryLimit        int
	CreatedAt          time.Time
	UpdatedAt          time.Time
}

type GameVersion struct {
	ID      int
	GameID  int
	Version string
}

results in

//
//import (
//	"time"
//)
//
//type Game struct {
//	ID                 int
//	Title              string
//	ContainerImageName string
//	CpuLimit           int
//	MemoryLimit        int
//	CreatedAt          time.Time
//	UpdatedAt          time.Time
//}
//
//type GameVersion struct {
//	ID      int
//	GameID  int
//	Version string
//}

I tried setting different line seperators but with the same results. It works fine on my react project.
image

@lourenci
Copy link

lourenci commented Jan 5, 2022

The problem here with go lang. 😢

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