Improvements to NotebookUtils for conversions between line-based

positions and string offsets

- Avoiding unnecessary string split and joins
- Added unit tests

Author: sid-srini

nbcode/notebooks/nbproject/project.xml

@@ -29,6 +29,15 @@
                         <specification-version>2.11.0</specification-version>
                     </run-dependency>
                 </dependency>
+                <dependency>
+                    <code-name-base>org.netbeans.api.annotations.common</code-name-base>
+                    <build-prerequisite/>
+                    <compile-dependency/>
+                    <run-dependency>
+                        <release-version>1</release-version>
+                        <specification-version>1.57</specification-version>
+                    </run-dependency>
+                </dependency>
                 <dependency>
                     <code-name-base>org.netbeans.api.java</code-name-base>
                     <build-prerequisite/>
@@ -153,10 +162,10 @@
                         <recursive/>
                         <compile-dependency/>
                     </test-dependency>
-                     <test-dependency>
-                    <code-name-base>org.netbeans.modules.java.lsp.server</code-name-base>
-                         <compile-dependency/>
-                </test-dependency>
+                    <test-dependency>
+                        <code-name-base>org.netbeans.modules.java.lsp.server</code-name-base>
+                        <compile-dependency/>
+                    </test-dependency>
                 </test-type>
             </test-dependencies>
             <public-packages/>

nbcode/notebooks/src/org/netbeans/modules/nbcode/java/notebook/NotebookDocumentStateManager.java

@@ -30,7 +30,6 @@
 import org.eclipse.lsp4j.NotebookDocumentChangeEvent;
 import org.eclipse.lsp4j.NotebookDocumentChangeEventCellStructure;
 import org.eclipse.lsp4j.NotebookDocumentChangeEventCellTextContent;
-import org.eclipse.lsp4j.Position;
 import org.eclipse.lsp4j.Range;
 import org.eclipse.lsp4j.TextDocumentContentChangeEvent;
 import org.eclipse.lsp4j.TextDocumentIdentifier;
