Project

General

Profile

Actions

Patch submission #507

closed

% command created

Added by Arnaud Tanguy over 13 years ago. Updated over 13 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
libyzis
Target version:
Start date:
01/17/2009
Due date:
% Done:

100%

Estimated time:

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

percentCommand.patch (2.9 KB) percentCommand.patch Arnaud Tanguy, 01/17/2009 11:31 AM
percentCommand2.patch (2.9 KB) percentCommand2.patch Arnaud Tanguy, 01/17/2009 07:17 PM
percentCommand.diff (7.49 KB) percentCommand.diff Arnaud Tanguy, 01/17/2009 08:19 PM
percentCommand.diff (6.14 KB) percentCommand.diff Arnaud Tanguy, 01/18/2009 08:49 AM
percentCommand_fix.diff (7.55 KB) percentCommand_fix.diff Arnaud Tanguy, 01/18/2009 05:00 PM
percentCommand_fix2.diff (7.51 KB) percentCommand_fix2.diff Arnaud Tanguy, 01/18/2009 05:37 PM
percentCommand.hg (1.34 MB) percentCommand.hg Arnaud Tanguy, 01/18/2009 06:20 PM
percentCommand_fix.hg (1.34 MB) percentCommand_fix.hg Arnaud Tanguy, 01/19/2009 07:54 AM
Actions #1

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 ?

Actions #2

Updated by Arnaud Tanguy over 13 years ago

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'.

Actions #3

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.

Actions #4

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).

Actions #5

Updated by Arnaud Tanguy over 13 years ago

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.

Actions #6

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.

Actions #7

Updated by Arnaud Tanguy over 13 years ago

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.

Actions #8

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.

Actions #9

Updated by Arnaud Tanguy over 13 years ago

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

Actions #10

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 :)

Actions #11

Updated by Arnaud Tanguy over 13 years ago

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 ?

Actions #12

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.

Actions #13

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.

Actions #14

Updated by Arnaud Tanguy over 13 years ago

Here is the bundle.

Actions #15

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!

Actions #16

Updated by Arnaud Tanguy over 13 years ago

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.

Actions #17

Updated by Loïc P. over 13 years ago

Applied, thanks.
Please open a separate bug report next time ;)

Actions #18

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)

Actions

Also available in: Atom PDF