744021 - Source Editor jump to prev/next block should move to prev/next sibling block when caret has no parent block

Status: RESOLVED FIXED

(DevTools :: Source Editor, enhancement)

Description

[Cedric Vivier [:cedricv]]

eg.


#foo {

}

#bar { // caret is here, ctrl-[ should jump to the bracket after #foo

}

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: Patch v0.1

This allows the required behavior. But there is one problem, while jumping to next/previous code block starting/ending, it will jump to any {/} even if it is inside a comment block/line or something like this :

{ //1

} //2   Caret after "}"


{ //3

  { //4


  } // 5

} // 6


When caret at the mentioned position, pressing Ctrl + } should jump to next sibling code block ending bracket, but it will go to 5th bracket instead.

If I want to correct this behavior, I would have to loop though the brackets once again. That will result in twice the number of computation from what were happening originally. (i.e. looping through the whole text of the file again.)

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.0

Changed the code according to the discussion on irc with Mihai.

Comment 3

[Mihai Sucan [:msucan]]

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

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

Thanks for your patch!

This is really close to something we can use, but I am not sure yet if we really want to land this.

One comment below...

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1272,5 @@
>        }
> +      // Moving to the previous code block starting bracket if caret not inside
> +      // any code block.
> +      if (matchingIndex == -1) {
> +        let lastOpeningOffset = text.lastIndexOf("{", caretOffset);

Why do you search for the opening brace? Why not the closing brace?

I tested with:

if (a) {
  if (a2) {
    // ...
  }
}

if (b) {¦
  if (b2) {
    // ...
  }
}

The ¦ character shows the cursor placement. If I press Ctrl-[ the cursor jumps to the curly brace of a2, which is unexpected. I expected the cursor will be placed at the start of the curly brace for "a".

The action for Ctrl-] has similar unexpected behavior.

Comment 4

[Girish Sharma [:Optimizer]]

Hmm, will implement in that way.

Comment 5

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.1

Comments addressed.
Please tell about the test. Should I make a new one, or add 2 (or 4 at max) more test cases in the current test related to bracket jump.

Comment 6

[Mihai Sucan [:msucan]]

Comment on attachment 615813 [details] [diff] [review]
Patch v1.1

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

Please write a separate test. Thanks!

One comment below.

::: browser/devtools/sourceeditor/source-editor-orion.jsm
@@ +1279,5 @@
> +          if (closingMatchingIndex < caretOffset && closingMatchingIndex != -1) {
> +            matchingIndex = closingMatchingIndex;
> +            break;
> +          }
> +          lastClosingOffset = text.lastIndexOf("{", lastClosingOffset - 1);

The call to lastIndexOf("{") was changed to lastIndexOf("}") before the while loop, but not inside the loop. Why?

Comment 7

[Girish Sharma [:Optimizer]]

Attachment: Patch v1.2

Fixed the bracket that I forgot to fix earlier.

Comment 8

[Girish Sharma [:Optimizer]]

Attachment: Patch v2.0 with tests

A separate test file for this bug included in this patch.

Since in this bug one earlier behavior is change i.e. In a code block like :

function () {
  // some code
|}| //Caret is at either of '|'

Pressing Ctrl + [ will move the caret at after the opening bracket "{" like "{|"

Earlier caret was moved before the opening bracket "{" like "|{"
This made the caret to come out of the code block, and since in this bug, consecutive Ctrl + [ lead to sibling jump, I updated the behavior so as to make the caret visually inside the code block only.

I know this was not required for this bug, but It was an inconsistent behavior that pressing Ctrl + ] makes the caret remain inside the code block ( "|}" ) while Ctrl + [ does not.

Thus corresponding test cases in the test of bug 729960 were updated by incrementing the checking offset by 1.

Comment 9

[Mihai Sucan [:msucan]]

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

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

Thanks for your patch Girish!

(sorry for the delay - been really busy with other work!)

Patch looks good but Ctrl-[ and Ctrl-] feel slow for me. I am not sure what we should do about this. I will look into this and let you know if we can land this as is, or if we want a different solution. Thanks!

Comment 10

[Mihai Sucan [:msucan]]

Girish: at this point the hack we have in this patch is a bit too much (diminishing return). The proposed feature here is not too important and the hack will become harder to maintain later. When we have better suited APIs in Orion we can return to this bug and implement a better solution.

Please let me know if you have any unaddressed questions/concerns.

Comment 11

[Girish Sharma [:Optimizer]]

I once had an alternate solution in mind. Indexing every bracket in the editor. Upon any change, the index will be updated for the entries that come after the changed location. Thus on pressing the shortcut, it would be just looping through an array, and not the whole text. Updating the index can also be done tactfully - lets say on line change basis - i.e. not updating on each new character addition, but updating only when cursor changes lines.
But I think that it would be too much of code for such a small enhancement of an existing feature.

Comment 12

[Cedric Vivier [:cedricv]]

I disagree it is not important... the current feature is almost useless in CSS mode.
In fact, the whole complexity of this patch imho is due to the "block"-finding behavior.

IMHO, for CSS this is unneeded and the feature would be already much more useful if it worked simply as "jump to previous/next {/}".

Comment 13

[Girish Sharma [:Optimizer]]

I have one better proposal:
1) In CSS mode, only check the above or below paragraph to find the next or previous sibling resp., no checking for parent, as generally we don't expect parent bracketing in CSS. At most there can be one parent '@-moz-document url() {...}'

2) In JS mode, If we can somehow just have a variable which will be a boolean indicating whether there is a parent code block for the block where the caret is put. (This will require 1 loop of checkMAtchinBracket, thus I prefer a small worker for this task). So when the user presses the shortcut, we already know whether there is a parent or not. Thus, jumping would be very easy.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Moving to Source Editor component.

Filter on CHELICERAE.

Comment 15

[Girish Sharma [:Optimizer]]

@Mihai, any opinions ?
Is the Orion update going to improve the condition of the complexity involved ?

Comment 16

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #15)
> @Mihai, any opinions ?
> Is the Orion update going to improve the condition of the complexity
> involved ?

No. We should use a CSS parser (or JS parser) which is aware of code blocks.

Comment 17

[Girish Sharma [:Optimizer]]

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

I tried finding a good parser, but none of them had solely the bracket knowledge, not even esprima or reflect.parse.

To do some performance testings, I applied the current patch (to my surprise, it got applied cleanly), and tested it on a 20K line code (an actual file, not randomly generated) for JS, and 15K for CSS, and it worked great. I could not perceive any visual lag so I think this patch should be fine.

The most that can be done is to remove the moving to next code block logic in JS (as this is very much required for CSS, atleast), but I would not suggest that as this is very useful in JS as well.

I think supporting upto 20K lines of code (atleast) without any lag is a valid use case, as not many people go beyond that on Scratchpad.

Rest is left up to you. Adding the review request back again :)

Comment 18

[Mihai Sucan [:msucan]]

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

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

Patch looks good. Thank you! Apologies for the review delay. I've been very busy with other work.

Nit: some lines in the patch go over 80 chars.

Second minor nit noted below...

::: browser/devtools/sourceeditor/test/browser_bug744021_next_prev_bracket_jump.js
@@ +13,5 @@
> +  waitForExplicitFinish();
> +
> +  let editor;
> +
> +  const windowUrl = "data:text/xml,<?xml version='1.0'?>" +

Please add ;charset=utf8 to avoid a warning.

Comment 19

[Girish Sharma [:Optimizer]]

Attachment: patch v2.1

Addressed NITs
carry forward r+

Comment 20

[Mihai Sucan [:msucan]]

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

Thank you Girish!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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