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-1342] Hide the panel for iOS in the Mobile Platforms Options #953

Merged
merged 3 commits into from
Oct 28, 2018

Conversation

junichi11
Copy link
Member

@junichi11 junichi11 commented Oct 6, 2018

https://issues.apache.org/jira/browse/NETBEANS-1342

Just hide the panel because the cordova.platforms.ios module is not available (see #856)

netbeans-1342-options-panel-after

Just hide the panel because the cordova.platforms.ios module is not available (see apache#856)
@junichi11 junichi11 added this to the 10.0 milestone Oct 6, 2018
@junichi11 junichi11 added the Cherry Pick Consider cherry picking for release update label Oct 6, 2018
@Chris2011
Copy link
Contributor

But when we hide it, we need to hide the cordova option inside of the HTML5/JS project too and to hide the cordova project template too.

@junichi11
Copy link
Member Author

I'm not sure about cordova but I think we should remove cordova modules from the cluster if we will not support cordova yet.

Copy link
Member

@sdedic sdedic left a comment

Choose a reason for hiding this comment

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

There are also some IOS-related factories in CordovaBrowserFactory shouldn't they move to ios support module ? CordovaPanel source defines controls related to IOS, but I can't find a place where the panel is added to a visible parent - is it a dead code ?

Re. remove: I've removed the ios module from the cluster because of license issues, but there's also android support, which seemed to be license-OK.

Just check whether PlatformManager.getPlatform(PlatformManager.IOS_TYPE) is null
@junichi11
Copy link
Member Author

junichi11 commented Oct 8, 2018

@sdedic

There are also some IOS-related factories in CordovaBrowserFactory shouldn't they move to ios support module ?

Maybe, we should do so. I'm not an expert this area. So if you can do that, I'll leave it to you as another problem :) I just wanted to fix NETBEANS-1342 (NPE).

Re. remove: I've removed the ios module from the cluster because of license issues, but there's also android support, which seemed to be license-OK.

Yes, I know it. I thought so. But I thought need not to add the cordova modules if we need to hide cordova options and project templates (Chris wrote above).

@junichi11
Copy link
Member Author

CordovaPanel source defines controls related to IOS, but I can't find a place where the panel is added to a visible parent - is it a dead code ?

I'm not sure what this means, sorry... CordovaPanel is MobilePlatformsPanel?
Here?
https://github.com/apache/incubator-netbeans/blob/d428744a920fbea150be723a0c2774e384d9c334/webcommon/cordova/src/org/netbeans/modules/cordova/options/MobilePlatformsPanel.java#L81

Copy link
Member

@geertjanw geertjanw left a comment

Choose a reason for hiding this comment

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

Great. Ideally, we'd also find time to figure out why the Cordova support, specifically that panel, does not work in Apache NetBeans 10 vs. 8.2 where it worked. But the question can also be asked how many were using this feature in the first place, I guess by excluding this functionality, we'll soon find out. :-) Ah, I see: #856

@Chris2011
Copy link
Contributor

Excluding is, in my opinion not an option as long as we don't have a good working NBAndroid Plugin, where it can live there. Hiding for the first thing is good, the rest shouldn't that hard to figure it out. I would prefer adding more feature instead of removing, otherwise, the users will find/use another IDE. IMHO. But of course this should be discussed at the mailing list.

@junichi11
Copy link
Member Author

@sdedic I'm not sure about that because I'm not the author, but I could also not find it...
It seems that just setVisible() method is called.
https://github.com/apache/incubator-netbeans/blob/4daaf04bee3a602a6a93a35422190e4ceec0bc0e/webcommon/cordova/src/org/netbeans/modules/cordova/project/CordovaPanel.java#L71-L74

Just in case, I'll hide the iOSPanel.

@junichi11
Copy link
Member Author

If there is no problem, let's squash and merge. At least, this change prevents NPE, I think.

@geertjanw
Copy link
Member

Cool, go ahead.

@junichi11 junichi11 merged commit a6ed963 into apache:master Oct 28, 2018
@junichi11 junichi11 deleted the netbeans-1342 branch October 28, 2018 12:48
junichi11 added a commit to junichi11/netbeans that referenced this pull request Oct 28, 2018
apache#953)

* [NETBEANS-1342] Hide the panel for iOS in the Mobile Platforms Options

Just hide the panel because the cordova.platforms.ios module is not available (see apache#856)

* Prevent NPE

Just check whether PlatformManager.getPlatform(PlatformManager.IOS_TYPE) is null

* Hide iOSPanel in the CordovaPanel
@junichi11 junichi11 removed the Cherry Pick Consider cherry picking for release update label Oct 28, 2018
lkishalmi pushed a commit that referenced this pull request Oct 29, 2018
#953)

* [NETBEANS-1342] Hide the panel for iOS in the Mobile Platforms Options

Just hide the panel because the cordova.platforms.ios module is not available (see #856)

* Prevent NPE

Just check whether PlatformManager.getPlatform(PlatformManager.IOS_TYPE) is null

* Hide iOSPanel in the CordovaPanel
lkishalmi pushed a commit to lkishalmi/netbeans that referenced this pull request Dec 29, 2018
apache#953)

* [NETBEANS-1342] Hide the panel for iOS in the Mobile Platforms Options

Just hide the panel because the cordova.platforms.ios module is not available (see apache#856)

* Prevent NPE

Just check whether PlatformManager.getPlatform(PlatformManager.IOS_TYPE) is null

* Hide iOSPanel in the CordovaPanel
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.

4 participants