Bugzilla – Full Text Bug Listing
| Summary: | [Help] Add warnings for duplicate Context Sensitive Help ids on UI elements when debug is enabled | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Chris Austin <ChrisAustin> | ||||||
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||||
| Status: | VERIFIED FIXED | QA Contact: | Eric Moffatt <emoffatt> | ||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | bokowski, cgold | ||||||
| Version: | 3.5 | ||||||||
| Target Milestone: | 3.6 M5 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Windows XP | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Chris Austin
Created attachment 152990 [details]
Initial idea
This is just a rough outline of the code - Strings still need to be externalized, and I could use some assistance in determining the best way to set up debug flags. As in, should we add something like org.eclipse.ui.workbench/debug/csh, or should we link it to help, org.eclipse.help/debug/csh.
Also, note, this is only a debug warning because duplicate context ids can be valid for different controls. This is only meant as a mechanism for helping to debug csh failures.
Just a few initial comments: 1) we don't want to hang onto controls/widgets after they've been disposed. Why not just keep the list of context IDs that have been set before? 2) I think there's a lot of different ways to get the same help ids registered against different controls, such as opening the same kind of dialog over and over, closing and opening a view part, opening the same editor over multiple files, etc. 3) For tracing you can just use sysouts or helper methods like org.eclipse.core.commands.util.Tracing.printTrace(String, String) ... we don't normally trace into the workspace log PW Thanks for the comments Paul. (In reply to comment #2) > Just a few initial comments: > > 1) we don't want to hang onto controls/widgets after they've been disposed. > Why not just keep the list of context IDs that have been set before? I don't believe I am actually holding onto the controls here - the setHelpTrace method I wrote does take an Object param (which is the Control being registered) but as I wrote it, I didn't actually need it. I only keep track of the String contextId, and the StackTraceElement where setHelp is called. So the first arg to setHelpTrace is obsolete and I will remove it. > 2) I think there's a lot of different ways to get the same help ids registered > against different controls, such as opening the same kind of dialog over and > over, closing and opening a view part, opening the same editor over multiple > files, etc. Yes, that seems to be true. I only warn of duplicate context ids if the current StackTraceElement called is different. So if the same dialog is opened twice, even spawned from different threads, it will have the same call on the stack, ie: org.mypackage.MyDialog.<init>(MyDialog.java:54) > 3) For tracing you can just use sysouts or helper methods like > org.eclipse.core.commands.util.Tracing.printTrace(String, String) ... we don't > normally trace into the workspace log Good to know, I can change this. Created attachment 153084 [details]
Patch Revision 2
Chris, I've just taken a quick look at the patch and it seems fine to me. I'll take a closer look once I get back to the 3.x work...I've tagged it for work in M5. Committed in >20100119. Applied the patch (also added an attribution and changed the copyright date). Chris, could you pick up a build (M5?) and verify that this is working as you expect? If you can't then please add some verification steps to this defect that will allow me to test. Marking as FIXED. (In reply to comment #6) > Chris, could you pick up a build (M5?) and verify that this is working as you > expect? If you can't then please add some verification steps to this defect > that will allow me to test. Thanks Eric - I will grab an M5 with the change and test it. Committed in >20100119. Removed the line Thread.currentThread().getStackTrace(); and replaced it with: // Create an unthrown exception to capture the stack trace IllegalArgumentException e = new IllegalArgumentException(); StackTraceElement[] stackTrace = e.getStackTrace(); in order to meet the Java 1.4 requirement. You should probably use "new RuntimeException()". I used IllegalArgumentException simply because it was already being used in the existing source. What's the concern (performance) ? Not performance - it just doesn't feel right. You are mis-using IllegalArgumentException for a case where there is no illegal argument. Not to nit pick but this *is* an illegal argument, you've called 'setHelp' with an id that's already in use for another part. The strict implementation of this would simply throw the exception after setting the error message to the string displayed in the sysout. That being said I'll change it since that's not how it's currently working... Committed in >20100119. Switched to RuntimeException. OK, I grabbed a build and everything seems to be working properly. Thanks again. Can you set the state to VERIFIED and indicate which build you used to test? (In reply to comment #15) > Can you set the state to VERIFIED and indicate which build you used to test? Yes of course. I used eclipse-SDK-N20100119-2000-win32. Thanks Chris ! |