Bugzilla – Full Text Bug Listing
| Summary: | Show full paths preference under C/C++->Debug is not really necessary | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Warren Paul <warren.paul> | ||||||||
| Component: | cdt-debug | Assignee: | John Cortell <john.cortell> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | Ken Ryall <ken.ryall> | ||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | elaskavaia.cdt, john.cortell, marc.khouzam, nobody, pawel.1.piech | ||||||||
| Version: | 7.0 | Flags: | nobody:
review+
|
||||||||
| Target Milestone: | 7.0.1 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Warren Paul
I believe this preference is meant to trigger the very first time the debug view and breakpoints view is opened. The way I imagine it is that if you create a new workspace and you import your preference file, you can have the debug/breakpoints view show full paths or not based on that preference, instead of having to set the option manually in the view. From what I see it is being used by ShowFullPathsAction#init (from ViewFilterAction) which calls getPreferenceValue(), which will check that preference but only if the view in question doesn't have it set specifically (i.e., only the first time the view is opened, in a new workspace). This logic works for both CDI and DSF (bug 248627) (In reply to comment #1) > I believe this preference is meant to trigger the very first time the debug > view and breakpoints view is opened. The way I imagine it > is that if you create a new workspace and you import your preference file, you > can have the debug/breakpoints view show full paths > or not based on that preference, instead of having to set the option manually > in the view. > > From what I see it is being used by > ShowFullPathsAction#init (from ViewFilterAction) which calls > getPreferenceValue(), which will check that preference but only > if the view in question doesn't have it set specifically (i.e., only the first > time the view is opened, in a new workspace). > This logic works for both CDI and DSF (bug 248627) Wow. I overlooked the use of the base preference key string in ViewFilterAction.getPreferenceValue(). I saw the code adding the string to the view id, as it does in other places. Talk about some code that could have used a little documentation to state the non-obvious. Personally, I don't like that mechanism. To have a preference exposed in the GUI that only has an effect the next time your create a workspace is a head-scratcher in my book. I'm sure there are exceptions where that kind of pref makes sense, but certainly not here. The action should simply persist the per-view state every time it's changed. That will provide desirable/expected behavior 99% of the time when creating a new workspace and importing the pref file, no? Note that the show full path action is the ONLY action using this mechanism (at least, subclassing ViewFilterAction), so that tells me it is doing something out of the ordinary. (In reply to comment #2) > (In reply to comment #1) > > I believe this preference is meant to trigger the very first time the debug > > view and breakpoints view is opened. The way I imagine it > > is that if you create a new workspace and you import your preference file, you > > can have the debug/breakpoints view show full paths > > or not based on that preference, instead of having to set the option manually > > in the view. > > Personally, I don't like that mechanism. To have a preference exposed in the > GUI that only has an effect the next time your create a workspace is a > head-scratcher in my book. That behavior was there before, and maybe there is another case where this behavior makes sense. I know Mikhail was using it. > I'm sure there are exceptions where that kind of > pref makes sense, but certainly not here. The action should simply persist the > per-view state every time it's changed. That will provide desirable/expected > behavior 99% of the time when creating a new workspace and importing the pref > file, no? Now that you mention it, as soon as the view is opened, it stores the general preference as a view-specific preference setting, which I believe is also stored when exporting preferences. Therefore, my scenario is only useful if you've exported your preferences without having opened one of the Debug or Breakpoints view. So I agree that this seems like a pretty rare case to have a global preference just for it. So, I second the motion to remove that global preference. However, I would like to get confirmation from Mikhail, as I may be missing some information on this. > Note that the show full path action is the ONLY action using this mechanism (at > least, subclassing ViewFilterAction), so that tells me it is doing something > out of the ordinary. Yeah, I was surprised too that there was this ViewFilterAction base class that no-one else used. (In reply to comment #2) > Wow. I overlooked the use of the base preference key string in > ViewFilterAction.getPreferenceValue(). I saw the code adding the string to the > view id, as it does in other places. Talk about some code that could have used > a little documentation to state the non-obvious. > > Personally, I don't like that mechanism. To have a preference exposed in the > GUI that only has an effect the next time your create a workspace is a > head-scratcher in my book. I'm sure there are exceptions where that kind of > pref makes sense, but certainly not here. The action should simply persist the > per-view state every time it's changed. That will provide desirable/expected > behavior 99% of the time when creating a new workspace and importing the pref > file, no? > In general, it's a common approach (not an exception) to have a global preference that affects all settings. Not sure if it works in this case, but by using "Reset Perspective" action users should be able to reset the default values of all UI related properties, so this is not only a one time preference. > Note that the show full path action is the ONLY action using this mechanism (at > least, subclassing ViewFilterAction), so that tells me it is doing something > out of the ordinary. As far as I remember the ViewFilterAction class was simply copied from the platform. It implements an enablement mechanism. For instance, "Show full paths" it is enabled only for ICBreakpoint and ICDebugElement elements in the Breakpoints and Debug views. At that time I thought it would be useful to have an abstract class. Don't forget that this was implemented long before the action enablement mechanism was introduced. (In reply to comment #4) > In general, it's a common approach (not an exception) to have a global > preference that affects all settings. Not sure if it works in this case, but by > using "Reset Perspective" action users should be able to reset the default > values of all UI related properties, so this is not only a one time preference. I just don't see this approach being of any practical use for this particular action (maybe in other cases, it does). I'd be *extremely* surprised if any user understands what that global preference represents (how it affects behavior), and even more surprised if they rely on it. It took three CDT developers to look at the code to understand what the preference does, and even now there's still some doubts. (In reply to comment #5) > I just don't see this approach being of any practical use for this particular > action (maybe in other cases, it does). I'd be *extremely* surprised if any > user understands what that global preference represents (how it affects > behavior), and even more surprised if they rely on it. It took three CDT > developers to look at the code to understand what the preference does, and even > now there's still some doubts. John, I don't mind removing this particular preference. The issue is not worth the time we have spent discussing it. I was just trying to explain what why it was implemented in this particular way. Created attachment 172069 [details]
Fix
Thanks, Mikhail. Here's a patch. Can you please review it?
Committed to HEAD. Commit to 7_0 branch pending code review. (In reply to comment #7) > Created an attachment (id=172069) [details] > Fix accidental regression in last commit. Also, enhanced documentation Weird patch name :-) I had a look at the patch since I played with this stuff not long ago. I think this makes the preferences clearer for the user but also makes the code clearer. 1- Can we move the ShowFullPathsAction.PREF_KEY to a Constants file that is also accessible by DsfLaunch? That would avoid having a copy of that id in DsfLaunch, which is not nice. 2- CDebugPreferencePage line 178 has a comment that should be removed. 3- PreferenceMessages#CDebugPreferencePage.5 can probably be removed 4- Maybe fix the comment above DsfLaunch.PREF_SHOW_FULL_PATHS (depening on #1) 5- I haven't tested it, but I believe that when starting from a fresh workspace (or resetting the perspective?), the showFullPath option will now be off, when the default used to be on. Do you think this will annoy the user? I think the only way around it is to put back the IS_SET trick... Created attachment 172077 [details]
Additional changes
Comment on attachment 172077 [details]
Additional changes
Additional changes based on Marc's review.
(In reply to comment #9) > Weird patch name :-) That's auto-completion for you. Usually a friend, but not always. > 1-4. Done. > 5- I haven't tested it, but I believe that when starting from a fresh workspace > (or resetting the perspective?), the showFullPath option will now be off, when > the default used to be on. Do you think this will annoy the user? I think the > only way around it is to put back the IS_SET trick... I think this is actually nicer, but that's just my preference. File paths are usually pretty long, making it quite a pain, and most source files are unique in a project. But, if someone objects, I can restore the previous behavior. BTW, the off state is not re-introduced on a perspective reset. It happens only for a new workspace. Please review latest patch. Will hold off on comitting. While I was installing the latest version and creating a new workspace Marc beat me with the review (thanks!). I agree with his comments except #5 - I think having "Show Full Paths" turned off by default is a good idea, hope nobody will notice this change. (In reply to comment #12) > (In reply to comment #9) > > 1-4. > > Done. Thanks. I didn't see changes to DsfLaunch, but I'm assuming you just forgot to include it in the posted patch? > I think this is actually nicer, but that's just my preference. (In reply to comment #13) > I think having "Show Full Paths" turned off by default is a good idea, That's fine with me then. Since it will be in both HEAD and 7.0.1, I'm sure someone will say something by the time 7.0.1 is released, if they are really bothered by it. Created attachment 172147 [details]
Additional changes
(In reply to comment #15) > Created an attachment (id=172147) [details] > Additional changes Looks good to me. Thanks Committed to HEAD and cdt_7_0. *** cdt cvs genie on behalf of jcortell *** Bug 316987: Show full paths preference under C/C++->Debug is not really necessary [*] IDsfDebugUIConstants.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/ui/IDsfDebugUIConstants.java?root=Tools_Project&r1=1.5&r2=1.6 [*] ICDebugPreferenceConstants.java 1.22 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/preferences/ICDebugPreferenceConstants.java?root=Tools_Project&r1=1.21&r2=1.22 [*] CDebugPreferencePage.java 1.32 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/preferences/CDebugPreferencePage.java?root=Tools_Project&r1=1.31&r2=1.32 [*] ShowFullPathsAction.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/ShowFullPathsAction.java?root=Tools_Project&r1=1.11&r2=1.12 [*] ViewFilterAction.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/ViewFilterAction.java?root=Tools_Project&r1=1.11&r2=1.12 [*] IDsfDebugUIConstants.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/ui/IDsfDebugUIConstants.java?root=Tools_Project&r1=1.6&r2=1.7 [*] ICDebugInternalConstants.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/internal/core/ICDebugInternalConstants.java?root=Tools_Project&r1=1.5&r2=1.6 [*] MANIFEST.MF 1.27 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/META-INF/MANIFEST.MF?root=Tools_Project&r1=1.26&r2=1.27 [*] DsfLaunch.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/debug/model/DsfLaunch.java?root=Tools_Project&r1=1.6&r2=1.7 [*] PreferenceMessages.properties 1.17 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/preferences/PreferenceMessages.properties?root=Tools_Project&r1=1.16&r2=1.17 [*] CDebugPreferencePage.java 1.33 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/preferences/CDebugPreferencePage.java?root=Tools_Project&r1=1.32&r2=1.33 [*] ShowFullPathsAction.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/ShowFullPathsAction.java?root=Tools_Project&r1=1.12&r2=1.13 [*] IDsfDebugUIConstants.java 1.5.2.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf.ui/src/org/eclipse/cdt/dsf/debug/ui/IDsfDebugUIConstants.java?root=Tools_Project&r1=1.5&r2=1.5.2.1 [*] ICDebugInternalConstants.java 1.5.20.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/internal/core/ICDebugInternalConstants.java?root=Tools_Project&r1=1.5&r2=1.5.20.1 [*] MANIFEST.MF 1.26.2.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.core/META-INF/MANIFEST.MF?root=Tools_Project&r1=1.26&r2=1.26.2.1 [*] DsfLaunch.java 1.6.2.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/debug/model/DsfLaunch.java?root=Tools_Project&r1=1.6&r2=1.6.2.1 [*] ShowFullPathsAction.java 1.11.2.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/actions/ShowFullPathsAction.java?root=Tools_Project&r1=1.11&r2=1.11.2.1 [*] PreferenceMessages.properties 1.16.2.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/preferences/PreferenceMessages.properties?root=Tools_Project&r1=1.16&r2=1.16.2.1 [*] CDebugPreferencePage.java 1.31.2.1 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/all/org.eclipse.cdt.debug.ui/src/org/eclipse/cdt/debug/internal/ui/preferences/CDebugPreferencePage.java?root=Tools_Project&r1=1.31&r2=1.31.2.1 |