@@ -48,8 +47,14 @@ public class NotebookDocumentStateManager {
     private final NotebookDocument notebookDoc;
     private final Map<String, CellState> cellsMap = new ConcurrentHashMap<>();
     private final List<String> cellsOrder;
+    private final CellStateCreator cellStateCreator;
 
     public NotebookDocumentStateManager(NotebookDocument notebookDoc, List<TextDocumentItem> cells) {
+        this(notebookDoc, cells, null);
+    }
+    
+    public NotebookDocumentStateManager(NotebookDocument notebookDoc, List<TextDocumentItem> cells, CellStateCreator cellStateCreator) {
+        this.cellStateCreator = cellStateCreator != null ? cellStateCreator : CellState::new;
         this.notebookDoc = notebookDoc;
         this.cellsOrder = new ArrayList<>();
         Iterator<NotebookCell> notebookCellsIterator = notebookDoc.getCells().iterator();
@@ -193,7 +198,7 @@ private void updateNotebookCellContent(NotebookDocumentChangeEventCellTextConten
             cellState.setContent(updatedContent, newVersion);
             LOG.log(Level.FINE, "Updated content for cell: {0}, version: {1}", new Object[]{uri, newVersion});
         } catch (Exception e) {
-            LOG.log(Level.WARNING, "applyContentChanges failed, requesting full content: " + uri, e);
+            LOG.log(Level.WARNING, "applyContentChanges failed, requesting full content for cell: {0}. Error - {1}", new Object[]{uri, e});
             try {
                 cellState.requestContentAndSet();
             } catch (Exception ex) {
@@ -222,60 +227,33 @@ private String applyContentChanges(String originalContent, List<TextDocumentCont
 
     private String applyRangeChange(String content, TextDocumentContentChangeEvent change) {
         Range range = change.getRange();
-        Position start = range.getStart();
-        Position end = range.getEnd();
-
-        String[] lines = content.split("\n", -1);
-
-        if (start.getLine() < 0 || start.getLine() >= lines.length
-                || end.getLine() < 0 || end.getLine() >= lines.length) {
-            throw new IllegalArgumentException("Invalid range positions");
-        }
-
-        StringBuilder result = new StringBuilder();
-
-        for (int i = 0; i < start.getLine(); i++) {
-            result.append(lines[i]);
-            if (i < lines.length - 1) {
-                result.append("\n");
-            }
-        }
-
-        String startLine = lines[start.getLine()];
-        String beforeChange = startLine.substring(0, Math.min(start.getCharacter(), startLine.length()));
-        result.append(beforeChange);
-
-        result.append(change.getText());
-
-        String endLine = lines[end.getLine()];
-        String afterChange = endLine.substring(Math.min(end.getCharacter(), endLine.length()));
-        result.append(afterChange);
-
-        for (int i = end.getLine() + 1; i < lines.length; i++) {
-            result.append("\n").append(lines[i]);
-        }
-
-        return result.toString();
+        return NotebookUtils.applyChange(content, range.getStart(), range.getEnd(), change.getText());
     }
 
-    // protected methods for ease of unit testing
-    protected void addNewCellState(NotebookCell cell, TextDocumentItem item) {
+
+    private void addNewCellState(NotebookCell cell, TextDocumentItem item) {
         if (cell == null || item == null) {
             LOG.log(Level.WARNING, "Attempted to add null cell or item");
             return;
         }
 
+        CellState cellState;
         try {
-            CellState cellState = new CellState(cell, item, notebookDoc.getUri());
-            cellsMap.put(item.getUri(), cellState);
+            cellState = cellStateCreator.create(cell, item, notebookDoc.getUri());
             LOG.log(Level.FINE, "Added new cell state: {0}", item.getUri());
         } catch (Exception e) {
             LOG.log(Level.SEVERE, "Failed to create cell state for: " + item.getUri(), e);
             throw new RuntimeException("Failed to create cell state", e);
         }
+        cellsMap.put(item.getUri(), cellState);
     }
 
     protected Map<String, CellState> getCellsMap() {
         return cellsMap;
     }
+    
+    // protected methods for ease of unit testing
+    protected interface CellStateCreator {
+        CellState create(NotebookCell cell, TextDocumentItem item, String notebookDocUri);
+    }
 }

nbcode/notebooks/src/org/netbeans/modules/nbcode/java/notebook/NotebookUtils.java

@@ -22,84 +22,69 @@
 import com.google.gson.JsonPrimitive;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.regex.Pattern;
 import jdk.jshell.SourceCodeAnalysis;
 import org.eclipse.lsp4j.Position;
+import org.netbeans.api.annotations.common.NonNull;
 
 /**
  *
  * @author atalati
  */
 public class NotebookUtils {
+    private static final Pattern LINE_ENDINGS = Pattern.compile("\\R");
 
     public static String normalizeLineEndings(String text) {
-        if (text == null) {
-            return null;
-        }
-
-        if (text.indexOf('\r') == -1) {
-            return text;
-        }
-
-        StringBuilder normalized = new StringBuilder(text.length());
-        int len = text.length();
-
-        for (int i = 0; i < len; i++) {
-            char c = text.charAt(i);
-            if (c == '\r') {
-                if (i + 1 < len && text.charAt(i + 1) == '\n') {
-                    i++;
-                }
-                normalized.append('\n');
-            } else {
-                normalized.append(c);
-            }
-        }
-
-        return normalized.toString();
+        return text == null ? null : LINE_ENDINGS.matcher(text).replaceAll("\n");
     }
 
     public static int getOffset(String content, Position position) {
         if (content == null || position == null) {
             return 0;
         }
 
-        String[] lines = content.split("\n", -1);
-        int offset = 0;
         int targetLine = position.getLine();
-        int targetChar = position.getCharacter();
-
         if (targetLine < 0) {
             return 0;
         }
-        if (targetLine >= lines.length) {
-            return content.length();
-        }
+        int targetChar = Math.max(0, position.getCharacter());
 
-        for (int i = 0; i < targetLine; i++) {
-            offset += lines[i].length() + 1;
-        }
+        int lineStartIndex = -1;
+        int lineEndIndex = -1;
+        int line = -1;
 
-        String currentLine = lines[targetLine];
-        int charPosition = Math.min(Math.max(targetChar, 0), currentLine.length());
-        offset += charPosition;
+        // find the start line in content
+        do {
+            lineStartIndex = lineEndIndex + 1;
+            lineEndIndex = content.indexOf('\n', lineStartIndex);
+            line++;
+        } while (line < targetLine && lineEndIndex >= 0);
 
-        return Math.min(offset, content.length());
+        return line < targetLine ? content.length() : Math.min(lineStartIndex + targetChar, lineEndIndex < 0 ? content.length() : lineEndIndex);
     }
 
-    public static Position getPosition(String content, int offset) {
-        if (content == null || offset <= 0) {
+    public static Position getPosition(String text, int offset) {
+        if (text == null || offset < 0) {
             return new Position(0, 0);
         }
 
-        int clampedOffset = Math.min(offset, content.length());
-
-        String textUpToOffset = content.substring(0, clampedOffset);
-        String[] lines = textUpToOffset.split("\n", -1);
-
-        int line = lines.length - 1;
-        int character = lines[line].length();
-
-        return new Position(line, character);
+        offset = Math.min(offset, text.length());
+        int lineStartIndex = -1;
+        int lineEndIndex = -1;
+        int line = -1;
+
+        // count line endings in content upto offset
+        do {
+            lineStartIndex = lineEndIndex + 1;
+            lineEndIndex = text.indexOf('\n', lineStartIndex);
+            line++;
+        } while (lineEndIndex >= 0 && offset > lineEndIndex);
+        
+        if (offset == lineEndIndex) {
+            return new Position(line + 1, 0);
+        } else {
+            return new Position(line, offset - lineStartIndex);
+        }
     }
 
     public static boolean checkEmptyString(String input) {
@@ -158,4 +143,76 @@ public static List<String> getCodeSnippets(SourceCodeAnalysis analysis, String c
 
         return codeSnippets;
     }
+
+    /**
+     * Applies the supplied change, that is encoded as a diff
+     * i.e. `{range-start, range-end, text-replacement}`, to the supplied text.
+     *
+     * This diff format can encode additions, deletions and modifications at a
+     * single range in the text.
+     *
+     * The supplied text is expected to contain normalized line endings, and, the
+     * new text adheres to the line ending normalization.
+     *
+     * @param  text         existing text
+     * @param  start        start of the range of replaced text
+     * @param  end          end of the range of replaced text
+     * @param  replacement  text to be added at the supplied position in text
+     * @throws IllegalArgumentException - when the supplied diff range is invalid
+     */
+    public static String applyChange(@NonNull String text, @NonNull Position start, @NonNull Position end, @NonNull String replacement) throws IllegalArgumentException {
+        int startLine = start.getLine();
+        int startLineOffset = start.getCharacter();
+        int endLine = end.getLine();
+        int endLineOffset = end.getCharacter();
+
+        if (startLine < 0 || endLine < startLine || (endLine == startLine && endLineOffset < startLineOffset)) {
+            throw new IllegalArgumentException("Invalid range positions");
+        }
+        
+        if (replacement.length() == 0 && startLine == endLine && startLineOffset == endLineOffset) {
+            return text; // Nothing to be done; no addition nor deletion
+        }
+
+        final int textLength = text.length();
+
+        int lineStartIndex = -1;
+        int lineEndIndex = -1;
+        int line = -1;
+
+        // find the start line in content
+        do {
+            lineStartIndex = lineEndIndex + 1;
+            lineEndIndex = text.indexOf('\n', lineStartIndex);
+            line++;
+        } while (line < startLine && lineEndIndex >= 0);
+
+        if (line < startLine) {
+            throw new IllegalArgumentException("Invalid range start out of bounds");
+        }
+
+        StringBuilder result = new StringBuilder(textLength + replacement.length());
+
+        // append content before the change
+        result.append(text, 0, Math.min(lineStartIndex + startLineOffset, lineEndIndex < 0 ? textLength : lineEndIndex));
+        // append added text, with line ending normalization
+        result.append(LINE_ENDINGS.matcher(replacement).replaceAll("\n"));
+
+        // find the end line in content
+        while (line < endLine && lineEndIndex >= 0) {
+            lineStartIndex = lineEndIndex + 1;
+            lineEndIndex = text.indexOf('\n', lineStartIndex);
+            line++;
+        }
+
+        if (line < endLine) {
+            throw new IllegalArgumentException("Invalid range end out of bounds");
+        }
+
+        if (lineStartIndex >= 0) {
+            // append content after the change
+            result.append(text, Math.min(lineStartIndex + endLineOffset, lineEndIndex < 0 ? textLength : lineEndIndex), textLength);
+        }
+        return result.toString();
+    }
 }

nbcode/notebooks/test/unit/src/org/netbeans/modules/nbcode/java/notebook/NotebookDocumentStateManagerTest.java

@@ -201,18 +201,8 @@ public void testSyncState_UpdateCellContent_FailureAndFallback() throws Interrup
     }
 
     private static class TestableNotebookDocumentStateManager extends NotebookDocumentStateManager {
-
         public TestableNotebookDocumentStateManager(NotebookDocument notebookDoc, List<TextDocumentItem> cells) {
-            super(notebookDoc, cells);
-        }
-
-        @Override
-        protected void addNewCellState(NotebookCell cell, TextDocumentItem item) {
-            if (cell == null || item == null) {
-                return;
-            }
-            CellState cellState = new TestableCellState(cell, item, getNotebookDocument().getUri());
-            getCellsMap().put(item.getUri(), cellState);
+            super(notebookDoc, cells, TestableCellState::new);
         }
     }