After file external change fire reloaded event

[errael]

This PR adds, PROP_RELOADING, to signal begin/end of document reloading due to an external change.

This change is about handling fixup after a document is reloaded. Currently, openide.text adjusts any document positions such that after the reload the PositionRefs have the same line number and column offset. However, the caret positions in any editors are set to have the same offset as before; and so the caret often seems to jump around after the reload. Maintaining line/col seems preferable. This was discussed in the dev mailing list thread:

    Detecting file changed externally, 2022, Jun 7
    https://lists.apache.org/thread/tb1mkx1koctt74y48gbyqhqsbtrq56ys

The editors now support multiple carets and there may be other fixups after a document reload in addition to caret position; for example, repositioning the viewport.

With the PROP_RELOADING events things can be handled as needed where needed; an editor kit might use this for caret position fixup.

Using these events, a caret's line/col can be preserved after reload like this:

1. PROP_RELOADING: evt.getNewValue() == true Save PositionRef for each caret
2. PROP_RELOADING: evt.getNewValue() == false Restore caret position from PositionRef saved in 1.

The PROP_RELOADING event is fired on the EDT. This is probably the right choice in general for these events; in this situation it is required to insure that the end event comes after DocumentOpenClose adjusts the carets and after it gets the CloneableEditorSupport ready for prime time.

Upated June 21 to reflect change to two events with PROP_RELOADING, instead of starting with fileChange and ending with PROP_RELOADED.

[errael]

@lkishalmi since you brought up the caret position issue, is there anything you'd like to add?

[lkishalmi]

@errael all I did is just to point to some source files.

[errael]

BTW, I tried PROP_RELOADED in an editor plugin to restore the caret position after a reload. The cursor behavior after reload is much improved.

[errael]

@ralphbenjamin, @sdedic With your knowledge of EditorCaret API, any comment on using PROP_RELOADED to restore multiple caret positions after a reload?

[errael]

all I did is just to point to some source files.

@lkishalmi I'm curious about what you meant:

try to improve the caret persistence situation

maybe this is about persistence between sessions and not reload?

[sdedic]

Is it guaranteed that the (1) file listener invocation in editor module and (2) actual document reload by openide.text happen always in the correct order (1), (2) ?

[errael]

Is it guaranteed that ... happen always in the correct order (1), (2) ?

@sdedic The reload path is

Wathcher thread: detects external change, FileObject.refresh, fires events
    CES does Mutex.EVENT.postReadRequest
(1) EDT: Starts reload in EDT, captures caret positions, task.schedule(0)
(2) RP thread: handles the reload, including **reading the file**,
        when finished does
(3)         Mutex.EVENT.postReadRequest: restore carets
(4)         Mutex.EVENT.postReadRequest: fire `PROP_RELOADED`

The path to the editor module is

Wathcher thread: detects external change, FileObject.refresh, fires events
    editor module does Mutex.EVENT.postReadRequest
(1) EDT: capture the carets

At least 4 thread changes versus 1. It's conceivable that the Watcher thread would not finish firing events (and that the CES event was fired before the editor event) before the document reload finishes. In that case the caret would be set to it's previous save point. If this is a concern, then a caret listener could save a caretPosition after every move. Is that overkill to prevent a rare glitch in caret position?

A PROP_START_RELOAD event could be fired from the EDT, in (1) above, this feels cleaner. Should this be included in this PR?

Two things to keep in mind

1. The external file change is truly asyncronous, no guarentees.
2. The file is modified, any setting of positions is a guess.

[errael]

Changed the event to PROP_RELOADING. Fire two events, start and end.

evt.getNewValue() == true indicates that a reloading is starting, evt.getNewValue() == false means that reloading is finished. This is more clear, requires only one listener in editor module and removes any potential race condition.

[sdedic]

Q: the PR description seems to mention changes in EditorLib2, but the only modified files are from openide.text ?

[errael]

Q: the PR description seems to mention changes in EditorLib2, but the only modified files are from openide.text ?

I mentioned it in the context of needing to deal with multiple carets. I tweaked the wording.

[errael]

@sdedic @dbalek @ralphbenjamin any comments/suggestions?

[sdedic]

I'm fine, although I'd prefer also a feature that uses the new API ... usually makes a good proof of API's suitability for the envisioned scenarios.

[errael]

also a feature that uses the new API

Yeah, not having that seems a little weird to me as well. I forget how/where, can't the usage of the property be marked as experimental/not-stable or something like that?

I'm currently running with this used by the jVi plugin, it adjust the caret and reposition the viewport after reload. But that doesn't qualify.

The issue is how to best use this. I'm thinking there might be considerable discussion around it. Wanted to get this in or visible so it could be tried out. I'll post something to dev, and see if there are any comments.

I was thinking of opening a draft PR on EditorCaret that uses this to adjust caret position after reload, and respect an IGNORE_RELOAD text component client property. Then there'll be something to kick around. But getting it ready for release could be a major task; and this might not be the best place to put it, editor kits might be a better place; or ....

Probably the biggest problem doing this is that jVi extends BaseCaret/ExtCaret (and the vim block mode is full featured in NetBeans) and I don't even know how to create multiple carets in the default editor.

[errael]

@neilcsmith-net Any chance to sneak this in?

[neilcsmith-net]

@errael no idea! Someone needs to make a case for it going in to delivery and rebase the PR. Sorry, I haven't looked at it at all as yet, so not in a position to make that call.

[sdedic]

... or we can use the extended time till NB16 to actually base some useful application on this added feature :) ?

[sdedic]

@errael time to merge before next freeze !

[neilcsmith-net]

@sdedic agreed! I was just looking at this. I assume that VSCode failure on Travis isn't an issue? Retriggered it once - not looked in depth at why it failed again.

[errael]

@sdedic agreed! I was just looking at this. I assume that VSCode failure on Travis isn't an issue? Retriggered it once - not looked in depth at why it failed again.

Could it have something to do with the PR being so old?

The two failures seem to be arising from:

/home/travis/build/apache/netbeans/nbbuild/netbeans/harness/build.xml:150:
No dependent module org.netbeans.modules.cloud.oracle

coming from two

The command "(cd java/java.lsp.server; ant build-vscode-ext ...

[neilcsmith-net]

@errael in theory the CI is meant to run against refs/pull/<ID>/merge which is a theoretical merge to master at the time it runs. Retriggering, not age of PR, should be the relevant bit. But who knows with Travis?! :shrug:

I'm going to merge - let's see what comes out in the wash.