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

Fixes = and == functionality to match VIM #502

Closed
wants to merge 5 commits into from

Conversation

davidquon
Copy link

This should fix the = and == functionality to match the behavior in VIM. The behaviors that should be fixed are:

  • Cursor is on the first non-whitespace character after command
  • Whitespace characters are removed from lines consisting of only whitespace characters

@davidquon
Copy link
Author

I think I found a use case that wasn't accounted for in this PR. Going to close this PR and try to fix it before submitting. Let me know if you have any feedback though about these changes. Thanks.

@davidquon davidquon closed this Oct 14, 2013
@JugglerShu
Copy link
Contributor

@davidquon
Thank you for your pull request.
If you find something wrong or want to add something before I merge it to the main develop branch
you just can write about it here and keep commiting your modification on your branch.
I think Github automatically tracks it and the pull request is automatically updated to the latest commit of the branch.
(I mean you do not need to close the pull request and recreate it.)

So I reopen this and wait for your fixed commit.

@JugglerShu JugglerShu reopened this Oct 15, 2013
@davidquon
Copy link
Author

Thanks @JugglerShu. Of course right after I opened the PR I found a bug. 😉 I wasn't sure what the process was about keeping a PR open so I decided to close it so you don't merge it.

No problem I can add visual mode + filter tests.


NSString *lineString = [[self xvim_string] substringWithRange:filterRange];
NSRange whiteSpaceRange = [lineString rangeOfString:@"^\\s*" options:NSRegularExpressionSearch];
lineString = [lineString stringByReplacingCharactersInRange:whiteSpaceRange withString:@""];
Copy link
Author

Choose a reason for hiding this comment

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

@JugglerShu - Maybe you can help point me in the correct direction here. The problem is that I need to remove the whitespace from the actual line of the file here and write it back. This isn't happening here as I thought as the line below test = FALSE; should has 8 spaces. If you == on that line in VIM all of the whitespace should be removed which is not happening.

    if( test )
    {
        test = FALSE;

    }

I have the range of the whiteSpaceRange and the filterRange as well as the substring of the filterRange. However what I'm not sure about is the best way to "edit" the line of the actual file to remove the whitespace or if if it's even allowed to edit from the NSTextView+VimOperation. Just wanted to see what your thoughts were about this and how best to "edit" this line to remove all the found whitespace characters.

Choose a reason for hiding this comment

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

Maybe the way forward would be to find the start of the line you are on and the line the end position is on. yank that, filter it, delete the lines using

- (void)xvim_deleteLinesFrom:(NSUInteger)line1 to:(NSUInteger)line2; 

then insert the filtered text back using

- (void)xvim_insertText:(NSString*)str line:(NSUInteger)line column:(NSUInteger)column;

both are methods on the category:

@interface NSTextView (VimOperation)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah that's one of the solutions I was thinking about too as well as just deleting all the text on the line if it's all whitespace however I wasn't sure if there was a more optimized solution. I was hoping for something more like NSString stringByReplacingCharactersInRange:withString: if possible though although I haven't found anything like that yet.

Choose a reason for hiding this comment

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

I doubt that it needs to be super optimized an operation as the files most people work on are on the order of 1000 LOC. this feature should be tested thoroughly with test cases though because it's potentially destructive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, anything is fine as far as it is working.
But usually when you want to edit text in XVim text view, think followings in order.

1. Find if there is a method named with "xvim_" which solves your problem (and use it if found)
2. Use [self insertText:replacementRange:] for any edit(insert,delete,replace) of a text. (This is handled by an undo manager properly)
3. If you find some editing operation is shared by several Vim commands, think about create xvim_ prefixed method in NSTextView+VimOperation category.

I wonder if your code here is handled by an undo manager properly.

Let me know if you have further questions.

Thank you for your great improvements!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the great feedback @weaksauce and @JugglerShu. I'll update taking into consideration the ideas above and make sure I handle adding some more tests like for undo and visual. 👍

@davidquon
Copy link
Author

I still need to manually test this a bit more and write the updated tests but hopefully this fixes the problem. It's probably not optimized so if you have any suggestions please let me know.

@davidquon
Copy link
Author

Good catch @weaksauce. Will investigate and fix. So many hidden use cases I didn't anticipate. 😉 Thanks for the help and feedback.

@davidquon
Copy link
Author

@JugglerShu, @weaksauce - Is there a way to get an array of lines in a visual block? Is that what xvim_selectedRanges was intended for? If so I think there is a bug in it as currently it only returns an array of one line. If there is nothing that returns an array of lines in a visual block let me know and I'll write code to figure this out in this PR. Thanks.

@davidquon
Copy link
Author

Also out of curiosity is there anyplace that documents the intention of methods or objects in XVim? If not I maybe I should open an issue as with multiple developers it be nice to have at least some light documentation of objects and methods so developers can know their intention.

@JugglerShu
Copy link
Contributor

@davidquon
xvim_selectedRanges is the method for getting the ranges of block selection. So if the method returns an array with one object when you are selecting multiple lines in block mode it is a bug.
I have just checked it but it looks working for easy visual block selection. I think it may not work in some cases.

Light documentation is OK but the code for XVim has been changing a lot and heavy documentation does not make much sense now. I have been added some explanation about classes or methods whenever I can in source files. I think the explanation in header file in a few lines are suitable at the moment.

@davidquon
Copy link
Author

@JugglerShu so it seems that xvim_selectedRanges works with XVIM_VISUAL_BLOCK (CTRL-v) but not XVIM_VISUAL_LINE (SHIFT-v). I will create an issue for this.

@davidquon
Copy link
Author

Created issue #512 for the problem with xvim_selectedRanges with XVIM_VISUAL_LINE (SHIFT-v).

@davidquon
Copy link
Author

I ran into a roadblock with this PR with the filter operation over multiple lines in visual mode. While the functionality was close to working the undo was broken as there were multiple operations being done "behind the scenes". I think fixing this may require a different approach in order not to break the visual mode and undo when performing the filter operation.

With the refactor in PR #535 I think these changes have become irrelevant and will need to be re-implemented if necessary on the buffer refactor code. Going to close this PR for now until it's determined what changes are needed (if any) after the buffer refactor.

@davidquon davidquon closed this Dec 2, 2013
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