729960 - Source Editor: add shortcuts to quickly jump to the code block start and end

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

We need shortcuts to jump to the start and end of the code block where the caret is located. In JavaScript that means jumping to the curly braces {}.

Comment 1

[Girish Sharma [:Optimizer]]

This can be done by using the editor._styler._findMatchingBracket function only, unfortunately that is a private function to orion.
Or there is one other way, parsing each and every bracket to find the matching one.
How should I progress? 
msucan: Assign this bug to me please.

Comment 2

[Girish Sharma [:Optimizer]]

s/progress/proceed

Comment 3

[Mihai Sucan [:msucan]]

To keep the code simple: yes, use _findMatchingBracket() from the Orion TextStyler. Please note that this is used only in the CSS and JS modes.

Thanks Girish!

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.0

The patch adds two shortcuts: Ctrl + '[' / ']'

the caret will move to the highlighted matching bracket (already a feature of source editor). If there is no matching bracket, then it will move to the nearest '{' in the upward direction (when Ctrl + [ is pressed) and to the nearest '}' in the downward direction (when Ctrl + ] is pressed)

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 610120 [details] [diff] [review]
Patch v1.0

Review of attachment 610120 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch Girish!

It works nicely, but not with code like:

function foo() {
  hello_world();

  if (bar) {
    bazbaz();
  }

  end_world();
}

