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.
The classes JPopupMenuPlus and JMenuPlus should be deleted IMHO. They claim they fix bugs (in JDK) that are most probably no more present. I have tried to make those two classes no-op and it had following effects: on single monitor it did not do anything harmfull and on dual monitor it fixed some problems. There are still some problems on dual monitor support - I will investigate further and (hopefully) point out the other problematic code in NB. But I am (almost) sure those two classes should go away - opinions? If you don't feel like touching this issue please reassign to myself - I will try to work on this if time permits.
I agree completely. Actually, we need to revisit the whole menu thing at some point in the future wrt OS-X support also. David, why don't you attach your patch so we could test it and see if it causes any problems? If you have a chance to work on it, please do - I'm going to have a full plate for some time, and it sounds like you've got a more burning need to see this fixed. I'll be happy to help, review, test, whatever. So per your request I'm reassigning it to you for now and adding myself to cc. Hint: If you find you simply *have* to do some work on another thread and postpone displaying the menu (we should really get rid of all stuff like this), have a look at javax.swing.PopupFactory. Better to have our own popup logic than more hacks to try and make the default logic play nice with it. But really we should be able to get rid of all such hacks.
Created attachment 12690 [details] Proposed diff
Ok - if anyone would be able to test on his/her environment I would be gratefull. This is diff against release35 but I think it should be obvious what it does - deletes all logic from those two classes.
I think a trunk patch will need to be somewhat different; Tim already made some changes in the trunk, e.g. removing NbPopupMenuUI. Affected trunk classes probably include: JPopupMenuPlus; JPopupMenuUtils; JMenuPlus; JInlineMenu. See also issue #35827. IMHO we should prioritize this. I'm sure we could do it for 3.6.
David, I tried your patches on OS-X, and while the menus work, each popup menu flashes three times when it is invoked. I remember this behavior from when JDK 1.3 first came out. Anyway, I don't know what is responsible for the flashing (maybe we are calling show three times, or adding items on the fly - who knows), but probably it should be solved before we commit it.
FWIW, the source of the flashing: [exec] SetVisible false [exec] java.lang.Exception: Stack trace [exec] at java.lang.Thread.dumpStack(Thread.java:1089) [exec] at org.openide.awt.JPopupMenuPlus.setVisible(JPopupMenuPlus.java:40) [exec] at org.openide.awt.JPopupMenuUtils.dynamicChange(JPopupMenuUtils.java:73) [exec] at org.openide.awt.JInlineMenu.addItems(JInlineMenu.java:289) [exec] at org.openide.awt.JInlineMenu.updateContents(JInlineMenu.java:154) [exec] at org.openide.awt.JInlineMenu.access$000(JInlineMenu.java:35) [exec] at org.openide.awt.JInlineMenu$Updater.run(JInlineMenu.java:299) [exec] at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:178) [exec] at java.awt.EventQueue.dispatchEvent(EventQueue.java:448) [exec] at java.awt.EventDispatchThread.pumpOneEventForHierarchy(EventDispatchThread.java: 230) [exec] at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java: 183) [exec] at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java: 177) [exec] at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java: 169) [exec] at java.awt.EventDispatchThread.run(EventDispatchThread.java:99) [exec] SetVisible true [exec] java.lang.Exception: Stack trace [exec] at java.lang.Thread.dumpStack(Thread.java:1089) [exec] at org.openide.awt.JPopupMenuPlus.setVisible(JPopupMenuPlus.java:40) [exec] at org.openide.awt.JPopupMenuUtils.dynamicChange(JPopupMenuUtils.java:79) [exec] at org.openide.awt.JInlineMenu.addItems(JInlineMenu.java:289) [exec] at org.openide.awt.JInlineMenu.updateContents(JInlineMenu.java:154) [exec] at org.openide.awt.JInlineMenu.access$000(JInlineMenu.java:35) [exec] at org.openide.awt.JInlineMenu$Updater.run(JInlineMenu.java:299) [exec] at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:178) [exec] at java.awt.EventQueue.dispatchEvent(EventQueue.java:448) [exec] at java.awt.EventDispatchThread.pumpOneEventForHierarchy(EventDispatchThread.java: 230) [exec] at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java: 183) [exec] at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java: 177) [exec] at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java: 169) [exec] at java.awt.EventDispatchThread.run(EventDispatchThread.java:99)
This used to be a problem only for heavyweight popups. Is it also a problem for the lightweight popup on OSX?
I think maybe the all popups on Aqua L&F are heavyweight. They all have that MacOS drop-shadow, and I doubt they bothered with cool tricks to do it with a lightweight popup.
Another culprit on dual monitor: JPopupMenuPlus! I have found that if there is a ToolsAction in the popup it appears on the wrong monitor. Most probably caused by the attempt to "fix" its position in JPopupMenuPlus, namelly when calling the setLocation(...). The whole JPopupMenuPlus is highly suspicious with this respect - might also deserve some big cut ...
IMO the right way to solve this is to eliminate the need for these classes. It looks like, after all these years, we still have other threads (or invokeLaters) throwing items into the menu after it's shown - or something weird is still done with menus. I've been running on my mac for a week or two with your patches to kill these classes, and all menus appear and flash three or four times, dancing around the screen. This is only happening because something is adding to the menu - the Editor menu flashes about six times before it "settles down". The reason this is a problem on the mac is that, there, all popups are heavyweight. Without these classes, flashing will also happen for heavyweight popups on other OSs. It makes me feel like I'm in a time warp to 1999... So I think a good start to eliminating these classes would be to fix whatever still ails our system of constructing/showing menus. This probably includes API changes - let toolbar presenters return an array of items, not try to make magical things like JInlineMenu. Then we can kill all these classes without breaking anything. For now, we should probably at least create some line-switch for users with multiple monitors that makes these classes effectively no-ops. I'll happily integrate a patch that does that.
If you would succeed to eliminate JInlineMenu that might be it. I hope that it is the only code doing the dancing. After you do it you will find that nobody else (besides DockingAction ;-) calls these strange things (JPopupMenuUtils). And JInlineMenu is used only from 5 actions - not that bad.
Moving target milestone to "future", because it's too complicated and time-expensive thing to do for 3.6, moreover we still can't test here. BTW thanx David for your effort!
Hey, I still work on this issue. Unfortunatelly I don't know your deadline - if you (Sun) would publish that I would be able to say whether I can make it for 3.6. I will definitelly have final patch for release35 during next week. Porting that patch to 3.6 and testing might take some time still but ...
David, what sort of patch are you working on - one that eliminates this code, or a line-switch type thing, or something that really fixes the menu mess? If it's something that has no effect unless the application or user sets a system property, I wouldn't have a problem with integrating that in the next couple weeks. If it will change the default behavior of menus, then it's too risky for 3.6.
If a property based solution would fit into 3.6 I am ready to contribute patch that will not do anything unless a special property is set. What about "org.netbeans.windows.dualMonitor=true"? The patch as it seems now will include the classes that I have already attached + JPopupMenuPlus + DefaultContainerImpl.
BTW - probably a complete patch would also fix issue #35827, which is too late to do now for 3.6.
David, sounds fine to me.
I've been playing with a highly ambitious idea for how to build a much more scalable, better performing actions subsystem that allows for declarative actions and various other stuff, and in particular, uses a "pull" rather than "push" model for enablement (I've got some strange ideas about polling schemes for enabling toolbar buttons - probably too weird but at least interesting to play with). As a result I've been reading the sources for JMenu/JMenuItem, etc. It seems to me there ought to be four or five ways to make sure a menu's contents are dynamically populated before it is shown, without all the weirdness - a number of them might not even require a subclass: - Add an action listener - this might get processed after the UI gets it and posts the popup, or before - not sure - Add a mouse listener - very likely to work - at least in theory, the menu should get posted in processMouseEvent(); listeners should be notified first. Just make sure to check isPopupMenuTrigger(), becuase in some places it's MOUSE_PRESSED, in others MOUSE_RELEASED - Worst case is use an AWTEventListener - Various safer places in the menu than addNotify() BTW, I strongly suspect the reason putting random non-JMenuItem components on the menu is that Apple is using getItemCount() & getItem(pos) in whatever adapter connects them to native menus. These methods are there for compatibility with AWT menus, and are documented to return null for anything but a JMenuItem. It would also explain the blank spaces where the missing items should be. One probable way to get them working properly on OS-X without doing anything terribly radical might be to override this method to drill into whatever random component has been added, and at least if it can find JMenuItems in it, it should be able to return or proxy them.
FWIW, I was wrong about getItemCount()/getItem().
To Tim: Re. pull vs. push - technically actions do currently fire property changes to tell their presenters when to change, but it *should* be the case that nothing happens when the action is not showing - the presenter should remove any listener from the action while the action is hidden (e.g. in a menu, until the menu is posted), and the action should notice that the listener count drops to zero (via addNotify) and cease listening on anything itself and firing changes. When a getter for a variable property (typically Action.enabled) is called at a time such that between the previous getter call and this one, there was at least some point during which there were no listeners, then any caches are discarded and the value is computed fresh. At least, I spent a couple of days at one point making sure this all worked in the case of NodeAction.enabled, and writing thorough unit tests for it. So don't just assume that changes are being pushed for no reason - if you see some being fired from a particular action when it is not visible, it may be a bug, so report it or fix it. Re. dynamically populating menu items - there is no technical difficulty in doing this, I think. Issue #35827 covers the necessity for having our APIs support it, which is the real reason this has been blocked for so long. Actually implementing it should be easy. See e.g. http://www.netbeans.org/unbranded-source/browse/~checkout~/javadoc/src/org/netbeans/modules/javadoc/search/IndexOverviewAction.java?content-type=text/plain All you have to do is override JMenu.getPopupMenu to make any necessary changes and then call super. It works fine AFAIK. If View -> Documentation Indices works on Mac OS X and multiple monitors etc. without apparent problems, then we are all set. (#35827 also discusses how the API can permit the menu to do the minimum of work in getPopupMenu, i.e. not add or remove menu items unless it needs to.)
*** Issue 30694 has been marked as a duplicate of this issue. ***
Created attachment 13230 [details] Proposed diff against trunk 2004/02/04
Hotfix patch uses system property -J-Dnetbeans.popup.no_hack=true. If the property is not set the code should do exactly what it did before. Please review. With this patch it works for me on dual monitor box (Win XP). Testers welcomed. If there are no objections I plan to put it into release36.
Don't forget to also patch openide/arch/arch-openide-actions.xml#exec-property.
Since there were no objections, I have applied the patch: Checking in core/windows/src/org/netbeans/core/windows/view/ui/TabbedListener.java; /cvs/core/windows/src/org/netbeans/core/windows/view/ui/TabbedListener.java,v <-- TabbedListener.java new revision: 1.7; previous revision: 1.6 done Processing log script arguments... More commits to come... Checking in openide/src/org/openide/awt/JMenuPlus.java; /cvs/openide/src/org/openide/awt/JMenuPlus.java,v <-- JMenuPlus.java new revision: 1.17; previous revision: 1.16 done Checking in openide/src/org/openide/awt/JPopupMenuPlus.java; /cvs/openide/src/org/openide/awt/JPopupMenuPlus.java,v <-- JPopupMenuPlus.java new revision: 1.8; previous revision: 1.7 done Checking in openide/src/org/openide/awt/JPopupMenuUtils.java; /cvs/openide/src/org/openide/awt/JPopupMenuUtils.java,v <-- JPopupMenuUtils.java new revision: 1.16; previous revision: 1.15 done Processing log script arguments... More commits to come... Checking in openide/arch/arch-openide-actions.xml; /cvs/openide/arch/arch-openide-actions.xml,v <-- arch-openide-actions.xml new revision: 1.22; previous revision: 1.21 done
*** Issue 39781 has been marked as a duplicate of this issue. ***
I prefixed the title in hopes that it will help the patch make it into the next release of NB. It will now show up in the NetCAT statistics.
The patch *will* make it into the next release. However, you will need to set a command line switch or it will be inactive.
*** Issue 39740 has been marked as a duplicate of this issue. ***
verified - fixed by reporter
*** Issue 41813 has been marked as a duplicate of this issue. ***
*** Issue 47848 has been marked as a duplicate of this issue. ***