Bugzilla – Full Text Bug Listing
| Summary: | [Memory Browser] do not create a rendering automatically when Memory Browser is opened | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 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: | normal | ||||||||||||||||||
| Priority: | P3 | CC: | john.cortell, Randy.Rohrbach, teodor.madan | ||||||||||||||||
| Version: | 7.0 | Flags: | john.cortell:
review+
|
||||||||||||||||
| Target Milestone: | --- | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Alain Lee
Created attachment 167679 [details]
Proposed fix
The attached patch addresses the following issues:
- Do not create rendering to any default address (including "0x0").
- User is required to enter an expression before rendering is created
Other improvements is to add text field decorators to:
* suggest to user to enter an expression in order to position the rendering
* add error decoration when entered expression is invalid
Also another change is to disable "Go" and "New Tab" for empty expression. I have attached screen shots with the new decoration reflecting different states of MemoryBrowser Created attachment 167680 [details]
Snapshot with initial state of MemoryBrowser
Created attachment 167681 [details]
Invalid expression decoration
The proposed fix should be able to fulfill our requirement. I have tried this patch out and I really like it. We have reported issues when bringing up the memory browser trying to access illegal memory ( 0 is not always real ). This makes this work better. We have also seen issues in our product when disconnecting, when there are no longer valid connections the view would still be trying to access 0 and fail. Generating failed request which are logged. Unless I find something, I intend to apply this patch to MAIN and perhaps to the 6.0.x branch for our older products. Randy I have tried this patch out and I really like it. We have reported issues when bringing up the memory browser trying to access illegal memory ( 0 is not always real ). This makes this work better. We have also seen issues in our product when disconnecting, when there are no longer valid connections the view would still be trying to access 0 and fail. Generating failed request which are logged. Unless I find something, I intend to apply this patch to MAIN and perhaps to the 6.0.x branch for our older products. Randy Created attachment 168643 [details]
Updated patch which recreates Teodors original patch on top of John Cortel's changes
I reapplied by hand the edits from teodor's original patch on top of John Cortel's changes ( which were massive and in many of the same areas ).
Randy
I have tested the reapplied changes from Teodor's patch and it seems as stable as before. So I am going to apply it. Randy I applied the re-edited patch. John please review. Thanks Randy Cool improvement. Some monitor comments... 1. Why is the DEC_REQUIRED center-left but the DEC_ERROR top-left? It would be nice if they were consistently placed given they're mutually exclusive. 2. The description text for DEC_REQUIRED should come from a properties file, not hard-coded in the code. 3. I think the DEC_REQUIRED messages should just be "Enter an expression". The "to position renderings" seems at best unnecessary, but more likely outright confusing. 4. Formatting in the code needs some improvement. The following is both ugly and unorthodox. fieldDecoration = FieldDecorationRegistry.getDefault() .getFieldDecoration(FieldDecorationRegistry.DEC_ERROR); If you split a line like this, the second part should be indented (preferably by two tabs). 5. The value of clearError() is questionable. I think the code would be clearer if it just called fWrongExpression.hide() Created attachment 168813 [details] Fix regression to memory space support. I gave the latest changes a run with the memory space support active, and found out the new logic broke the memory space support. Basically, the comobox for selecting the memory spaces isn't show up. It was easy to fix, though. Also, I fixed the fact that the goto field showed up indented even when the memory spaces widget was hidden. Committed to HEAD. Randy, feel free to review this if you like, but it's really just an adjustment to the code I added via bug # 309032. Created attachment 168975 [details]
Externalize String fix
Patch addresses:
1)code formatting
2)non-externalization string and the suggested message for DEC_REQUIRED
(In reply to comment #11) > Cool improvement. Some monitor comments... > > 1. Why is the DEC_REQUIRED center-left but the DEC_ERROR top-left? It would be > nice if they were consistently placed given they're mutually exclusive. Though the decorators are mutually exclusive, these are the places I would expect to see them. i.e. for a hint of mandatory field to enter usually the decorator is position in the middle of the field. Also the errors are usually placed on top corner. > > 5. The value of clearError() is questionable. I think the code would be clearer > if it just called fWrongExpression.hide() The idea was that clearing of error is done from multiple places (e.g. when user starts typing), this would localize eventual changes to decide the error presentation changes. > > 5. The value of clearError() is questionable. I think the code would be clearer
> > if it just called fWrongExpression.hide()
>
> The idea was that clearing of error is done from multiple places (e.g. when
> user starts typing), this would localize eventual changes to decide the error
> presentation changes.
Then you would equally want to abstract the *setting* of the error. Basically, the following method is awkward given that clearError() simply manipulates fWrongExpression. One can go either way with this...remove clearError() or add a setError(...) method. It doesn't matter to me which. I'm just hoping for consistency.
public void handleExpressionStatus(final IStatus message) {
if (message.isOK()) {
clearError();
} else {
fWrongExpression.setDescriptionText(message.getMessage());
fWrongExpression.show();
}
}
Randy, you have not responded to or addressed the issues I brought up. OK, I updated the code to be consistent. I took out the clearError() routine. That is the style I like better. Randy (In reply to comment #17) > OK, I updated the code to be consistent. > > I took out the clearError() routine. That is the style I like better. > > Randy That addresses issue 5 only. 2, 3 and 4 are still relevant (Teo provided a justification for 1). John OK, Sorry for going to fast. I'll look at the other issues. Randy Created attachment 170720 [details]
Took Teodor patch and cleaned it up a little.
John I have applied the patches/changes from Teodor and myself. I think this addresses all of the issues you had. Randy (In reply to comment #21) > I have applied the patches/changes from Teodor and myself. I think > this addresses all of the issues you had. Yep. It does. Thanks! *** cdt cvs genie on behalf of jcortell *** Bug 312098: do not create a rendering automatically when Memory Browser is opened (fixed regression to memory space support) [*] MemoryBrowser.java 1.23 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/memory/org.eclipse.cdt.debug.ui.memory.memorybrowser/src/org/eclipse/cdt/debug/ui/memory/memorybrowser/MemoryBrowser.java?root=Tools_Project&r1=1.22&r2=1.23 |