Feat: exposing getter setter for widget manager created using line widgets #5673

nlujjawal · 2024-11-12T15:38:01Z

Description of changes:
Exposing getter and setter for widget manager created using line widgets.
So now ace consumers can use the getter to access the widgetManager which can be used to create line widgets.

Added and setter which creates widgetManger(an instance of line_widgets) link it with editor.session.widgetManager and attached it the editor.
Added and getter which returns the widgetManger(an instance of line_widgets) attached with the edit_session and not with editor because session can be created without editor with some line_widgets so session.widgetManager should work without editor. And that session can be linked with editor later.
Updated the ace.d.ts types to have the setWidgetManager and getWidgetManager methods in Editor type.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Pull Request Checklist:

No backwards incompatible changes were made to Ace's public interface which is defined through the main typings file (ace.d.ts) and its references:

github-actions · 2024-11-12T15:38:19Z

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

akoreman · 2024-11-12T15:40:27Z

src/editor.js

+     * Set "widgetManager" in editor.session
+     * @returns void
+     */
+    setWidgetManager() {


calling this setWidgetManager might lead to the assumption that it accepts a parameter which gets set, maybe something like attachWidgetManager makes sense?

akoreman · 2024-11-12T15:42:06Z

ace.d.ts

@@ -953,6 +953,8 @@ export namespace Ace {
    splitLine(): void;
    setGhostText(text: string, position: Point): void;
    removeGhostText(): void;
+    setWidgetManager(): void;


Do we need to expose this function? consumers will want to access the widget manager (which they can do through getWidgetManager) and when it's not defined yet that will be handled by this getter. Do you see an use-case where someone wants to attach a widget manager but not use it afterwards?

akoreman · 2024-11-12T15:43:24Z

ace.d.ts

@@ -953,6 +953,8 @@ export namespace Ace {
    splitLine(): void;
    setGhostText(text: string, position: Point): void;
    removeGhostText(): void;
+    setWidgetManager(): void;
+    getWidgetManager(): Ace.LineWidgets;


Ace.LineWidgets is only exported from the internal type declaration file at the moment right? we should include it in this declaration file as well if we're exposing this function. (this is probably also what's breaking the CI build atm)

akoreman · 2024-11-12T15:45:02Z

We also use the widget manager for the ghost text here, does it make sense to convert this instance as well to the new functions?

nightwing · 2024-11-12T21:13:40Z

The reason widgetManager was not defined here, was that initially we wanted to keep lineWidgets as an optional feature that could be excluded from the bundle. Since we have added import of lineWidgets to the editor itself, i'd suggest to convert session.widgetManager to a getter, so that this.session.widgetManager will always return something.

diff --git a/src/edit_session.js b/src/edit_session.js
index 718105762..af93087e5 100644
--- a/src/edit_session.js
+++ b/src/edit_session.js
@@ -12,6 +12,7 @@ var oop = require("./lib/oop");
 var lang = require("./lib/lang");
 var BidiHandler = require("./bidihandler").BidiHandler;
 var config = require("./config");
+var { LineWidgets } = require("./line_widgets");
 var EventEmitter = require("./lib/event_emitter").EventEmitter;
 var Selection = require("./selection").Selection;
 var TextMode = require("./mode/text").Mode;
@@ -2386,6 +2387,23 @@ class EditSession {
         return currentLine[pos.column - 1];
     }
 
+    get widgetManager() {
+        var widgetManager = new LineWidgets(this);
+        
+        if (this.$editor)
+            widgetManager.attach(this.$editor);
+
+        return widgetManager;
+    }
+    set widgetManager(value) {
+        Object.defineProperty(this, "widgetManager", {
+            writable: true, 
+            enumerable: true,
+            configurable: true,
+            value: value,
+        });
+    }
+
     destroy() {
         if (!this.destroyed) {
             this.bgTokenizer.setDocument(null);
diff --git a/src/editor.js b/src/editor.js
index c9d2bf7a6..c9df18860 100644
--- a/src/editor.js
+++ b/src/editor.js
@@ -23,7 +23,6 @@ var CommandManager = require("./commands/command_manager").CommandManager;
 var defaultCommands = require("./commands/default_commands").commands;
 var config = require("./config");
 var TokenIterator = require("./token_iterator").TokenIterator;
-var LineWidgets = require("./line_widgets").LineWidgets;
 var GutterKeyboardHandler = require("./keyboard/gutter_handler").GutterKeyboardHandler;
 var nls = require("./config").nls;
 
@@ -372,7 +371,9 @@ class Editor {
         this.curOp = null;
         
         oldSession && oldSession._signal("changeEditor", {oldEditor: this});
+        if (oldSession) oldSession.$editor = null;
         session && session._signal("changeEditor", {editor: this});
+        if (session) session.$editor = this;
         
         if (session && !session.destroyed)
             session.bgTokenizer.scheduleStart();
@@ -1502,10 +1503,6 @@ class Editor {
      * @param {Point} [position] Position to insert text to
      */
     setGhostText(text, position) {
-        if (!this.session.widgetManager) {
-            this.session.widgetManager = new LineWidgets(this.session);
-            this.session.widgetManager.attach(this);
-        }
         this.renderer.setGhostText(text, position);
     }
 
@@ -1513,8 +1510,6 @@ class Editor {
      * Removes "ghost" text currently displayed in the editor.
      */
     removeGhostText() {
-        if (!this.session.widgetManager) return;
-
         this.renderer.removeGhostText();
     }

github-actions · 2024-11-13T16:43:28Z

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

codecov · 2024-11-13T16:45:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.01%. Comparing base (74e95d1) to head (f1d6816).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5673      +/-   ##
==========================================
- Coverage   87.01%   87.01%   -0.01%     
==========================================
  Files         598      598              
  Lines       43681    43680       -1     
  Branches     7205     7204       -1     
==========================================
- Hits        38010    38009       -1     
  Misses       5671     5671

Flag	Coverage Δ
unittests	`87.01% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-11-13T16:46:43Z

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

src/edit_session.js

github-actions · 2024-11-13T16:50:31Z

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

github-actions · 2024-11-13T16:58:55Z

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

…dgets

ace.d.ts

github-actions · 2024-11-13T17:03:07Z

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

src/edit_session.js

…ated using line widgets

github-actions · 2024-11-14T10:13:51Z

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

akoreman · 2024-11-14T10:47:44Z

src/edit_session.js

+     * @returns {LineWidgets} object
+     */
+    get widgetManager() {
+        const widgetManager = new LineWidgets(this);


how does this work when both consumers want to use editor.widgetManager and we want to use it internally? Doesn't this mean that when a consumer calls the widgetManager all the existing widgets currently in the editor get deleted?

nice catch. Will be adding the check to only create widgetManager if it doesn't exists.

github-actions · 2024-11-14T10:53:00Z

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

akoreman · 2024-11-14T10:59:17Z

src/edit_session.js

+    /**
+     * Get "widgetManager" from EditSession
+     * 
+     * @returns {LineWidgets} object


This gives session.widgetManager the LineWidgets type, which I think means that session.widgetManager.addLineWidget(...) will result in a typescript error right? Should this return type be WidgetManager?

github-actions · 2024-11-14T11:01:16Z

One of the public type files has been updated, plase make sure there are no backwards incompatible changes done in the PR.

nlujjawal requested a review from akoreman November 12, 2024 15:38

akoreman reviewed Nov 12, 2024

View reviewed changes

nlujjawal force-pushed the expose_widget_manager branch from ec97825 to c381585 Compare November 13, 2024 16:43

nlujjawal force-pushed the expose_widget_manager branch from c381585 to 52436cd Compare November 13, 2024 16:46

akoreman reviewed Nov 13, 2024

View reviewed changes

src/edit_session.js Show resolved Hide resolved

nlujjawal force-pushed the expose_widget_manager branch from 52436cd to f0bed9b Compare November 13, 2024 16:50

nlujjawal force-pushed the expose_widget_manager branch from f0bed9b to c08b103 Compare November 13, 2024 16:58

Feat: exposing getter setter for widget manager created using line wi…

fdc468c

…dgets

akoreman reviewed Nov 13, 2024

View reviewed changes

ace.d.ts Outdated Show resolved Hide resolved

nlujjawal force-pushed the expose_widget_manager branch from c08b103 to fdc468c Compare November 13, 2024 17:02

nightwing reviewed Nov 13, 2024

View reviewed changes

src/edit_session.js Show resolved Hide resolved

Feat: exposing getter setter for widget manager from edit_session cre…

7d2a56c

…ated using line widgets

nightwing approved these changes Nov 14, 2024

View reviewed changes

akoreman reviewed Nov 14, 2024

View reviewed changes

added condition to check if widget manager already exists

5ab34ee

akoreman reviewed Nov 14, 2024

View reviewed changes

revert last commit

f1d6816

nlujjawal merged commit f5d0c19 into master Nov 14, 2024
4 checks passed

nlujjawal deleted the expose_widget_manager branch November 14, 2024 11:13

akoreman mentioned this pull request Nov 20, 2024

chore: small tweaks LineWidget type #5683

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: exposing getter setter for widget manager created using line widgets #5673

Feat: exposing getter setter for widget manager created using line widgets #5673

nlujjawal commented Nov 12, 2024 •

edited

Loading

github-actions bot commented Nov 12, 2024

akoreman Nov 12, 2024

akoreman Nov 12, 2024

akoreman Nov 12, 2024 •

edited

Loading

akoreman commented Nov 12, 2024

nightwing commented Nov 12, 2024

github-actions bot commented Nov 13, 2024

codecov bot commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 14, 2024

akoreman Nov 14, 2024

nlujjawal Nov 14, 2024

github-actions bot commented Nov 14, 2024

akoreman Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024

Feat: exposing getter setter for widget manager created using line widgets #5673

Feat: exposing getter setter for widget manager created using line widgets #5673

Conversation

nlujjawal commented Nov 12, 2024 • edited Loading

github-actions bot commented Nov 12, 2024

akoreman Nov 12, 2024

Choose a reason for hiding this comment

akoreman Nov 12, 2024

Choose a reason for hiding this comment

akoreman Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

akoreman commented Nov 12, 2024

nightwing commented Nov 12, 2024

github-actions bot commented Nov 13, 2024

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

github-actions bot commented Nov 14, 2024

akoreman Nov 14, 2024

Choose a reason for hiding this comment

nlujjawal Nov 14, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 14, 2024

akoreman Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 14, 2024

nlujjawal commented Nov 12, 2024 •

edited

Loading

akoreman Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

akoreman Nov 14, 2024 •

edited

Loading