Bugzilla – Full Text Bug Listing
| Summary: | [Memory Browser] Want to keep a history in the Go To address field | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Tools] CDT | Reporter: | Alain Lee <a-lee> | ||||||||||||||||||||
| Component: | cdt-memory | Assignee: | Randy Rohrbach <Randy.Rohrbach> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | Ted Williams <ted> | ||||||||||||||||||||
| Severity: | critical | ||||||||||||||||||||||
| Priority: | P3 | CC: | john.cortell, Randy.Rohrbach | ||||||||||||||||||||
| Version: | 7.0 | Flags: | john.cortell:
review+
|
||||||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Alain Lee
This is an important feature for our product. Created attachment 187314 [details]
patch
Alain In the GoToAddressBarWidget at line 159 you clean the list even if the incoming list is null. Is this what you really want to do here. Also this is a new feature which is being applied to 7.0.2. I think I like this feature, but since it is a feature you need to get approval from the community to add a feature to 7.0.2. I doubt this will be hard to do. I will apply the patch and see what it looks like and give you more feeback. Randy Created attachment 187358 [details]
Update to Alain's original patch
Alain
When I applied your patch it did not compile, just a simple Text to Combo
change. When I tested it I found that the Combo box was not retaining the
selections. I have never really used a Combo/Text box before and I would
have thought the retention would be automatic. I solved this by creating
a new Method which is called when the GoToAddress is called.
So take a look at my changes.
I really like the way this looks and I am +1 for this.
Randy
Alain When I applied your patch it did not compile, just a simple Text to Combo change. When I tested it I found that the Combo box was not retaining the selections. I have never really used a Combo/Text box before and I would have thought the retention would be automatic. I solved this by creating a new Method which is called when the GoToAddress is called. So take a look at my changes. I really like the way this looks and I am +1 for this. Randy Created attachment 187394 [details]
Add persistense for the expressions
I added persistense of the expressions in the Property/Preference store so the
entered expressions would not be lost when the view is closed and reopened. I
also added a Menu choice to clear the expressions.
I am taking this one.
I have created twp patches here.
1.) Some fixes to Alain's patch to get it to work.
2.) Everything #1 has plus persistence of the expressions so
if the view is closed and reopened they come back. I also
added the menu operation to clear the expression list.
John Cortel please take a look at these patches. Despite the lateness
of the request, I think this adds real value. It does not affect the
APIs, only new functions were added.
Since I was working on a fix from Andre St Laurent ( a Wind River
guy who works for me ) I included in this patch his work. This is
bugzilla 330468. It had to do with not properly clearing the "x"
error indication.
Thanks
Randy
(In reply to comment #3) > Alain > In the GoToAddressBarWidget at line 159 you clean the list even if the > incoming list is null. Is this what you really want to do here. Randy, This is intentional so that the list can be cleared if the active rendering has been changed and the new active rendring does not have any history. (In reply to comment #7) > I am taking this one. > I have created twp patches here. > 1.) Some fixes to Alain's patch to get it to work. > 2.) Everything #1 has plus persistence of the expressions so > if the view is closed and reopened they come back. I also > added the menu operation to clear the expression list. > John Cortel please take a look at these patches. Despite the lateness > of the request, I think this adds real value. It does not affect the > APIs, only new functions were added. > Since I was working on a fix from Andre St Laurent ( a Wind River > guy who works for me ) I included in this patch his work. This is > bugzilla 330468. It had to do with not properly clearing the "x" > error indication. > Thanks > Randy Randy, I was unable to apply your patch for GoToAddressBarWidget on top of my patch. There are some conflicts because your patch was based on version 1.6 while my patch was based on 1.7. My patch also has a method called addToExpressionList() that provides the function as the addExpressionToList() method in your patch. (In reply to comment #7) > 2.) Everything #1 has plus persistence of the expressions so > if the view is closed and reopened they come back. I also > added the menu operation to clear the expression list. Randy, I was going to ask if we needed to persist the expressions but you got it implemented already. This is what our customers are looking for. In my patch, I tried to keep the list in the local cache based on memory space. Could we persist the list based on the memery space so that each memory space has a different list? Is it possible to persist the list based on the launch configuration. Therefore, the same list is restored if the same configuration is relaunched? (In reply to comment #7) > John Cortel please take a look at these patches. Despite the lateness > of the request, I think this adds real value. It does not affect the > APIs, only new functions were added. Randy, I'm going to wait until you respond to Alain's last two posts, in case it results in an updated patch. Created attachment 187875 [details]
Updated patch with "Memory Space" support
Alain requested support for different Memory Spaces. So I
changed the methods to include a Memory Space ID. Currently
they do not do anything to delineate.
I emailed/talked with Alain and he would like ( and I agree )
to put the persistense in the Launch and based on memory IDs.
So the methods now support this. Wind Rivers product does not
support Memory Spaces so I am not going to invest more time in
that direction at this point. Alain would like this patch applied
and then he will work on advancing it.
For Wind River the single maintained list ( like with the Expressions
View ) is sufficient at this time.
Randy
Randy, I'm having problems applying your latest patch file. I've attached a screenshot. Eclipse seems unable to figure out where your changes are supposed to fit into. I suspect something to do with how you created the patch? Created attachment 187880 [details]
Screenshot showing problems with patch
John That looks weird. The edits are pretty simple and straightforward. To create the patch I just do Team --> Create Patch select all defaults and Finish. Is there something else I need to do. I can create ZIP with just the changed files. Randy Created attachment 187881 [details]
ZIP of the patch I submitted earlier
ZIP of the patch I submitted earlier.
For some reason when John Cortel is looking at it the Apply patch function
does not seem happy with it.
Randy
Looking at the patch file, I'm seeing something strange. It claims to have a delta to revision 1.7 of org.eclipse.cdt.debug.ui.memory.memorybrowser/plugin.xml ...but the last version in the repo is 1.6! Did you possibly create this patch in a workspace created from Wind River's repository? Created attachment 187882 [details]
Forgot the plugin.xml in the ZIP
(In reply to comment #17) > Looking at the patch file, I'm seeing something strange. It claims to have a > delta to revision 1.7 of > > org.eclipse.cdt.debug.ui.memory.memorybrowser/plugin.xml > > ...but the last version in the repo is 1.6! Did you possibly create this patch > in a workspace created from Wind River's repository? Actually, I was looking at the wrong plugin. That file has a rev 1.8. So it appears the issues is that you don't have the latest from CVS. You need to do that before you create the patch John I check all of the files ( only 3 ) against HEAD and the history shows I have the latest changes, plus mine. If you want give me a call 781-364-2226. I attached the sources above, but something is clearly wrong. I did a TEAM --> UPDATE to make sure and it shows nothing new for these files. Thx, Randy (In reply to comment #17) > Looking at the patch file, I'm seeing something strange. It claims to have a > delta to revision 1.7 of > org.eclipse.cdt.debug.ui.memory.memorybrowser/plugin.xml > ...but the last version in the repo is 1.6! Did you possibly create this patch > in a workspace created from Wind River's repository? I created my original patch based on Indigo and 1.7 was the latest version. (In reply to comment #21) > (In reply to comment #17) > > Looking at the patch file, I'm seeing something strange. It claims to have a > > delta to revision 1.7 of > > org.eclipse.cdt.debug.ui.memory.memorybrowser/plugin.xml > > ...but the last version in the repo is 1.6! Did you possibly create this patch > > in a workspace created from Wind River's repository? > > I created my original patch based on Indigo and 1.7 was the latest version. Indigo is whatever is in CVS this very second :-) And that's 1.8 (for that plugin.xml) (In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #17) > > > Looking at the patch file, I'm seeing something strange. It claims to have a > > > delta to revision 1.7 of > > > org.eclipse.cdt.debug.ui.memory.memorybrowser/plugin.xml > > > ...but the last version in the repo is 1.6! Did you possibly create this patch > > > in a workspace created from Wind River's repository? > > > > I created my original patch based on Indigo and 1.7 was the latest version. > Indigo is whatever is in CVS this very second :-) And that's 1.8 (for that > plugin.xml) You are right. I was still using the build from last December (In reply to comment #16) > Created attachment 187881 [details] > ZIP of the patch I submitted earlier > ZIP of the patch I submitted earlier. > For some reason when John Cortel is looking at it the Apply patch function > does not seem happy with it. > Randy Randy, I quickly browsed through your changes. It does not seem that the history is updated when a rendering for a different CPU is made active. The Expression text does get reloaded. I think the history should get reloaded as well to match the expression text. This is why my patch keeps the history in the CTabItem's data Created attachment 187889 [details]
Updated version against 8.0
We finally worked it out. I had constructed the patch incorrectly.
Randy
John
I created a new patch. Then resync back to HEAD and reapplied
the patch as you would and ( of course ) it looked a whole lot
better.
When you take a look at this please remember a few things
1.) This contains another patch for bugzilla 330468. So
there are a combination of edits. I was working on
that one, when Alain's patch came in and they were
in the same files. So killing 2 birds so to speak.
2.) I added a simple persistense to Alain's patch which
he would like to enhance. So we elected to have a
Memory Space ID specified in the new methods. But
the current implementation still is really only
managing an entire list.
3.) Alain intends to work from this, by creating a
Memory Space ID persistense which is probably based
in the launch. But this is progress in itself. I
really liked the drop-down approach.
Randy
Here's my review of the latest patch. Much of this is minor stuff: 1. In ClearExpressionsListAction.java, the instanceof check in the init() method seems unnecessary. The run() method does a more selective instanceof. fView can just be an IViewPart. There are other obvious alternatives. 2. In GoToAddressBarWidget, 2.1 expressionStatus should be fExpressionStatus, for consistency 2.2 fExpressionStatus could just be assigned to STATUS.OK_STATUS and save ourselves an object creation 2.3 StringTokenizer is commonly used for parsing a delimited string. May not save much code in getSavedExpressions(), but it may end up a little more intuitive 2.4 createExpressionField need not check the returned reference for either null or zero length. The method is private and never returns null. Also, checking for zero length adds no value. 3. There should be a cap on the history length, using FIFO. Giving the user an option to completely clear the history is nice, but the truth is it should never get to that. The view should ensure the list doesn't get unwieldy. This is par for the course in features that maintain a history in widgets (e.g., "Open Recent"). Alain I just looked at your patch and the changes show there. When I went to apply it they did not get applied, so I created my own. I like the idea of a per render list. But I also did it the way I did to have a list which comes back to life when you close the view and reoopen it. Feel free to change my patch to include your ideas, but try and make sure to have a persistense when the view goes away. Also you asked about a null pointer removing everything. No issue changing that either. But there must be a way to indicate clearing the list for the clear expressions menu selection I created. Randy (In reply to comment #28) > Alain > I just looked at your patch and the changes show there. > When I went to apply it they did not get applied, so I > created my own. > I like the idea of a per render list. But I also did it > the way I did to have a list which comes back to life > when you close the view and reoopen it. > Feel free to change my patch to include your ideas, but > try and make sure to have a persistense when the view > goes away. > Also you asked about a null pointer removing everything. > No issue changing that either. But there must be a way > to indicate clearing the list for the clear expressions > menu selection I created. > Randy Randy, I will let you check in your changes first and I will make these additonal changes when I make the changes to support Memory Space. Most likely, I will need to keep a list per rendering per memory space anyways. Created attachment 188477 [details]
Update version with John Cortel suggestions about a FIFO and some code cleanup
Update version with John Cortel suggestions about a FIFO and some code cleanup
Folks I checked these changes in to HEAD after making some cleanup changes and functional changes to the code suggested by John Cortel. John please review if you wish. Alain, you can go ahead and work further on it. Randy (In reply to comment #31) > Folks > > I checked these changes in to HEAD after making some cleanup changes > and functional changes to the code suggested by John Cortel. > > John please review if you wish. > > Alain, you can go ahead and work further on it. > > Randy Thanks for addressing my comments. The only concern I have with the latest change is that with the cap. 30 seems rather high. And since the cap is per memory space, if a user is working with two memory spaces, that's a history that's 60 elements long! I think a reasonable cap is 10, and I don't think it should be per memory space. It should be 10, period. (In reply to comment #32) > (In reply to comment #31) > > Folks > > > > I checked these changes in to HEAD after making some cleanup changes > > and functional changes to the code suggested by John Cortel. > > > > John please review if you wish. > > > > Alain, you can go ahead and work further on it. > > > > Randy > Thanks for addressing my comments. The only concern I have with the latest > change > is that with the cap. 30 seems rather high. And since the cap is per memory > space, if a user is working with two memory spaces, that's a history that's 60 > elements long! I think a reasonable cap is 10, and I don't think it should be > per memory space. It should be 10, period. I think our customers prefer one history per memory space. I agree that 30 per space may be too much. Let's stick with one history for now. If our customers really want one history per space, we can deal with it later. > I think our customers prefer one history per memory space. I agree that 30 per
> space may be too much. Let's stick with one history for now. If our customers
> really want one history per space, we can deal with it later.
Cool. Can you handle this change in the patch I suspect you'll be contributing to actually support memory spaces? (Both reducing it to a flat history and setting the cap at 10)
(In reply to comment #34) > > I think our customers prefer one history per memory space. I agree that 30 per > > space may be too much. Let's stick with one history for now. If our customers > > really want one history per space, we can deal with it later. > Cool. Can you handle this change in the patch I suspect you'll be contributing > to actually support memory spaces? (Both reducing it to a flat history and > setting the cap at 10) Sure, I will take of these. Thanks Randy and John for your help. |