In nested blocks Ctrl-[ and Ctrl-] become confused. For example, if I place the cursor at end_world() and I press Ctrl-[ the cursor moves to the if opening brace, instead of the expected function opening brace. Can you fix this?

Comment 6

[Girish Sharma [:Optimizer]]

Okay .
I need to write test also for this ?

Comment 7

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #6)
> Okay .
> I need to write test also for this ?

Yes! Once you get this working properly, go ahead and write a test.

Also take in consideration code like this:

function foo() {
  hello_world();

  // test { curly braces }
  if (bar) {
    bazbaz();
  }

  test("hello {foo}!");

  end_world();
}

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #7)
> Also take in consideration code like this:
> 
> function foo() {
>   hello_world();
> 
>   // test { curly braces }
>   if (bar) {
>     bazbaz();
>   }
> 
>   test("hello {foo}!");
> 
>   end_world();
> }

You mean to say that I should not move the cursor to the {/} after the // right?

Also, what about the case when the code block is not complete :

function foo() {
  hello_world();

  // test { curly braces }
  if (bar) {
    bazbaz();
  }

  test("hello {foo}!");

  end_world();

and caret is at end_(caret)world();

then should I jump to the nearest '{' on pressing Ctrl + [ or do not go anywhere ?

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #8)
> (In reply to Mihai Sucan [:msucan] from comment #7)
> > Also take in consideration code like this:
> > 
> > function foo() {
> >   hello_world();
> > 
> >   // test { curly braces }
> >   if (bar) {
> >     bazbaz();
> >   }
> > 
> >   test("hello {foo}!");
> > 
> >   end_world();
> > }
> 
> You mean to say that I should not move the cursor to the {/} after the //
> right?

Correct.

> Also, what about the case when the code block is not complete :
> 
> function foo() {
>   hello_world();
> 
>   // test { curly braces }
>   if (bar) {
>     bazbaz();
>   }
> 
>   test("hello {foo}!");
> 
>   end_world();
> 
> and caret is at end_(caret)world();
> 
> then should I jump to the nearest '{' on pressing Ctrl + [ or do not go
> anywhere ?

Ideally, it should move the caret to the first curly brace at the start of the foo() function. To keep things simpler, for now just focus on code that has no missing start/end braces.

Thanks!

Comment 10

[Girish Sharma [:Optimizer]]

What about the case that I pinged you on irc ?
For that, parsing is the only way.
Any ideas ?

Comment 11

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.5

Patch with proposed method to get levelled block start/end

Mihai: I will add more comments and check for all edge cases once you think that this method is okay.

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: Path v1.6

This version is tested and works for all cases.

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 610917 [details] [diff] [review]
Path v1.6

Review of attachment 610917 [details] [diff] [review]:
-----------------------------------------------------------------

This works surprisingly well! Awesome even if it is a "hack".

Please return true in both of the new methods. Also, what happens in the HTML and text modes?

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1190,5 @@
> +    return styler._findMatchingBracket(model, aOffset);
> +  },
> +
> +  /**
> +   * Moves the cursor to the matching opening bracket if at corresponding closing

S/Moves/Move/

@@ +1191,5 @@
> +  },
> +
> +  /**
> +   * Moves the cursor to the matching opening bracket if at corresponding closing
> +   * bracket, otherwise to the nearest opening bracket before the cursor.

otherwise move to the opening bracket for the current block of code.

(please do the same comment adjustments for the other method as well.)

@@ +1201,5 @@
> +    let caretOffset = this.getCaretOffset() - 1;
> +    let matchingIndex = this._getMatchingBracketIndex(caretOffset);
> +
> +    // If caret not at '}' bracket, finding the index of correct levelled
> +    // block start before caret.

If the caret is not at the closing bracket "}", find the index of the opening bracket "{" for the current code block.

@@ +1233,5 @@
> +    let caretOffset = this.getCaretOffset();
> +    let matchingIndex = this._getMatchingBracketIndex(caretOffset - 1);
> +
> +    // If caret not at '{' bracket, finding the index of correct levelled
> +    // block end after caret.

If the caret is not at the opening bracket "{", find the index of the closing bracket "}" for the current code block.

Comment 14

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #13)
> This works surprisingly well! Awesome even if it is a "hack".

Thanks!

> Please return true in both of the new methods. Also, what happens in the
> HTML and text modes?

Okay. What should happen in the html and text mode ? There are no brackets there.

> otherwise move to the opening bracket for the current block of code.
> (please do the same comment adjustments for the other method as well.)
> 
Hmm.
Will fix the rest of comments as well. Next patch will containt test also.

Comment 15

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #14)
> (In reply to Mihai Sucan [:msucan] from comment #13)
> > Please return true in both of the new methods. Also, what happens in the
> > HTML and text modes?
> 
> Okay. What should happen in the html and text mode ? There are no brackets
> there.

The current patch breaks in both modes. In text mode there's no ._styler IIRC. In HTML mode ._styler is different - it's not the TextStyler object, and it doesn't have the findMatchingBracket method.

I suggest you check the mode and do an early return false if the mode is unsupported. Currently this code only supports the CSS and JS modes.

> > otherwise move to the opening bracket for the current block of code.
> > (please do the same comment adjustments for the other method as well.)
> > 
> Hmm.
> Will fix the rest of comments as well. Next patch will containt test also.

Thanks!

Comment 16

[Girish Sharma [:Optimizer]]

Okay.

Comment 17

[Girish Sharma [:Optimizer]]

Attachment: Patch v2.0 with tests

Addressed the comments from previous patch.
Added test for the bug.

Comment 18

[Mihai Sucan [:msucan]]

Comment on attachment 611555 [details] [diff] [review]
Patch v2.0 with tests

Review of attachment 611555 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good!

r+ with the comments below addressed. Once you update the patch I will push it to the try server.

Thank you!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1184,5 @@
> +  _getMatchingBracketIndex: function SE__getMatchingBracketIndex(aOffset)
> +  {
> +    let model = this._model;
> +    let styler = this._styler;
> +    let text = this.getText();

Is text used in this method?

Are the styler  and model variables used more than once in this method?

@@ +1186,5 @@
> +    let model = this._model;
> +    let styler = this._styler;
> +    let text = this.getText();
> +
> +    return styler._findMatchingBracket(model, aOffset);

You could just do: return this._styler._findMatchingBracket(this._model, aOffset);

@@ +1228,5 @@
> +      this.setCaretOffset(matchingIndex);
> +      return true;
> +    }
> +
> +    return false;

At the end of the method you need to return true, even if you didn't make changes: the shortcut works for the current mode and it's legitimate for the shortcut to have no effect. We still need to prevent the default action of the keyboard shortcut.

(same for both methods!)

@@ +1253,5 @@
> +    // If the caret is not at the opening bracket "{", find the index of the
> +    // closing bracket "}" for the current code block.
> +    if (matchingIndex == -1 || matchingIndex < caretOffset) {
> +      let text = this.getText();
> +      let openingOffset = text.lastIndexOf('{', caretOffset);

Code style: please always use double quotes.

@@ +1260,5 @@
> +        if (openingMatchingIndex > caretOffset) {
> +          matchingIndex = openingMatchingIndex;
> +          break;
> +        } else {
> +          openingOffset = text.lastIndexOf('{', openingOffset - 1);

No need for else after break.

::: browser/devtools/sourceeditor/test/browser_bug729960_block_bracket_jump.js
@@ +15,5 @@
> +  let editor;
> +
> +  const windowUrl = "data:text/xml,<?xml version='1.0'?>" +
> +    "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'" +
> +    " title='test for bug 725430' width='600' height='500'><hbox flex='1'/></window>";

Please update the bug number.

@@ +83,5 @@
> +    is(editor.getCaretOffset(), 61,
> +       "JS : Jump to opening bracket in a nested function with caret inside");
> +
> +    let CSSText = "#object {\n" +
> +                  "  propert: value;\n" +

nit: property.

Comment 19

[Girish Sharma [:Optimizer]]

Attachment: Final patch

Updated, ready to be pushed to try.

Comment 20

[Mihai Sucan [:msucan]]

Comment on attachment 612256 [details] [diff] [review]
Final patch

Review of attachment 612256 [details] [diff] [review]:
-----------------------------------------------------------------

Please rebase this patch on top of bug 739192. The patch fails to apply cleanly for me. We will push both at once - first 739192, then this one.

I suggest you use mercurial queues to make it easy for you to manage the two patches. See:

http://mercurial.selenic.com/wiki/MqTutorial
https://developer.mozilla.org/en/Mercurial_Queues

Thanks!

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1220,5 @@
> +    }
> +
> +    if (matchingIndex > -1) {
> +      this.setCaretOffset(matchingIndex);
> +      return true;

Is this needed here? You have a return true below.

(the same question for the other method)

Comment 21

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #20)
> Comment on attachment 612256 [details] [diff] [review]
> Final patch
> 
> Review of attachment 612256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please rebase this patch on top of bug 739192. The patch fails to apply
> cleanly for me. We will push both at once - first 739192, then this one.
> 
> I suggest you use mercurial queues to make it easy for you to manage the two
> patches. See:
> 
> http://mercurial.selenic.com/wiki/MqTutorial
> https://developer.mozilla.org/en/Mercurial_Queues

I tried earlier, but was unable to figure out. I will give it another shot.
 
> ::: browser/devtools/sourceeditor/source-editor-orion.jsm
> @@ +1220,5 @@
> > +    }
> > +
> > +    if (matchingIndex > -1) {
> > +      this.setCaretOffset(matchingIndex);
> > +      return true;
> 
> Is this needed here? You have a return true below.
> 
> (the same question for the other method)

Yes not now.

Comment 22

[Girish Sharma [:Optimizer]]

Attachment: Final patch rebased on bug 739192's patch

Mihai, I have rebased the patch so that You can first push bug 739192 and then this one. Patch for 739192 is the same and needes no changes.

Comment 23

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #22)
> Created attachment 612522 [details] [diff] [review]
> Final patch rebased on bug 739192's patch
> 
> Mihai, I have rebased the patch so that You can first push bug 739192 and
> then this one. Patch for 739192 is the same and needes no changes.

I just looked into the patch and wanted to push both patches to the try server, but I have failures on my machine.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_bug729960_block_bracket_jump.js | CSS : Jump to closing bracket of the code block when caret at block start - Got 45, expected 44
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_bug729960_block_bracket_jump.js | CSS : Jump to closing bracket of code block when inside the function - Got 45, expected 44

There's something wrong - I did take a look at the interdiff between your new and previous patches: nothing obvious. It should all work.

Please re-run the tests on your machine and make any needed fixes. Thank you!

Comment 24

[Girish Sharma [:Optimizer]]

Attachment: Final patch rebased on bug 739192's patch

The change propert -> property was failing the 2 tests as I forgot to update 44 to 45 after adding a "y" before the "}".
Everything is fixed now.

Comment 25

[Mihai Sucan [:msucan]]

Comment on attachment 612650 [details] [diff] [review]
Final patch rebased on bug 739192's patch

This is great work! Thank you Girish!

But there's one bug I found now: open the Style Editor and press Ctrl+]: it works. Now try Ctrl+[ and you will see that the jump to the opening bracket happens, but a closing ] square bracket is added at the new caret location. This is a serious bug.

I have doubts that the bug is caused by your code - what your code does is only "read" stuff - no writing.

Currently I do not have time to debug Orion and the Style Editor. Can you please do this? Find out why the ] character is added and fix the problem.

Suggestions on how to approach this task:

1. Check if this is a problem in the Style Editor or in Orion. To do this simply set the CSS mode in scratchpad.js, so you edit CSS in scratchpad, instead of JS. If you now get the same broken behavior as in the Style Editor, then it's a bug in Orion. If the code works well in CSS mode, in Scratchpad, then this is a bug caused by the Style Editor and how it integrates Orion.

2. If you determined this is a bug in the style editor: look into how Source Editor is used in StyleEditor.jsm. search for new SourceEditor(). Check if the style editor has any keyboard event handlers and what they do, or if there's some other stuff that might break your new code.

3. If you determined this might be a bug in Orion: open orion.js and search for _findMatchingBracket() and the related methods. See if there's a possible condition for making changes in the editor. Also, add debug output, dump() calls in setText() and other methods that call setText(). See where the setText() comes from.

If you find this too hard / boring, don't worry. I can do this, but I have to finish up a patch I am working on. :)

Thank you very much for your patience!

(no try push, sorry!)

Comment 26

[Girish Sharma [:Optimizer]]

Its due to the fact that Style Editor auto completes any opening bracket.

I tried it with CSS mode Scratchpad and it worked perfectly.

I tried the latest nightly without this patch and pressing Ctrl + ']' does nothing, whereas pressing Ctrl + '[' acts as pressing "[" and thus the style editor also adds a "]" after the manually entered "[".

Now with my patch, the manually entered "[" is not getting inputted as I have an event handler to it. But the funtion that automatically adds "]" for "[" still adds it without checking for a modifier key press.

This is an easy fix, should I fix this in this patch only, or open a new follow up bug. (only a 2-3 line change) ?

Comment 27

[Girish Sharma [:Optimizer]]

Attachment: [in-fx-team] Final patch rebased on bug 739192's patch and fixing Style Editor issue

I have fixed styleeditor.jsm and added two tests in the browser_styleeditor_new.js checking for the two shortcuts Ctrl + [/]

Comment 28

[Mihai Sucan [:msucan]]

Comment on attachment 612862 [details] [diff] [review]
[in-fx-team] Final patch rebased on bug 739192's patch and fixing Style Editor issue

Review of attachment 612862 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for finding the problem and for the fix! r+!

I pushed your patches to the try server:
https://tbpl.mozilla.org/?tree=Try&rev=2b5af962163c

Comment 29

[Mihai Sucan [:msucan]]

Comment on attachment 612862 [details] [diff] [review]
[in-fx-team] Final patch rebased on bug 739192's patch and fixing Style Editor issue

Landed:
https://hg.mozilla.org/integration/fx-team/rev/c0946061c57f

Thank you Girish!

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/c0946061c57f

Comment 31

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/Tools/Using_the_Source_Editor#Keyboard_commands

Listed on Firefox 14 for developers.