apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.62k stars 840 forks source link

Update NbEventSpy for maven 4. #7529

Closed mbien closed 2 months ago

mbien commented 2 months ago

@cstamas will this work or can the session change the container later? In my tests the container of the context and the session were always the same instance.

cstamas commented 2 months ago

That's right: this is the correct change

mbien commented 2 months ago

@cstamas awesome! thanks for checking.

cstamas commented 2 months ago

Ideally Plexus should be injected, but this seems not as a component?

cstamas commented 2 months ago

Oh, it is a plexus component: https://github.com/apache/netbeans/blob/master/java/maven/mavensrc/org/netbeans/modules/maven/event/components.xml

cstamas commented 2 months ago

Ideally you want to get rid of Plexus XML files, especially those manually authored

mbien commented 2 months ago

done

i tested the event spy and it worked fine, but i have no clue what the other thingy does. Usage is here: https://github.com/apache/netbeans/blob/0253a5dbb57e507aaae21e11443101f0fd08b971/java/maven/src/org/netbeans/modules/maven/cos/CosChecker.java#L555

mbien commented 2 months ago

ah I get it. COS stands for compile on save. As far as I see IDEWorkspaceReader1/2 classes are used to log changes, probably for debugging purposes, since I don't see anywhere attempts to parse the log lines on the other side (which is different to the event spy). So this should be fine. (c~urious that both files are identical~ one hooks into org.sonatype...Reader the other into org.eclipse...Reader)

I played with cos a bit and it isn't working properly in NB 22. I had "apply code changes on save" checked in the debugger settings and it didn't work. With cos disabled it was fine. So this PR should not affect anything there. I personally don't use this feature since I don't like when IDEs mess with the build.

edit: my test setup: this is my test setup:

public class Mavenproject1 {
    public static void main(String[] args) {
        while (true) {
            foo();  // <- set break point
        }
    }
    private static void foo() {
        System.out.println("Hello");  // change text while debuggin
    }
}

with cos disabled and by manually hitting the "apply code changes" button everything worked fine. With cos enabled the apply code changes button became a no-op and saving did nothing. This is NB 22, without this PR applied.

edit2: ok, it is working, however, you have to wait a minute after you hit save. It appears to work once the indexing task ran?

INFO [org.netbeans.ui.indexing]: Indexing started, time from last indexing 12,244 ms.
INFO [org.netbeans.ui.indexing]: Indexing finished, indexing took 222 ms.

once you see this in the NB log, you know cos swapped out the code. It can take quite long.

neilcsmith-net commented 2 months ago

ah I get it. COS stands for compile on save.

Ah, I hadn't realised that the meaning of COS there was your stumbling point or I'd have mentioned it. 😄 Hopefully can be separated out from this change, and possibly a discussion to be had elsewhere on whether this slowly deteriorating feature should just be removed?!

mbien commented 2 months ago

Hopefully can be separated out from this change,

What do you mean by that? All this does is to switch from component.xml to annotations. I think it would be more confusing/error prone when IDEWorkspaceReader1/2 would keep using component.xml while the NbEventSpy is using annotations and both are built in the same module.

neilcsmith-net commented 2 months ago

Sorry, badly worded. I just meant separating out the discussion on the other points you raised - whether it's actually working correctly, whether it needs fixing now or for Maven 4. As you said, this PR should not affect things there (or make it any worse!).

mbien commented 2 months ago

@neilcsmith-net awesome. Yep agreed. Btw I updated https://github.com/apache/netbeans/pull/7529#issuecomment-2198498169 with a test case since I discovered that cos does work, just extremely delayed.