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

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: msucan, Assigned: Optimizer)

Tracking

({dev-doc-complete})

Trunk
Firefox 14
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sourceeditor][fixed-in-fx-team])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

5 years ago
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 {}.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
s/progress/proceed
(Reporter)

Comment 3

5 years ago
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!
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
(Assignee)

Comment 4

5 years ago
Created attachment 610120 [details] [diff] [review]
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)
Attachment #610120 - Flags: review?(mihai.sucan)
(Reporter)

Comment 5

5 years ago
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?
Attachment #610120 - Flags: review?(mihai.sucan)
(Assignee)

Comment 6

5 years ago
Okay .
I need to write test also for this ?
(Reporter)

Comment 7

5 years ago
(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();
}
(Assignee)

Comment 8

5 years ago
(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 ?
(Reporter)

Comment 9

5 years ago
(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!
(Assignee)

Comment 10

5 years ago
What about the case that I pinged you on irc ?
For that, parsing is the only way.
Any ideas ?
(Assignee)

Comment 11

5 years ago
Created attachment 610908 [details] [diff] [review]
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.
Attachment #610120 - Attachment is obsolete: true
Attachment #610908 - Flags: review?(mihai.sucan)
(Assignee)

Comment 12

5 years ago
Created attachment 610917 [details] [diff] [review]
Path v1.6

This version is tested and works for all cases.
Attachment #610908 - Attachment is obsolete: true
Attachment #610908 - Flags: review?(mihai.sucan)
(Assignee)

Updated

5 years ago
Attachment #610917 - Flags: review?(mihai.sucan)
(Reporter)

Comment 13

5 years ago
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.
Attachment #610917 - Flags: review?(mihai.sucan) → feedback+
(Assignee)

Comment 14

5 years ago
(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.
(Reporter)

Comment 15

5 years ago
(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!
(Assignee)

Comment 16

5 years ago
Okay.
(Assignee)

Comment 17

5 years ago
Created attachment 611555 [details] [diff] [review]
Patch v2.0 with tests

Addressed the comments from previous patch.
Added test for the bug.
Attachment #610917 - Attachment is obsolete: true
Attachment #611555 - Flags: review?(mihai.sucan)
(Reporter)

Comment 18

5 years ago
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.
Attachment #611555 - Flags: review?(mihai.sucan) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 612256 [details] [diff] [review]
Final patch

Updated, ready to be pushed to try.
Attachment #611555 - Attachment is obsolete: true
Attachment #612256 - Flags: review+
(Reporter)

Comment 20

5 years ago
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)
(Assignee)

Comment 21

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
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.
Attachment #612256 - Attachment is obsolete: true
Attachment #612522 - Flags: review+
(Reporter)

Comment 23

5 years ago
(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!
Depends on: 739192
(Assignee)

Comment 24

5 years ago
Created attachment 612650 [details] [diff] [review]
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.
Attachment #612522 - Attachment is obsolete: true
Attachment #612650 - Flags: review?(mihai.sucan)
(Reporter)

Comment 25

5 years ago
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!)
Attachment #612650 - Flags: review?(mihai.sucan)
(Assignee)

Comment 26

5 years ago
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) ?
(Assignee)

Comment 27

5 years ago
Created attachment 612862 [details] [diff] [review]
[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 + [/]
Attachment #612650 - Attachment is obsolete: true
Attachment #612862 - Flags: review?(mihai.sucan)
(Reporter)

Comment 28

5 years ago
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
Attachment #612862 - Flags: review?(mihai.sucan) → review+
(Reporter)

Comment 29

5 years ago
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!
Attachment #612862 - Attachment description: Final patch rebased on bug 739192's patch and fixing Style Editor issue → [in-fx-team] Final patch rebased on bug 739192's patch and fixing Style Editor issue
(Reporter)

Updated

5 years ago
Whiteboard: [sourceeditor] → [sourceeditor][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c0946061c57f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
Documentation updated:

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

Listed on Firefox 14 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.