Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 345974

Summary: [expr] Changing an expression name in the expression view should not accept an empty expression
Product: [Eclipse Project] Platform Reporter: Winnie Lai <wlai>
Component: DebugAssignee: Platform-Debug-Inbox <platform-debug-inbox>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: Michael_Rennie, pawel.1.piech, pwebster
Version: 3.7   
Target Milestone: 3.8 M5   
Hardware: PC   
OS: All   
Whiteboard:
Attachments:
Description Flags
patch
pawel.1.piech: iplog+
cdi gdb screenshot
none
dsf gdb screenshot none

Description Winnie Lai CLA 2011-05-16 12:02:09 EDT 
Build Identifier: 3.7.0  I20110428-0848

In the expressions view, it is possible to click on an expression and edit it.
If the user puts in a blank expression, the expression should be removed instead of leaving a blank expression inside the view. The reason is to reflect the same logic in ExpressionManagerContentProvider.AddNewExpressionElement.modify that checks for an empty string and allows only non-empty string being added to expression manager.


Reproducible: Always

Steps to Reproduce:
1.Launch eclispe without any debugger.
2. Add an non-empty expression
3. Edit the expression from (2) to either make it an empty string or a string full with blanks only.

I expect step 3 will remove the expression rather than the view keeps the expression with blanks or keeps the empty string.
Comment 1 Winnie Lai CLA 2011-05-16 12:07:25 EDT 
Created attachment 195754 [details]
patch

Please review this patch and include it in 3.7 RC2 or 3.7.0.
Comment 2 Pawel Piech CLA 2011-05-18 11:15:10 EDT 
IMO the safer thing to do would be to revert the expression back to what it was before the edit, rather than removing it.  
I don't know if the bug is severe enough to target for RC2, I'd rather leave it for 3.8, or 3.7.1.  

Opinions?
Comment 3 Michael Rennie CLA 2011-05-18 11:45:02 EDT 
+1 for 3.8, like you say, its not critical or blocking anyone.
Comment 4 Winnie Lai CLA 2011-05-18 11:56:39 EDT 
(In reply to comment #3)
> +1 for 3.8, like you say, its not critical or blocking anyone.

(In reply to comment #2)
> IMO the safer thing to do would be to revert the expression back to what it was
> before the edit, rather than removing it.  
> I don't know if the bug is severe enough to target for RC2, I'd rather leave it
> for 3.8, or 3.7.1.  
> Opinions?

I double-verify that,
a) if user press ESC button while modifying, the modify method is not called.
b) The only time for end-user to apply the change is when ENTER button is pressed and so the modify method is invoked.
Thus, I don't think it should revert to the previous string because the user does not intend to cancel the change -- the user intends to apply the change.
Comment 5 Winnie Lai CLA 2011-05-18 12:42:22 EDT 
(In reply to comment #3)
> +1 for 3.8, like you say, its not critical or blocking anyone.

This bug is to reflect the need of consistency of ExpressionManagerContentProvider.AddNewExpressionElement.modify. Also, if an empty string is left in the view while there are no debuggers, its impact depends on how individual debuggers handle it when they become active, for e.g.,
 - java debugger handles it by always quoting each of the variables;
 - cdi gdb shows an error [see cdi_gdb screenshot];
 - dsf gdb has a blank row (no name, no errors) and the "Add new expression" is shifted below the row [see dsf_gdb screenshot].
When dsf gdb becomes active and user clicks on the empty string, the expression gets removed, which is a result of Bug 233111 addressed by Pawel.

This is not a fatal defect. +1 for 4.7.1.
Comment 6 Winnie Lai CLA 2011-05-18 12:58:20 EDT 
Created attachment 196004 [details]
cdi gdb screenshot
Comment 7 Winnie Lai CLA 2011-05-18 13:05:47 EDT 
Created attachment 196007 [details]
dsf gdb screenshot
Comment 8 Pawel Piech CLA 2011-05-18 13:30:39 EDT 
(In reply to comment #5)
> When dsf gdb becomes active and user clicks on the empty string, the expression
> gets removed, which is a result of Bug 233111 addressed by Pawel.

I guess I contradicted myself :-o  It's funny how much more conservative I think in context of the platform project.  But stepping back to my former self, I guess removing the expression is more useful.
Comment 9 Winnie Lai CLA 2012-01-06 10:35:58 EST 
Hello Pawel,

Do you think you can review this patch and get it committed to 3.8 whichever the earliest milestone?

Thanks,
Winnie


(In reply to comment #8)
> (In reply to comment #5)
> > When dsf gdb becomes active and user clicks on the empty string, the expression
> > gets removed, which is a result of Bug 233111 addressed by Pawel.
> I guess I contradicted myself :-o  It's funny how much more conservative I
> think in context of the platform project.  But stepping back to my former self,
> I guess removing the expression is more useful.
Comment 10 Pawel Piech CLA 2012-01-10 14:15:44 EST 
(In reply to comment #9)
> Hello Pawel,
> 
> Do you think you can review this patch and get it committed to 3.8 whichever
> the earliest milestone?

Sure.  Thank you for bring it up.
Comment 11 Pawel Piech CLA 2012-01-10 17:09:29 EST 
I pushed the fix.  Thank you for the contribution.

http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=6f4ad90bdd669f6e4a0b1313240a75c5294757b8