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

[NETBEANS-1965] Export public & friend packages for Payara Plugin #1091

Closed
wants to merge 2 commits into from
Closed

Conversation

jGauravGupta
Copy link
Contributor

No description provided.

@geertjanw
Copy link
Member

Makes sense. Thanks for working on making Apache NetBeans and Payara work together. If no one objects within 24 hours, I will merge this.

@geertjanw
Copy link
Member

Why does org.netbeans.modules.openide.windows need to be public?

@jGauravGupta
Copy link
Contributor Author

Payara Micro plugin's context service provider supersedes the org.netbeans.modules.openide.windows.GlobalActionContextImpl.

@geertjanw
Copy link
Member

Only for Windows, not for other operating systems? And I suppose you're using 'supersedes' in the annotation and you need to expose this module because you need to depend on it for ordering purposes? It sounds a little bit suspicious.

@geertjanw
Copy link
Member

Essentially, while I agree completely with the aim behind this PR, we're creating a bunch of new APIs here -- what's the process we should follow for that?

@jGauravGupta
Copy link
Contributor Author

Not specific to operating systems, Payara Micro context provider supersedes to make current project in selection regardless of the current TopComponent.
For more info : http://wiki.netbeans.org/DevFaqAddGlobalContext

@geertjanw
Copy link
Member

But won’t that supersede the default context provider and thereby change the context completely throughout the IDE for all features of the IDE?

@geertjanw
Copy link
Member

A different approach to consider would be to donate the Payara plugin source code to Apache NetBeans, i.e., to have it in the trunk of Apache NetBeans and to develop it there. It would be a fantastic contribution to NetBeans, especially since the next release, targeted to be released in March, will focus on the enterprise, i.e., Jakarta EE cluster.

@jGauravGupta
Copy link
Contributor Author

Exactly, we (Payara Team members) already had discussion about it last week.

@geertjanw
Copy link
Member

That would mean you would need to make none of (or less of) these friend requests and you would not need to expose any (or several) of these APIs. Instead, you'd work with the Payara source code as a standard part of Apache NetBeans.

@jGauravGupta
Copy link
Contributor Author

Regarding Payara plugin donation, we had just discussion, so officially not approved yet. I hope so once approved by both party, It will start after next Payara Server release.

That would mean you would need to make none of (or less of) these friend requests

I understand that most of APIs don't need to be expose for internal modules. But can't find much difference for friend request, As friend requests are also added for GlassFish modules.

@geertjanw
Copy link
Member

Yes, that's true, and you're right. Looking forward to this a lot.

@jlahoda
Copy link
Contributor

jlahoda commented Jan 21, 2019

-1 on the current patch, sorry.

I think adding an (external) module as a friend to modules that already have an external friend is not very controversial.

What is controversial is opening new packages. I think that in all cases, it is necessary to start with a description why a particular package is opened.

The most controversial are cases where a package is opened publicly. Some cases (like opening org.netbeans.modules.openide.windows), seem to be simply undesirable, and any usecase which needed that should be implemented as a proper API enhancement.

Specifically on the org.netbeans.modules.openide.windows package: overriding GlobalActionContextImpl will affect the whole IDE, I believe, so some care needs to be taken. But I don't see why the package must be public. ServiceProvider allows to supersede (mask out) other implementation given their name, no need to make the package public. So what is exactly the usecase?

Thanks.

Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless my voice is silenced by others who support this change, I am against this PR and I am going to close it.

@@ -538,7 +538,11 @@
</test-dependency>
</test-type>
</test-dependencies>
<public-packages/>
<public-packages>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be done without justification (e.g. without reviewing the code that needs that).

<package>org.netbeans.modules.j2ee.sun.ddloaders.multiview.ejb</package>
<package>org.netbeans.modules.j2ee.sun.ddloaders.multiview.jms</package>
<package>org.netbeans.modules.j2ee.sun.ddloaders.multiview.web</package>
<package>org.netbeans.modules.j2ee.sun.ddloaders.multiview.webservice</package>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very suspicious. Why nobody needed these packages yet? There is something fishy in the code you write.

@@ -141,8 +143,48 @@
<package>org.netbeans.modules.j2ee.sun.dd.api.serverresources</package>
<package>org.netbeans.modules.j2ee.sun.dd.api.services</package>
<package>org.netbeans.modules.j2ee.sun.dd.api.web</package>
<package>org.netbeans.modules.j2ee.sun.dd.impl</package>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 on these. There is a reason it is called impl.

@JaroslavTulach
Copy link

-10

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.

None yet

4 participants