apache / netbeans

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

Move the Recent Projects and Recent Files dashboard widgets into the respective modules. #7566

Closed neilcsmith-net closed 1 month ago

neilcsmith-net commented 1 month ago

Dashboard updates.

Move the Recent Projects and Recent Files dashboard widgets into the respective modules. Remove the need for friend access from IDE dashboard and allow platform applications to use these widgets in dashboard configurations.

A second commit removes the now unused Welcome folder.

neilcsmith-net commented 1 month ago

I'd meant to remove the friend reference. I've left the test references for now - there are references to no longer existing modules in various test related files. I don't think there's any that have to be removed, given this module has been excluded from the build for some time, whereas removing the references might have an affect on some tests. Exclusions I've kept for now.

mbien commented 1 month ago

awesome! each removed friend dependency is a small victory for maintainability IMO.

neilcsmith-net commented 1 month ago

Thanks @mbien Let's just double check my assertion that removing that code has no effect on tests :smile: Then I'll merge.

mbien commented 1 month ago

good that you checked since one vscode-ext step seems to be failing.

To safe time, this is a false positive and happens when green too:

    [junit] SEVERE: No way to find original stream handler for jar protocol
    [junit] java.lang.reflect.InaccessibleObjectException: Unable to make field transient java.net.URLStreamHandler java.net.URL.handler accessible: module java.base does not "opens java.net" to unnamed module @73db4768

and this too

     [junit] Testcase: org.netbeans.modules.nbcode.integration.VerifyPresentUpdateCentersTest:BeforeFirstTest:  Caused an ERROR
    [junit] Forked Java VM exited abnormally. Please note the time in the report does not reflect the time until the VM exit.
    [junit] junit.framework.AssertionFailedError: Forked Java VM exited abnormally. Please note the time in the report does not reflect the time until the VM exit.
    [junit]     at jdk.internal.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
    [junit]     at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    [junit]     at java.base/java.util.Vector.forEach(Vector.java:1365)

and this is apparently also normal

     [exec] [3043:0713/214404.710157:ERROR:bus.cc(407)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")

this however seems to be new

      [exec] WARNING [org.netbeans.modules.java.lsp.server.protocol.Server]: Error occurred during LSP message dispatch
     [exec] java.lang.NullPointerException
     [exec]     at java.base/java.util.Objects.requireNonNull(Objects.java:209)
     [exec]     at java.base/java.util.Arrays$ArrayList.<init>(Arrays.java:4137)
     [exec]     at java.base/java.util.Arrays.asList(Arrays.java:4122)
     [exec]     at org.netbeans.modules.java.lsp.server.protocol.WorkspaceServiceImpl.lambda$executeCommand$14(WorkspaceServiceImpl.java:434)

but lets start it again to see what happens. cc @sdedic

mbien commented 1 month ago

third retry didn't improve the situation. I started a build in my fork to check if this also occurs on master: https://github.com/mbien/netbeans/actions/runs/9930024667

sdedic commented 1 month ago

Not checking thoroughly yet - the NPE is from a listener on opened projects, which SHOULD report a Project[0] not null in its events. But null is usually a valid old/new property value on a PCE; so the lsp server could get a conservative bugfix anyway.

mbien commented 1 month ago

the build on my fork is green. So this PR is somehow influencing the vscode step? Or are we just unlucky in rolling dice today...

sdedic commented 1 month ago

Seems that:

Warning - could not install some modules:
        org.netbeans.api.dashboard - No module providing the capability org.openide.windows.WindowManager could be found.
...
        org.netbeans.modules.projectui - The module org.netbeans.api.dashboard would also need to be installed.

This is because of https://github.com/apache/netbeans/pull/7566/files#diff-8f03d1d646c1a04005bdb542a481593908de9e4318115934696f93ec9907215cR37 hard dependency that was not there originally.

neilcsmith-net commented 1 month ago

I wondered if it could be that (wrong project trampoline in trace?)

~So what are the implications of adding dashboard's dependencies to VSCode plugin? This was potentially missed in review of that API.~

neilcsmith-net commented 1 month ago

Actually, looking at this afresh on my computer rather than trying to make sense of the test trace on my phone, I realised the dependency on the Window API is there. The problem seems to be the lack of a window manager token. I'm pretty sure this isn't required, but was added by the APISupport TopComponent wizard and I hadn't realised.

See https://github.com/apache/netbeans/blob/master/apisupport/apisupport.wizards/src/org/netbeans/modules/apisupport/project/ui/wizard/winsys/NewTCIterator.java#L321

Whereas the window API arch docs say "You might use OpenIDE-Module-Requires: org.openide.windows.WindowManager but it is not generally done. "

Longer term we could if necessary move the default (Swing) dashboard displayer into a separate module. But hopefully this fixes the immediate issue. @sdedic the API itself doesn't rely on Swing, aside from actions, so an alternative displayer could be used in the VSCode plugin in future, should there be interest in exposing any widgets added.

sdedic commented 1 month ago

@neilcsmith-net Maybe dashboard could OpenIDE-Module-Recommends the WindowManager token. On applications where available the relevant module (and therefore UI representation) will load.

neilcsmith-net commented 1 month ago

@sdedic thanks. I didn't add that because I couldn't think of a situation whereby the window API does not effectively do that anyway?! We do have two instances of that recommends in the code base, but not in core platform things that depend on openide.windows though.

neilcsmith-net commented 1 month ago

Looks like all tests OK now. Will merge tomorrow unless any further concerns?