This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
API request Unless there's a better option, request is to add an inner class[1] to WebLogicalViewProvider to support adding a Project action and badging for visualweb projects when database connections used by the project are not available. The feature requested is similar feature as the BrokenServerSupport in org.netbeans.modules.j2ee.common.ui This enhancement is needed for the following reasoning: If the database connections are not available then the project cannot be developed at design-time or executed. At design-time a Component Error page will open instead of the expected page that has database-bound components. The alternative is to badge the Data Source Reference node of a project. However, this is a usability issue in that a user is not informed adequately that the project has a problem and also to maintain consistency with the broken server feature. Target milestone is M10 Dependencies Described in these specifications, under Usability review: http://wiki.netbeans.org/wiki/view/VWUserSettingsMigrationSpec http://www.netbeans.org/issues/show_bug.cgi?id=99740 http://wiki.netbeans.org/wiki/view/ResolveBrokenDataSourcesSpec http://www.netbeans.org/issues/show_bug.cgi?id=93949 The module that needs access to APIs is org.netbeans.modules.visualweb.dataconnectivity In visualweb 5.5. we had our own copy of WebLogicalViewProvider to support this. Now that project ui has merged for Web Applications, visualweb no longer has access to WebLogicalViewProvider In visualweb's previous version, an inner class was added to support checking the project to see if database connections were missing (BrokenDatasourceAction) [1] In order to support the feature I'm requesting in 6.0 proposoal is to include the following inner class in org.netbeans.modules.web.project.ui.WebLogicalViewProvider private class BrokenDatabaseAction extends AbstractAction implements PropertyChangeListener, Runnable { private RequestProcessor.Task task = null; private boolean brokenDatabase; public BrokenDatabaseAction() { putValue(Action.NAME, NbBundle.getMessage(RaveLogicalViewProvider.class, "LBL_Fix_Broken_Database_Action")); // NOI18N evaluator.addPropertyChangeListener(this); checkMissingDatabase(); } public boolean isEnabled() { return brokenDatabase; } public void actionPerformed(ActionEvent e) { String instance = BrokenDataSourceSupport.selectServer(project); run(); } public void propertyChange(PropertyChangeEvent evt) { checkMissingDatabase(); } private void checkMissingDatabase() { if (task == null) { task = BROKEN_DATABASE_RP.create(this); } javax.swing.SwingUtilities.invokeLater(new Runnable () { public void run() { task.schedule(100); } }); } public synchronized void run() { boolean old = brokenDatabase; brokenDatabase = BrokenDataSourceSupport.isBroken(project); if (old != brokenDatabase) { fireIconChange(); fireOpenedIconChange(); fireDisplayNameChange(null, null); } } } // </RAVE> }
changing component name to web/projects
Since there are dependencies on this request, raising to P1
Who is going to do the change? Could you attach a diff file?
Created attachment 43081 [details] Diff of proposed changes to WebLogicalViewProvider.java
If change approved and diffs are ok then I can make the changes
Created attachment 43082 [details] project metadata change to include Data Sources
Note, to check for a missing database connection may take a few seconds - the reason for running in a separate thread From the UI review, reviewers prefer , when a database connection is missing for the project, a dialog pops up, as it does for the missing server. The problem is if the project is missing both server and database connection then 2 dialogs opening, one on top of the other would happen. To solve this, I should check for a missing server before posting the dialog.
Please disregard the first diff of WebLogicalViewProvider The next one is more up-to-date Since checking for a missing server occurs before checking for missing database connections, there shouldn't be an issue with 2 dialogs opening.
Created attachment 43084 [details] the Diff of proposed changes to WebLogicalViewProvider.java
The remaining implementation, dialogs and checking will be done in org.netbeans.modules.visualweb.dataconnectivity.ui I'll also need to make org.netbeans.modules.visualweb.dataconnectivity.ui a public package Note the web project's project meta data includes org.netbeans.modules.visualweb.dataconnectivity (Data Source) as a library reference
added url to UI spec
AB01: this will cause the j2ee cluster to depend on the visualweb cluster. OTOH visualweb already depends on j2ee. I don't think circular dependencies between clusters are supported, and even if they were, j2ee should not depend on visualweb. AB02: since we use data sources in all Java EE project types, such a check should probably be made in all project types, not only the visual web one. AB03: re. "to check for a missing database connection may take a few seconds - the reason for running in a separate thread": I see the check ultimately takes place in the event thread because of the SW.invokeLater() call in run(). Why? AB04: there are data races (missing synchronization) all over the code accessing both "task" and "brokenDatabase". AB05: do you really need to do a check whenever any property changes? And OTOH, are you listening on anything that may affect the database connection availability (e.g., don't you also need to listen on ConnectionManager?). Hard to tell what you need to listen to since BrokenDatabaseSupport is not attached and I can't find it in visualweb/dataconnectivity. AB06: I'm missing code to do the actual badging when data sources are broken. Shouldn't you also modify WebLogicalViewRootNode.getMyIcon() and getMyOpenedIcon() to call brokenDatabaseAction.isEnabled()? AB07: Please send diffs in the unified format (cvs diff -u).
My comments are similar to Andrei's comments. PP01: j2ee cluster CAN NOT depends on the visual cluster. As Andrei wrote, if you introduce the dependency, then you create a circular dependency. It means that you abort the separation j2ee from visual web pack. Is not possible to ship NetBeans without visual web cluster then. PP02: > Note the web project's project meta data includes > org.netbeans.modules.visualweb.dataconnectivity (Data Source) as a library > reference Is it possible to separate the functionality outside visual cluster. If yes, then it can be put in the j2ee cluster. PP03: > I'll also need to make org.netbeans.modules.visualweb.dataconnectivity.ui > a public package If you want to have a public package, you should go through regular api review. It's really necessary? As I wrote, you cannot use this api-to-be in j2ee cluster. Who will be the clients of such API? PP04: The diff doesn't contains all changes. You should attach diff of all changed files. Then I can apply the diff and try it. Now I can't. I agree with Andrei's all comments as well. I'm against to commit this to the cvs in the current state.
PP05: What is the api change, which we should to review? The package org.netbeans.modules.web.project.ui is not a part of any API or SPI. So there isn't any api change in the j2ee cluster or at least in the attached diff file. -> I'm removing the API and API_REVIEW_FAST keywords.
Andrei's comment that we need to do this for all projects using database connections is a good point. Also, the fact that there a number of different scenarios where database connections could get broken is a good point. I'd like to see some ideas/suggestions on how this could be done differently to support all projects using connections, to eliminate the circular dependency, and to handle all the various situations where a database connection can become invalid. Perhaps we should call a meeting to more effectively work through all these issues.
JB01: re. AB01, web project adds a reference to j2ee for the UIs , dataconnectivity was following this pattern. Instead, I think it would be better for the j2ee and dataconnectivity UIs to live in web project. This would eliminate one dependency. JB02: re. AB02 , this is a very good comment. I haven't seen feedback from users who have Web Applications with data sources and complained about missing connections. I may have been biased towards visualweb, but I have seen feedback, frequently about broken projects because connections had been removed JB03: re. AB03 . For pre-6.0 visualweb projects, the data sources were not stored in the project. As a result, a project must be modeled (insync loads the class and checks for references to data sources). This modeling takes longer depending on the size of the project. For 6.0 projects, no modeling is needed so there should not be much of a delay. When opening a project, dataconnectivity can check the version of the project to see if it needs to be modeled or not. JB04: re. AB04, basic testing didn't detect synchronization problems, but I could synchronize checkMissingDatabase(). in BrokenDatabaseSupport(to be attached), showAlert is synchronized. JB05: re. AB05 the property listener may be needed for pre-6.0 projects. I'll look into this. I'll attach BrokenDatabaseSupport JB06: re. AB06 good catch, I'll attach this also (previously I had implemented this at the dataconnectivity node)
Created attachment 43134 [details] BrokenDatabaseSupport
btw, j2ee utilities exposes ui as a public package dataconnectivity was following this pattern.
Since the request is to add dependency of enterprise cluster on visualweb it *is* API change. Personally -1 from for the same reason as AB01/PP01 BrokenDatabaseSupport.boolean brokenAlertShown is only assigned to and the value is never read. AB04 about data races is correct and I do not accept JB04 as an answer (BTW: http://www.azulsystems.com/events/javaone_2007/2007_DebugDataRace.pdf is worth reading). It seems that we need another solution. Minor comment: please use unified or context diff format for your patches (diff -u or diff -c).
J2ee is in the same cluster as web project. So there is not the problem with dependency. But you introduce cyclic dependency between j2ee cluster and viasual web cluster, which is problem.
re-implementation has been completed, except for clarifications/agreements on items below and also will have the code reviewed tomorrow prior to adding new diffs. JB07 - If acceptable, to prevent the circular dependency on visualweb.dataconnectivity, I would like to use org.netbeans.modules.j2ee.common.ui for resolving database connections for 6.0, that will contain dialogs and BrokenDatabaseSupport classes, similar to missing server support. If need be, this can be refactored in a future release. (note, no more dependency on visualweb.dataconnectivity) JB08 - Only visualweb Web Projects will be supported for this feature in 6.0 Support for Web Projects and other projects that use database connections can be added in a future release JB09 - concern about BrokenServerSupport has a few static methods, doesn't seem like a good design. Instead, I prefer not to use static methods in BrokenDatabase support. If this is not an issue to use static methods, then same design as BrokenServerSupport as BrokenDatabaseSupport
Re JB07: I think it's acceptable. But it should be done correctly, that we don't have to refactored in a future. It will an api so it should be done properly. Re JB08: What J2SE projects? Can we solve it in the same way? How is it solve with data banding in Matisse? Please attach complete diff, when it will be available.
Re Re JB 08 if implementation for resolving data sources resides in j2ee then also supporting J2SE projects wouldn't make sense as these projects do not use J2EE features, correct ? If agree then is there another acceptable package to place the resolving data source code?
JB10 - Additionally, there is a related task that will require an additional check in WebLogicalViewProvider which is checking the project type and version. Unfortunately, the easiest way is a kludge. This would use Lookup API // if project is a Web Application then check to see if the project is // a visualweb project and then check the version for appropriate action. // if the project is not a 6.0 visualweb project then fall through // (for non-6.0 projects, no project badging, no dialog posted, no Project context menu added) if (WebModule.getWebModule(project.getProjectDirectory()) != null) { WebPropertyEvaluator wpe = (WebPropertyEvaluator) project.getLookup().lookup(WebPropertyEvaluator.class); wpe.evaluator().getProperty("*j2ee.platform.is.jsr109*"); ... } Any objection to using WebModule APIs and project.getLookup() ? By using these APIs, I can check for the existance of property keys in private.properties. The mere existance lets me know the version of a visualweb project. visualweb has an API to get the project version but it is not appropriate to use.
You can use WebModule API and lookup, they are api. But I don't understand, why you are asking? The condition if (WebModule.getWebModule(project.getProjectDirectory()) ... doesn't have to work in every case. You relay on location of the project and the web module. There are web modules, which can have project folder in different place than sources, like freeform web projects.
from my previous comment here's the reasoning for checking visualweb/Creator project: // if project is a Web Application then check to see if the project is // a visualweb project and then check the version for appropriate action. // if the project is not a 6.0 visualweb project then fall through // (for non-6.0 projects, no project badging, no dialog posted, no Project context menu added) Since WebLogicalViewProvider is in web, then do I need to check if project is a Web Module? If I still do need to check, please suggest a way to check to see if project is a web project
I will be attaching new diffs and sources. Changes have been made based on feedback and the sources have been tested and working. I have a question though about the Alert panel. How to check if the "Don't show again" checkbox is checked . I see how J2EE checks but it appears a J2EE property value is retrieved ? I'm referring to BrokenServerSupport's showAlert() <snip> if (Boolean.getBoolean("j2eeserver.no.server.instance.check")) { return; } </snip>
Created attachment 43370 [details] revised WebLogicalViewProvider with BrokenDatabaseSupport
Created attachment 43371 [details] WebLogicalViewProvider.java
Created attachment 43372 [details] Bundle.properties from web.project for WebLogicalViewProvider
Created attachment 43373 [details] Bundle.properties from j2ee.utilities for the Broken Datasource dialogs
Created attachment 43374 [details] NoSelectedDatabaseConnectionWarning.java
note, NoSelectedDatabaseConnectionWarning.java and the next few attachments are the UI implemenation for the Broken Datasource support and the proposal is to place these dialogs and implementation in org.netbeans.modules.j2ee.common.ui
Created attachment 43375 [details] org.netbeans.modules.j2ee.common.ui.NoSelectedDatabaseConnectionWarning.form
Created attachment 43376 [details] org.netbeans.modules.j2ee.common.ui.BrokenDatabaseSupport.java
Created attachment 43377 [details] org.netbeans.modules.j2ee.common.ui.DatabaseConnectionResolver.java
Created attachment 43378 [details] org.netbeans.modules.j2ee.common.ui.BrokenDatabaseAlertPanel.java
Created attachment 43379 [details] org.netbeans.modules.j2ee.common.ui.BrokenDatabaseAlertPanel.form
The files just previously attached is the implementation proposed. If there is something doesn't look right or could be done differently, please provide suggestions. To support other projects besides visualweb should be straightforward, no refactoring should be required. This implementation checks to see that j2ee datasources existing in the project have the required database connections registered in the IDE. If connections needed are not available then the Project node is badged, HtmlDisplayName text color changes to red, a new action is added and an Alert dialog will open to instruct the user how to fix the project. When right-clicking on the Project node and choosing Resolve Datasource Problems.. menuitem, a dialog opens that lists the datasources used by the project for which database connections are missing. By selecting a datasource item in the dialog then clicking the Add Connection dialog, the Add Database Connection dialog will open with the database parameters filled in, if available from the datasource. If they are available, all the user has to do is to click OK and the project is resolved and can be used. I'll update the UI spec tomorrow, see URL field here in this issue.
I updated the UI spec based on feedback from nb-usability http://wiki.netbeans.org/wiki/view/ResolveBrokenDataSourcesSpec There are some screenshots to help understand this feature better
Two issues I know that need to be resolved (mentioned earlier) 1) In the Alert dialog, how to set the result of clicking the checkbox so the dialog is not shown again 2) What is the best way to detect the project is a Web Project, or do I even need to since this is in the web/project module 3) Is the code designed well enough to avoid refactoring if/when other types of projects need to use the code (see explanation of this feature in a comment I made just a few minutes ago)
Please next time just attach a diff with the files (e.g., a diff for web/project and another one for j2ee/utilities). It looks better now, but there are many issues, mainly in BrokenDatabaseSupport and related: AB01: it seems you don't need the "WebModule.getWebModule(project.getProjectDirectory()" since you already check for an "http://www.sun.com/creator/ns:creator-data" element in the auxiliary configuration, which can only be in a Creator web project. Moreover, if you look at the type of the project field in WebLogicalViewProvider, you'll notice it is WebProject, so you know it is a web project. AB02: do not create your own threads, use a RequestProcessor instead. Probably simplest to rename BROKEN_LINKS_RP in WebLogicalViewProvider to BROKEN_RP and use it for the data source check too. AB03: synchronized on the run() method is unneeded and misleading, please remove it. AB04: why did you remove the code that avoided sending spurious node change events in doCheckMissingDatabase()? I.e., boolean oldBrokenDatabase = brokenDatabase; brokenDatabase = ... if (oldBrokenDatabase != brokenDatabase) { SwingUtilities.invokeLater(...) } AB05: instead of String version = null; version = auxElement.getAttribute("jsf.project.version"); you can just String version = auxElement.getAttribute("jsf.project.version"); AB06: you probably need to listen on ConnectionManager.getDefault() in the action. What happens if the project opens as broken and the user (without invoking the broken data source dialog) adds a matching database connection in the DB Explorer? AB07: seems a bit strange to call BrokenDatabaseSupport.showAlert() from doCheckMissingDatabase, since that method can run multiple times, but you only want that dialog to pop up for a given project once -- when the project is opened. You could maintain a firstTime flag which you would test each time and set to false the first time you call showAlert(). AB08: I don't understand why everything is named "BrokenDatabase...". It's not database connections that are broken, but data sources. Thus please rename to BrokenDataSource. Same for DatabaseConnectionResolver, it doesn't resolve connections, but data sources, so it should be DataSourceResolver. AB09: I don't understand the reason for the disposeIfEmpty() method. BrokenServerSupport doesn't need one. AB10: you made BrokenDatabaseSupport stateful (because of the brokenDatasources) field, but it needs to be stateless, since it may be called from multiple projects at once. AB11: you have non-API methods in the API. getBrokenDatasource(), getBrokenDatasources() and addDatabaseConnection() are not intended to be called by clients (such as WebLogicalViewProvider), so they should be moved somewhere else. AB12: why did you remove the timeout code avoiding multiple dialogs ("brokenAlertLastTime", etc.) in BrokenDatabaseSupport? AB13: why did you remove the SwingUtilities.invokeLater() call in BrokenDatabaseSupport? The method's Javadoc now incorrectly states it can be called from any thread, but that doesn't seem to be true anymore. AB14: selectDatasource() is a bad name given what the method does. I know there is BrokenServerSupport.selectServer(), but that one makes sense because there is a single server for a project that the user needs to select. Consider something like fixDatasources(). The method should also not return a value (what is the meaning of the "selected database connection" mentioned by the Javadoc of this method anyway?). AB15: typo in "LBL_Resolve_Missing_Dataources_Title". AB16: do not call setShowAgainBrokenServerAlert() in BrokenDatabaseAlertPanel, implement a similar method (e.g., setShowAgainBrokenDataSourceAlert) instead. I guess this also address your question #1 in desc42. AB17: what is connStatus in DatabaseConnectionResolver good for? It seems it can be removed. Same for the brokenDatasources field. Thus you get a stateless DatabaseConnectionResolver and you can make all methods in it static. Please do so, since this class is used from a static context (BrokenDatabaseSupport). You can then remove the databaseConnectionResolver field in BrokenDatabaseSupport. AB18: I think NULL_JDBC_DRIVER_ARRAY does not affect performance in any visible way, but eats up unnecessary memory. Simplest to create an empty array whenever needed (but see AB21). AB19: the datasourceName parameter to DCR.addDatabaseConnection() is not used. AB20: it seems DCR.addDatabaseConnection() is never called with a null brokenDatasource parameter. If that is the case please remove the check for null in the method. AB21: the following code: if (newDrivers.length == 0) newDrivers = (JDBCDriver[]) theDriver.toArray(NULL_JDBC_DRIVER_ARRAY); is unneeded. It says "if newDrivers is an empty array, make newDrivers an empty array". Please remove it, together with the theDriver list. You don't need to check the result of JDBCDriverManager.getDrivers() for null, it never returns null, as mentioned in the Javadoc. The following code: if (!found) { i = 0; return null; } also doesn't need to set i to 0. Actually you can just remove the found flag an have a "JDBCDriver foundDriver" instead and after the for loop which searches the driver you can just "return foundDriver". AB22: the matchString() method is not needed, since it is always used with a false ignoreCase parameter. Consider using org.openide.util.Utilities.compareObjects() instead. AB23: the getNoSelectedDatabaseConnectionWarningDialog() method in NoSelectedDatabaseConnectionWarning does not make any sense. If you need to close the dialog from within the panel, pass the dialog instance to the panel. AB24: refresh() in NSDCW is unused, please remove it. AB25: NSDCW.selectDatabaseDialog() should be renamed similarly to BDS.selectDatasources() and should not return a value. AB26: you have references to NoSelectedServerWarning.class in NSDCW, please remove them. AB27: NoSelectedDatabaseConnectionWarning is too hacked. What is the datasourceList.setVisible(false) call good for? Why is the key "LBL_NoSuitableDatabaseConnectionWarning_jLabel2" named like "no suitable connection" when its text is "select a data source"? Why do you expect "datasourceList.getModel().getSize()" to be zero? If that can happen the dialog should not be displayed at all, should it? Why is the "Add Connection" button named jButtonAddServer? There are many places referring to "server" in this class, please clean them. AB28: the getSelectedInstance() method doesn't make any sense. There should be no OK button in the dialog, just a Close one, always enabled, like in the Resolve Reference dialog. AB29: instead of having a ListModel of String-s (JNDI names of data sources) you could put the data sources in the ListModel and use the ListCellRenderer to display their JNDI names instead of the results of their toString() methods. Then you don't need to retrieve the data source for the JNDI name when the Add Connection button is pressed -- the item will be the data source.
I have just one additional note. Use //NOI18N comment on lines, where is needed like in the comment AB05.
Wow, thanks for spending all this time reviewing the code, Andrei and Petr. Given time constraints, I think it would be helpful for you to identify which issues you believe must be fixed before checkin, and which can be fixed after feature freeze. David
I'm not a big fan of the "let's commit ASAP, we'll fix it later" approach. I understand that it's necessary for big features (e.g., to allow QE to test as soon as possible). However, this approach leads to many bugs, of which some might not even get fixed (we don't fix all P3s for FCS), not even in the next release. There are still six working days until feature freeze and I believe my comments can be addressed by then, especially since I tried to give hints as to what needs to be done. I will try to come up with a list of items that can be deferred, but don't expect many of them.
Point well taken, Andrei. It's a delicate balance between trying to make sure something goes in as high quality but we don't become so picky that we delay the release because of refinements that could be handled during the bugfixing period. Perhaps at least prioritizing the items in some way, so John can make sure he focuses on the right items in the right order, so in case push comes to shove we don't end up delaying the release because of significant issues. David
The suggestions are useful and valid. By giving these explicit suggestions, communication is improved and turnaround time quicker. Although, I agree with much of the latest feedback, including the name choices BrokenDatabase, some of the cleanup(removal) came as a result of some code reviews and a couple of the items mentioned were questions I asked, such as whether I also need to determine if the project is a web module. In basic testing, the code does work. Prioritizing items to address is helpful, but not needed. Most, or all of the these items pointed out can be addressed in time
All feedback AB01-AB29 and the //NOI18N comment have been addressed. I have a couple of questions. Possibly tomorrow I can resubmit diffs (in 2 attachments, 1 for WebProject and the other for J2EE Project Support) Re: AB06 (listening in on ConnectionManager) To do this requires WebProject to have a dependency on Database Explorer. I haven't checked yet if this causes a circular dependency. If not, is it OK to add this dependency. Re: AB11 (non-API methods getBrokenDatasource(), etc. ) I moved the implementation and remaining methods to BrokenDatasourceSupport. Is this reasonable ? Re: AB09 (disposeIfEmpty) - it doesn't make sense to keep the NSDCW dialog around if all datasources have been resolved. If this dialog is disposed then user doesn't have to click Close. Also, some implementation I removed from suggestions will have to be restored to update the datasource list in the NSDCW dialog (when a user resolves the datasource by add the corresponding connection then the datasource can be removed from the list)
> Re: AB06 (listening in on ConnectionManager) To do this requires WebProject > to have a dependency on Database Explorer. I haven't checked yet if this > causes a circular dependency. If not, is it OK to add this dependency. No problem to have such a dependency. > Re: AB11 (non-API methods getBrokenDatasource(), etc. ) I moved the > implementation and remaining methods to BrokenDatasourceSupport. Is this > reasonable ? Can't tell until I see the diff. However, you should not have any public methods in BrokenDatasourceSupport that are not called from web/project. > Re: AB09 (disposeIfEmpty) - it doesn't make sense to keep the NSDCW dialog > around if all datasources have been resolved. If this dialog is disposed > then user doesn't have to click Close. You're right, it doesn't. But this should be achieved by the dialog itself. The client shouldn't have to call a separate method for that. > Also, some implementation I removed from suggestions will have to be restored > to update the datasource list in the NSDCW dialog (when a user resolves the > datasource by add the corresponding connection then the datasource can be > removed from the list) That actually makes sense. Data sources can be removed from the list once they are resolved (either in the dialog or by manually registering a matching connection in the Services tab). Which item suggests not doing this? Switching component to j2ee/code, since the API will be added to j2ee/utilities.
Updates made, code is much cleaner, attachments will follow. Alert was working fine, then it stopped showing. On line 152 of BrokenDatasourceSupport, the result of !DatasourceUISettings.getDefault().isShowAgainBrokenDatasourceAlert() is returning true for some reason. I would expect the result should return false as I hadn't clicked on the showAgain checkbox. Some property got set when working with the UI ? Please check the BrokenDatasourceAction in WebLogicalViewProvider. I cleaned up the threading, but may be some remaining issue. Note, the NoSelectedDatabaseConnectionWarning dialog has been renamed to MissingDatabaseConnectionWarning Names should be more meaningful now.
Created attachment 43549 [details] changes to web/project/ui
Created attachment 43550 [details] changes to j2ee project utilities
apologies for neglecting to label my latest comments JB10 - Please disregard the Alert issue mentioned earlier : "Alert was working fine, then it stopped showing..." It was not showing because the previous time I had selected the checkbox so that the dialog should not show again, even after re-starting IDE, value is cached.
I found a bug in MissingDatabaseConnectionWarning in DatasourceListModel.refreshModel() . Checking connections might be done by another thread at the same time. Now checking in a RP thread. Source file attached.
Created attachment 43584 [details] MissingDatabaseConnectionWarning.java
It looks better now, thanks for addressing many issues. Some still remain, however, and there are some new ones: AB30: you are making some unintentional changes in project.xml: - remove the specification version for the org.netbeans.modules.j2ee.dd module - modify the specification version for the org.netbeans.modules.j2ee.dd.webservice and org.netbeans.modules.j2ee.persistenceapi modules Please revert these changes. AB31: in resource bundles you sometimes use "datasource", sometimes "data source". The right term is "data source" I think. AB32: I don't think the following code is correct: // if no open projects have broken datasources then remove the connection listener Project[] openProjects = OpenProjects.getDefault().getOpenProjects(); boolean anyBroken = false; for (int i = 0; i < openProjects.length; i++) { if (BrokenDatasourceSupport.isBroken(openProjects[i])){ anyBroken = true; } } if (!anyBroken) { ConnectionManager.getDefault().removeConnectionListener(this); } Each project has its own ConnectionListener. Why decide to unregister it based on other open projects? Also, the listener should probably only be unregistered when the project is closed. What if, for example, a connection is removed from the Services tab? That would make the project broken again. Thus you should probably use a weak listener for it (see org.openide.util.WeakListeners). AB33: what are the isShowAgainBrokenRefAlert() and setShowAgainBrokenRefAlert() methods in DatasourceUISettings for? Please drop DatasourceUISettings and just add SHOW_AGAIN_BROKEN_DATASOURCE_ALERT and a getter and setter for it to J2EEUISettings. AB34: you still have public methods in BrokenDatasourceSupport which should not be public, as they are not intended to be called by clients such as WebLogicalViewProvider. Already suggested in AB06 and in desc50 ("However, you should not have any public methods in BrokenDatasourceSupport that are not called from web/project"). AB35: you should not modify the static dlg fields in MissingDatabaseConnectionWarning, it is prone to race conditions. Instead, have a setDialog() method on MissingDatabaseConnectionWarning and use it to pass the dialog created in selectDatasources(), as already suggested in AB23. AB36: the following code in MDCW Datasource ds = (Datasource)datasourceList.getSelectedValue(); String itemSelected = ds.getJndiName(); if (itemSelected != null) { Datasource brokenDatasource = BrokenDatasourceSupport.getBrokenDatasource(project, itemSelected); can be replaced with Datasource BrokenDatasource = (Datasource)datasourceList.getSelectedValue(); You probably can remove the getBrokenDatasource() method completely. AB37: why do BDS.showAlert() and MDCW.selectDatasources() return a String value and what is the meaning of that string? What is the meaning of the MDCW.getSelectedDatasource() method and what does it return? AB38: there is code which enables the Close button of the Resolve Data Sources dialog when there are data sources to fix and disables it when there aren't. Why is this necessary, since when there aren't any data sources to fix the dialog should be hidden, and otherwise the button should be enabled? AB39: the datasources field of DatasourceListModel should not be static. AB40: the following code is MDCW.jButtonAddConnectionActionPerformed() ((DatasourceListModel) datasourceList.getModel()).refreshModel(); if (datasourceList.getModel().getSize() > 0 && !datasourceList.isVisible()) { datasourceList.setVisible(true); jScrollPane2.setBorder(scrollPaneBorder); jTextArea2.setText(org.openide.util.NbBundle.getMessage(MissingDatabaseConnectionWarning.class, "LBL_SelectDatasource")); } initDatasourceList(); refreshes the model twice (once by refreshModel() and then again by initDatasourceList()). Why is this necessary? Isn't refreshModel() enough? I also don't understand the code in the if statement. When is datasourceList not visible and why? AB41: DatasourceListModel probably doesn't need to be static and you don't need to pass the project to it, since it's available on the enclosing class. AB42: it seems to me you need to fire a list model change event from refreshModel(). Does the following scenario work? With the Resolve Data Sources dialog open for a broken data source, go to the Service tab and register a matching connection. The dialog should be hidden -- is it? AB43: please describe the exact issue that leads to refreshing the broken datasource model asynchronously as you mention in desc55. It could be caused by the datasources field being static (see AB39). Actually it might make sense to do the check asynchronously (since you do it almost everywhere else). You need to synchronize the access to the datasources field though (you have an unsynchronized access in the task's run() method) and you need to ensure that the list change event is fired in the event thread. The most important are AB30, AB32, AB35, AB38, AB39, AB40, AB41, AB43.
One more thing: please send real diffs that can be applied with the patch program. You need to add the new files (if the CVS support didn't do it for you) and do a "cvs diff -N" to ensure that newly added files will be present in the diff. Or use the NetBeans CVS support, but note it can't generate diffs in the "unified diff" format. Best to use the cvs command line tool if you can.
Most of AB30-AB43 addressed Re: AB32 . IMO, this is a nice-to-have. If a user is mucking around with connections, say deleted the wrong one then presumably they know how to replace the connection. Providing extra support for this deleting connection scenario can wait for next release or if really needed, as a bug fix. Re: AB43 Well, we've summed it up here in this issue: when a user adds a connection on their own through the Services tab then the Project node should get restored. The user cannot add connections with the Resolve Data Source dialog open. Adding more listeners, I think is not necessary. Re: AB35 I just made some quick changes by adding a setDlg(Dialog) and getDlg() then when disposing, call getDlg().dispose(). Seems awkward, please advise. Re: AB30 I'm using the IDE to just add the library reference, shouldn't be spec versions for other libraries I won't answer remaining questions now, maybe later.
Created attachment 43692 [details] J2EE utilities diffs
Created attachment 43693 [details] web project diffs
AB44: instead of commenting out code // if (datasourceList.getModel().getSize() > 0 && !datasourceList.isVisible()) { // datasourceList.setVisible(true); please just don't commit it. AB45: you added some duplicate code: if (brokenAlertShown || brokenAlertLastTime+BROKEN_ALERT_TIMEOUT > System.currentTimeMillis() || !J2EEUISettings.getDefault().isShowAgainBrokenDatasourceAlert()) { return; } if (brokenAlertShown || brokenAlertLastTime+BROKEN_ALERT_TIMEOUT > System.currentTimeMillis()) { return; } The second if statement was added in the last diff, probably unintentionally, please remove it. AB46: when you display the Resolve Data Sources dialog, no data source is selected, but the Add Connection button is enabled and a NullPointerException is thrown when the button is clicked. I don't agree with your answer to AB32, it is not a nice-to-have, but it is needed for consistency. When you remove a server in the Services tab, all project which have that server as the target will be marked as broken. It should work for connections the same way. If you don't have time to address this before you commit please file a bug for it. However, I still don't see why you need to look at other project in order to decide whether to remove this project's ConnectionListener. Could you please explain? IMHO instead of // if no open projects have broken data sources then remove the connection listener Project[] openProjects = OpenProjects.getDefault().getOpenProjects(); boolean anyBroken = false; for (int i = 0; i < openProjects.length; i++) { if (!BrokenDatasourceSupport.getBrokenDatasources(openProjects[i]).isEmpty()){ anyBroken = true; } } if (!anyBroken) { ConnectionManager.getDefault().removeConnectionListener(this); } you should just do if (!brokenDatasources) { ConnectionManager.getDefault().removeConnectionListener(this); } Re. AB34: the dlg field and the setDlg() method should not be static. If the field is static it can for example leak (probably not a big issue). Also conceptually the dialog is tied to the panel (one panel -> one dialog), so it makes sense for the dialog to be held by the panel instance. You don't need getDlg(). You did not address AB43: " You need to synchronize the access to the datasources field though (you have an unsynchronized access in the task's run() method)" and AB37. Do you plan to address them before you commit?
Created attachment 43720 [details] Resolve Data Sources dialog showing the label twice
Re: AB32 , I will file a bug. Except for AB46, the other issues have since been addressed I hope to fix AB46 today, else I may have to file a bug
Re: AB46 need more time to resolved. If not then I prefer to file an issue and fix after feature freeze (feature works but this is a high priority issue to fix) Re: AB32, I prefer to file a bug also I should be able to get help from someone today for AB46. If no other issues, (I'll attach diffs again) I'll commit later today.
Created attachment 43784 [details] web project diffs
Created attachment 43785 [details] j2ee utilities diffs
Forgot to mention, the other issues mentioned from AB30-AB46 , excluding AB32 and AB46 should have been addressed. please see diffs I just attached
AB34 still remains: the dlg field and the setDlg() method should not be static (see desc62). Can be fixed after commit.
The latest diff "Fri Jun 15 13:15:00 +0000 2007: j2eeutil2.diff " doesn't contain setDlg(...) I will commit the changes today. I'll file issues for AB46 and AB32 and try to address AB46 by the end of next week
you're right about the static Dialog instance. It has since been removed and will attach a diff of the integration I will make. Then I will file 2 bugs for AB46 and AB32
Created attachment 43812 [details] j2ee utilities diffs
For this feature, I'm adding an inner class BrokenDatasourceAction to Web Project's WebLogicalViewProvider to support adding a Project action and badging for visualweb projects when database connections used by the project are not available. Also support added to J2EE Project Utilities to add Database connections and listen to changes to Database Connections then remove the badged, marked project The feature requested is similar feature as the BrokenServerSupport in org.netbeans.modules.j2ee.common.ui For this release, only visualweb projects are supported. In future releases other types of projects that use Data Sources can be supported by making a straightforward change to WebLogicalViewProvider This enhancement is needed for the following reasoning: If the database connections are not available then the visualweb project cannot be developed at design-time or executed. At design-time a Component Error page will open instead of the expected page that has database-bound components. Also based on feedback from UI review for the spec, see the URL field for a link to the spec.
In the latest diff "Fri Jun 15 19:23:00 +0000 2007: j2eeutil3.diff", I had left in a call dlg.dispose() I removed this before integrating.
AB32 fixed http://www.netbeans.org/issues/show_bug.cgi?id=106885
Have you filed an issue for AB46 yet? If so, please make it block this issue.