Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add atom.textEditors.add to TE constructor #1190

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

asiloisad
Copy link
Contributor

@asiloisad asiloisad commented Jan 10, 2025

Problem

Hi, I want make a package that works with mini text-editors of find-and-replace package (and others mini text-editors). The problem is that mini text-editors are not a part of workspace, so atom.workspace.getActiveTextEditor() doesn't work. A package doesn't register it to atom.textEditors too. A mini text-editors can be added to atom.textEditors by hand , but no one use it, built-in packages included. Six of my community packages can be improve by this change.

Alternatives

I'm using a little hacky method to do it by now, but It has many cons, e.g. I cannot detect mini text-editor before user send event to it, cannot observe it etc.

function getEditor(event) {
    const element = event.target.closest('atom-text-editor')
    if (!element) { return }
    const editor = element.getModel()
    if (!editor) { return }
    return editor
}

@asiloisad
Copy link
Contributor Author

asiloisad commented Jan 10, 2025

It works as desired. I can get access to all text-editors via atom.textEditors.getActiveTextEditor() and observe all text-editors by atom.textEditors.observe(callback). The common API atom.workspace.getActiveTextEditor() doesn't change after all.

All tests failed, but I do not know what is going on 🙁

@asiloisad asiloisad changed the title [Experimental] Add atom.textEditors.add to TE constructor Add atom.textEditors.add to TE constructor Jan 10, 2025
@savetheclocktower
Copy link
Contributor

Package tests failed because of a CI issue with the new Ubuntu images on GitHub Actions. Not your fault.

I think this isn't something that should happen automatically; it would instantly change the meaning of atom.workspace.getActiveTextEditor(). If we did add mini text editors to the registry, the time to do that would not be in the TextEditor constructor, since the editor doesn't exist in the workspace until it's been appended to an element.

I'd be open to supporting this use case a bit more officially if other users were clamoring for this, but I think you're the first one. :)

If you want better ways to detect when a mini text editor is an active text editor, I'd suggest doing something like

document.addEventListener('focusin', () => {
  if (document.activeElement.matches(`atom-text-editor[mini]`) {
    // do something
  }
});

You could also try a MutationObserver to know when a subtree changes (and a new atom-text-editor[mini] may have been added to the page), but that's a bit chatty.

@asiloisad
Copy link
Contributor Author

There was a similar disscousing at discord. You have be a member of thread and said "my instinct is that it's probably fine for getActiveTextEditor() to return whatever text editor has focus, even if it isn't a “full” editor in a pane".

I want to notice, that atom.textEditors has never been official API. It was mention in AtomEnvironment, but lead to "Page not found". My feeling is that we can improve API of program without any regression. There has been a lot of question at old atom forum how to get deatached text editors via API and all goes into tricky solutions. In source code it still marked as "Experimental" from many, many years.

As I mentioned - six of my packages would benefit from this improvement. Other users could also benefit. There are currently not many new packages appearing, which may be why you do not see requests for this topic.

Another solution would be to create a separate registry, but this seems unnecessary since atom.textEditors has this use.

@asiloisad
Copy link
Contributor Author

A command atom.workspace.getActiveTextEditor() remain unchanged, I want change atom.textEditors.

@savetheclocktower
Copy link
Contributor

So I see this comment in text-editor-registry.js:

// If you want packages to be able to add functionality to your non-pane text
// editors (such as a search field in a custom user interface element), register
// them for observation via `atom.textEditors.add`.

This requires that a package opt into this behavior when adding their TextEditor instances to the page, which feels like the right trade-off. The package author knows whether it makes sense for a specific text field they created to have the functionality of a text editor or just a simple text input box.

That said: you, as a Pulsar user, have the right to ignore the author's intent and write a package that forces all atom-text-editor elements to be added to the registry. My goal with the above code snippet was to give you a way to do that in a just-in-time fashion without having to wait for a keystroke within the editor.

I'll think about this some more and maybe do some experiments to see how the editing experience varies before and after you add an editor to the registry.

@asiloisad
Copy link
Contributor Author

I will provide few example how new API can be utilized:

  1. community package can introduce text converting in search field of find-and-replace package, like work-map

  2. community package can make smooth scroll to github commit detail view, like smooth-scroll

  3. multi cursor can operate in mini text editors, like multi-cursor-plus or super-cursor

  4. overtype mode can be used in mini text editor per global setting, like atom-overtype-mode or overtype-mode.

  5. extended method of selecting can be used in mini text editors, like super-select

  6. deatached text-editors can be easy get by clear solution.

My goal is introduce an API which allow you to get and to observe all text editors inside Pulsar. It doesn't necessarily have to be the solution I wrote, but I think it's a good thing because finally an experimental feature will have some use.

Another way could be to expose your function in the official API with disposable in return, but it will require a bit of additional code to achive get and observe methods.

@asiloisad
Copy link
Contributor Author

document.addEventListener('focusin', () => {
  if (document.activeElement.matches(`atom-text-editor[mini]`) {
    // do something
  }
});

This method does not solve a task when I need to patch all text editors (inc. mini) after it was created. It's a case of overtype-mode, because I need to apply a patch to deal with new class list overtype-cursor to apply CSS when needed 🙁.

@savetheclocktower
Copy link
Contributor

savetheclocktower commented Jan 11, 2025

document.addEventListener('focusin', () => {
  if (document.activeElement.matches(`atom-text-editor[mini]`) {
    // do something
  }
});

This method does not solve a task when I need to patch all text editors (inc. mini) after it was created. It's a case of overtype-mode, because I need to apply a patch to deal with new class list overtype-cursor to apply CSS when needed 🙁.

document.querySelectorAll('atom-text-editor')?

@asiloisad
Copy link
Contributor Author

It's like a getTextEditors, but I need something like observeTextEditors. A patch must be applied after text editor is created, even if user do not focus it yet.

@savetheclocktower
Copy link
Contributor

It's like a getTextEditors, but I need something like observeTextEditors. A patch must be applied after text editor is created, even if user do not focus it yet.

@asiloisad! I have given you methods to

  • detect when new TextEditors are on the page in advance of the user typing text into them;
  • grab all TextEditors on the page at any given moment

which should suffice for any strategy you have to force all TextEditors to be in TextEditorRegistry against the package authors' wishes.

In addition, TextEditorRegistry already has ::observe — which functions exactly like atom.workspace.observeTextEditors for editors that are already in the registry.

Until I better understand the difference in behavior that goes with adding something to TextEditorRegistry, I do not think it is a good idea to automatically add something to TextEditorRegistry before it exists on the page, much less without asking the creator of the TextEditor.

In the meantime, you have what you need in order to write a package that puts all TextEditors into the registry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants