Patch submission #507
closed% command created
100%
Description
I created the % command. This commands jump to the next opening char (like (, [ ...) and find the closing character such as ), ] ...
- I didn't know how to name the function, so I used the command name.
- The function is long, maybe too, it could maybe be separated in some little functions.
- You'll see in the code a comment about vim behavior.
When there are no closing character, vim don't move. I think it should me more logical to move to the first found. For example
some text ( and nothing to close the bracket
if you put the cursor before the bracket, and press %, vim don't move at all, isn't it more logical and useful to jump on the bracket ?
There is a lua script which test all the case I think about, there may be others, but I think it's rather complete.
Files
Updated by Loïc P. over 13 years ago
I can't read your patch file... what is its format?
Could you provide one in clear text ?
Updated by Arnaud Tanguy over 13 years ago
- File percentCommand2.patch percentCommand2.patch added
Apparently, there was a problem when uploading, as the patch should be a 4Kb instead of 2.9.
I recreated it and resend it.
[edit] That's strange, the file is still 2.9Kb here and 4Kb on my computer. I hope this time it will be readable.
I created the patch with 'hg bundle'.
Updated by Loïc P. over 13 years ago
Ok, so this is bundle and not a patch (please add .hg suffix next time;))
This has to be discussed with orzel, but to do patch review, I would prefer having a clear text patch (using hg export or hg diff, or maybe there is a way to tell hg to produce human readable bundles), and if it is accepted, then we may ask for a bundle.
I'll look to your patch as soon as possible.
Thank you!
[edit] it seems that .hg is preferred for bundles extension.
Updated by Arnaud Tanguy over 13 years ago
Ok, I don't know about it. As I saw .patch extension everywhere, I used it.
I'll put .hg extension to bundles, and have a look at 'hg export' (I'm getting started with mercurial).
Updated by Arnaud Tanguy over 13 years ago
- File percentCommand.diff percentCommand.diff added
Here is the diff file. As I said, the function is quite long, so it should be splitted. Just tell me if it is needed, and i'll see how I can split it.
Updated by Loïc P. over 13 years ago
Ok, so I quickly read it. Indeed this is a looong patch :)
Two suggestions :
- use of QString methods to search for matches (I'm especially thinking to indexOf and lastIndexOf methods).
- instead of splitting into different functions, try to factorize the forward and backward search : there so few differences (only incrementing/decrementing, indexOf/lastIndexOf and correspondingMatch/toMatch renaming) that I'm quite sure it may be done in a single loop. It will heavily reduce the size of the patch.
Updated by Arnaud Tanguy over 13 years ago
- File percentCommand.diff percentCommand.diff added
Ok, thanks for the advices, it's now far smaller.
I used indexOf as you suggested, I managed to factorise the two loops in one.
It still pass the test I made before.
Updated by Loïc P. over 13 years ago
Yeah, it's better!
But are you sure it is working?
+ while(c<=line.length() && c>=0)
+ {
+ if(toMatch.indexOf(line[c]) != -1) nOpen++;
+ else if(correspondingMatch.indexOf(line[c]) != -1) nClose++;
+ if (nOpen == nClose) {
+ newCursorPos.setLineColumn(l, c);
+ return newCursorPos;
+ }
+ c += direction;
+ }
Perhaps I miss something, but as you are starting with nbOpen = nbClose = 0, if the first char of line is not a toMatch, nor a correspondingMatch, it will stops, because nbOpen == nbClose (= 0).
I think you'll have to initialize nbOpen (resp. nbClose) to 1 I guess.
+ something strange : you are not using correspondingCh : it will certainly fail on ill samples like : "{ ()) }" (if you press % on '{')
Certainly the test nbOpen == nbClose should be done only when current char is correspondingCh.
Another thing :
+ l += direction; + if(l<0) return cursorBefore; + line = args.view->buffer()->textline(l); + if(direction == 1) c = 0; else c = line.length();
You'll have a problem if l = lineCount().
I guess "if ( l == maxLine ) break;" would be a better test than "if ( l < 0) return .."
[edit] and it must be c = line.length() - 1.
Updated by Arnaud Tanguy over 13 years ago
- File percentCommand_fix.diff percentCommand_fix.diff added
Perhaps I miss something, but as you are starting with nbOpen = nbClose = 0, if the first char of line is not a toMatch, nor a correspondingMatch, it will stops, because nbOpen == nbClose (= 0).
Yeah, but it's expected : when there is neither open char, nor close char under the cursor, the cursor don't move.
+ something strange : you are not using correspondingCh : it will certainly fail on ill samples like : "{ ()) }" (if you press % on '{')
Yeah, you're right, it didn't work correctly. In fact, I was quite tired, and I confused correspondingCh with correpondingMatch, and as a matter of fact, It worked and pass the tests well. It's now corrected. I assume we don't have to reproduce vim bugs, as with this case, vim don't work well : it finds forward, but not backwards...
Certainly the test nbOpen == nbClose should be done only when current char is correspondingCh.
Indeed. It's corrected.
I added the case { ()) } in the lua script.
I'm quite lost in my local repository, as I worked on others commands since, so there is also the gm commands diffs with it, sorry
Updated by Loïc P. over 13 years ago
Why ch and correspondingCh are QString? Should'nt they be QChar? It's a bit strange to do indexOf just for testing char equality. That would make the code clearer.
Correct this and I will certainly merge your patch :)
Updated by Arnaud Tanguy over 13 years ago
- File percentCommand_fix2.diff percentCommand_fix2.diff added
Yes, indeed, it was strange, and I can't even remember why I wrote it like that yesterday.
It's corrected. Sorry for taking your time for such stupid errors, I'll try to do better next time.
Do you want a bundle ?
Updated by Loïc P. over 13 years ago
Arnaud Tanguy wrote:
Do you want a bundle ?
Yes why not. I never used them, but let's try! It will prevent you to have to do another branch checkout.
Updated by Arnaud Tanguy over 13 years ago
Ok, so I'll try to create a clean bundle, including only the needed changes. I'm not used to create bundles too, it's my third one, so I'm a bit lost...
Let's try.
Updated by Arnaud Tanguy over 13 years ago
- File percentCommand.hg percentCommand.hg added
Here is the bundle.
Updated by Loïc P. over 13 years ago
- Status changed from New to Closed
- Assignee set to Loïc P.
- Target version set to 1.0-alpha2
Arnaud Tanguy wrote:
Here is the bundle.
Applied (r4239).
Thanks!
Updated by Arnaud Tanguy over 13 years ago
- File percentCommand_fix.hg percentCommand_fix.hg added
I added the two lines
YCursor YModeCommand::percentCommand(const YMotionArgs &args, CmdState state, MotionStick ms )
{
+ if ( ms != NULL ) *ms = MotionNoStick;
+ *state = CmdOk;
Here is the bundle.
Updated by Loïc P. over 13 years ago
Applied, thanks.
Please open a separate bug report next time ;)
Updated by Thomas Capricelli over 13 years ago
@Arnaud : usually the way to go with mercurial is to have several 'clones' and do different works (here: different commands) in different clones. It's very easy to send patches/updates between clones (cd yzis.command1; hg in ../yzis.command2; hg pull ../yzis.command2)
@Loïc : (care : it's quite dangerous to use rev number with mercurial, better use checksums)