Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 334991 - Variables view: Add history with recent expressions to the details pane
Summary: Variables view: Add history with recent expressions to the details pane
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Debug (show other bugs)
Version: 3.7   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 4.11 RC1   Edit
Assignee: Sarika Sinha CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 498469
  Show dependency tree
 
Reported: 2011-01-21 05:35 EST by Markus Keller CLA
Modified: 2019-03-31 10:35 EDT (History)
8 users (show)

See Also:
daniel_megert: pmc_approved+
Vikas.Chandra: review+


Attachments
Read only hover with combo and "wrong" value selected from it (266.12 KB, image/png)
2019-01-26 03:37 EST, Andrey Loskutov CLA
no flags Details
Selected variable shows nonsense value proposed by combo (269.13 KB, image/png)
2019-01-26 03:38 EST, Andrey Loskutov CLA
no flags Details
To narrow list with nonsense values from previously *selected* variables (264.08 KB, image/png)
2019-01-26 03:40 EST, Andrey Loskutov CLA
no flags Details
Save changes in Variables? (242.93 KB, image/png)
2019-01-26 13:28 EST, Andrey Loskutov CLA
no flags Details
light bulb in staging view (17.86 KB, image/png)
2019-01-28 02:17 EST, Andrey Loskutov CLA
no flags Details
combo issues (130.53 KB, image/png)
2019-01-31 03:19 EST, Andrey Loskutov CLA
no flags Details
works with not editable values (131.71 KB, image/png)
2019-01-31 03:20 EST, Andrey Loskutov CLA
no flags Details
shows some recently used selection in unrelated context (131.42 KB, image/png)
2019-01-31 03:21 EST, Andrey Loskutov CLA
no flags Details
Unusable combo with meaningless proposals (245.60 KB, image/png)
2019-02-19 15:49 EST, Andrey Loskutov CLA
no flags Details
Another attempt to use it (403.84 KB, image/png)
2019-02-20 06:50 EST, Andrey Loskutov CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2011-01-21 05:35:07 EST
HEAD

I frequently use the expression evaluation support in the details pane of the Variables view. Often, I need to inspect the same expression multiple times in a row, or I need to apply a small change.

This is quite cumbersome now, since the expression is gone as soon as I choose Inspect or Display. A history popup with recent expressions would be very handy.
Comment 1 Dani Megert CLA 2011-01-21 05:45:57 EST
One for me!
Comment 2 Markus Keller CLA 2016-07-25 10:29:12 EDT
The conditional breakpoints editor already shows a similar history with recent expressions.

In the Variables view, the combo label would also be a good place to advertise the existence of the integrated "Display view" functionality in that pane. I guess the hardest part of this bug will be to come up with a good label.
Comment 3 Sarika Sinha CLA 2016-07-26 20:31:42 EDT
This change request emerged based on a scientific study on how developers debug, in discussion with Daniel Megert, Markus Keller, Sarika Sinha, and Moritz Beller.
Link to the study  -
http://repository.tudelft.nl/islandora/object/uuid%3Abf3325ce-f246-4977-91bc-785f877347b8?collection=education
Comment 4 Sarika Sinha CLA 2018-04-17 00:57:47 EDT
Moving out.
Comment 5 Sarika Sinha CLA 2018-11-19 04:22:03 EST
Analysis done, the support has to go in Platform Debug as the Details pane is defined in the org.eclipse.debug.internal.ui.views.variables.VariablesView.

JDT/CDT or any implementer can define a property to say it needs the history combo. 
Behavior of combo will be similar to condition history combo.

Details about Combo -
Description - Choose a previously entered expression to assign and perform other actions 
Combo Entries - 
1) the value of variable
2) Other expressions used for this variable
3) Global History with expressions used for other variables

Suggestion for better Description is welcome. As CDT has very limited actions, can not go int detail. Will see if description can be specific to JDT which will give more words.
Comment 6 Eclipse Genie CLA 2019-01-21 06:07:07 EST
New Gerrit change created: https://git.eclipse.org/r/135468
Comment 7 Eclipse Genie CLA 2019-01-22 03:39:40 EST
New Gerrit change created: https://git.eclipse.org/r/135536
Comment 10 Andrey Loskutov CLA 2019-01-26 03:22:58 EST
(In reply to Sarika Sinha from comment #5)
> Behavior of combo will be similar to condition history combo.
> 
> Details about Combo -
> Description - Choose a previously entered expression to assign and perform
> other actions 
> Combo Entries - 
> 1) the value of variable
> 2) Other expressions used for this variable
> 3) Global History with expressions used for other variables

I've just was surprised by this feature and was curious to test it out. However it went bad.

I must confess, this way how it is implemented right now it is more distracting than helping, and I think this must be fixed or reverted. 

My biggest issue is that it seems to automatically remember all *selected* values and adds them to the "proposal" list. So while a short debugging session just selecting variables fills the list with nonsense "proposals". After that, one looses an overview *what* is the *actual* value if one selects something in the list.

Second: if the variables are on the right stack, the combo and the proposal list is to narrow to be useful, can't be resized and so can't be fully read / used (if it would contain real expressions).

Third: it allows to select those nonsense values to absolutely unrelated variables of different types and does not show that the value is actually not applied yet. There is no indication that the text in the detail pane is need to be "saved" first. I know this was like this before, but now it is really confusing.
Comment 11 Andrey Loskutov CLA 2019-01-26 03:32:56 EST
Also this combo appears now in the debug hover over variables? And allows me to select some value? But this makes no sense at all, one can't "apply" this value in the debug hover, there is no "Ctrl+S" or other way to do this, it is read-only..
Comment 12 Andrey Loskutov CLA 2019-01-26 03:37:10 EST
Created attachment 277300 [details]
Read only hover with combo and "wrong" value selected from it
Comment 13 Andrey Loskutov CLA 2019-01-26 03:38:08 EST
Created attachment 277301 [details]
Selected variable shows nonsense value proposed by combo
Comment 14 Andrey Loskutov CLA 2019-01-26 03:40:16 EST
Created attachment 277302 [details]
To narrow list with nonsense values from previously *selected* variables
Comment 15 Andrey Loskutov CLA 2019-01-26 13:28:55 EST
Created attachment 277306 [details]
Save changes in Variables?

It also looks like the change caused another strange regression: after debug session, on committing a change in Git Staging view, a "Save changes in Variables?" dialog appears, and disappears only if Variables view is closed. There is NOTHING in Variables view needed to be saved at this moment.

I think we should revert the change.
Comment 16 Sarika Sinha CLA 2019-01-28 00:54:02 EST
Thanks Andrey for trying out!!
I am looking into the issues, based on the results we can decide how to proceed.
Comment 17 Andrey Loskutov CLA 2019-01-28 02:05:49 EST
Sarika, from my feeling, a combo between panes doesn'T fit & look good. WDYT about a content assist, like in the EGit Staging view, with the light bulb etc? This would fit for any view size and will not confuse.
Comment 18 Sarika Sinha CLA 2019-01-28 02:14:20 EST
We had content assist in Detail pane of variables view but not many people were aware about it, so we also need to handle the discoverability part also.

Where do you suggest the light bulb ?
Comment 19 Andrey Loskutov CLA 2019-01-28 02:17:54 EST
Created attachment 277326 [details]
light bulb in staging view
Comment 20 Sarika Sinha CLA 2019-01-28 04:00:31 EST
(In reply to Andrey Loskutov from comment #19)
> Created attachment 277326 [details]
> light bulb in staging view

This can give the tip that the Detail pane has content assist, but how do we help the user in remembering the older values ?

By the way we have local history and global history like conditions in the combo.
Comment 21 Andrey Loskutov CLA 2019-01-28 04:04:45 EST
(In reply to Sarika Sinha from comment #20)
> (In reply to Andrey Loskutov from comment #19)
> > Created attachment 277326 [details]
> > light bulb in staging view
> 
> This can give the tip that the Detail pane has content assist, but how do we
> help the user in remembering the older values ?

Automatically once Ctrl+S is executed we apply the condition and save it if it was applied without errors.
Comment 22 Dani Megert CLA 2019-01-28 04:30:45 EST
(In reply to Andrey Loskutov from comment #11)
> Also this combo appears now in the debug hover over variables? And allows me
> to select some value? But this makes no sense at all, one can't "apply" this
> value in the debug hover, there is no "Ctrl+S" or other way to do this, it
> is read-only..

-1 for having a combo in the hover.
Comment 23 Dani Megert CLA 2019-01-28 04:32:43 EST
(In reply to Andrey Loskutov from comment #19)
> Created attachment 277326 [details]
> light bulb in staging view

We also show this e.g. in the Find/Replace dialog when Regex is enabled.
Comment 24 Sarika Sinha CLA 2019-01-28 07:43:37 EST
I am working on disabling the combo from hover.

Yes, the light bulb will help.
Comment 25 Eclipse Genie CLA 2019-01-29 06:25:45 EST
New Gerrit change created: https://git.eclipse.org/r/135927
Comment 26 Sarika Sinha CLA 2019-01-29 09:51:54 EST
(In reply to Eclipse Genie from comment #25)
> New Gerrit change created: https://git.eclipse.org/r/135927

Removes the combo from Hover.

I am seeing following error in the gerrit build tests -
16:59:54 	at org.eclipse.swt.SWT.error(SWT.java:4621)
16:59:54 	at org.eclipse.swt.SWT.error(SWT.java:4510)
16:59:54 	at org.eclipse.swt.SWT.error(SWT.java:4481)
16:59:54 	at org.eclipse.swt.graphics.Cursor.<init>(Cursor.java:185)
16:59:54 	at org.eclipse.swt.widgets.Display.getSystemCursor(Display.java:3009)
16:59:54 	at org.eclipse.swt.custom.StyledText.<init>(StyledText.java:1247)
16:59:54 	at org.eclipse.jface.text.TextViewer.createTextWidget(TextViewer.java:1676)
16:59:54 	at org.eclipse.jface.text.TextViewer.createControl(TextViewer.java:1699)
16:59:54 	at org.eclipse.jface.text.source.SourceViewer.createControl(SourceViewer.java:439)
16:59:54 	at org.eclipse.jface.text.source.SourceViewer.<init>(SourceViewer.java:426)
16:59:54 	at org.eclipse.jface.text.source.SourceViewer.<init>(SourceViewer.java:402)
16:59:54 	at org.eclipse.ui.console.TextConsoleViewer.<init>(TextConsoleViewer.java:237)
16:59:54 	at org.eclipse.ui.console.TextConsoleViewer.<init>(TextConsoleViewer.java:224)
16:59:54 	at org.eclipse.ui.internal.console.IOConsoleViewer.<init>(IOConsoleViewer.java:59)
16:59:54 	at org.eclipse.ui.internal.console.IOConsolePage.createViewer(IOConsolePage.java:73)
16:59:54 	at org.eclipse.ui.console.TextConsolePage.createControl(TextConsolePage.java:153)
16:59:54 	at org.eclipse.ui.internal.console.IOConsolePage.createControl(IOConsolePage.java:64)
16:59:54 	at org.eclipse.ui.internal.console.ConsoleView.doCreatePage(ConsoleView.java:326)
16:59:54 	at org.eclipse.ui.part.PageBookView.createPage(PageBookView.java:378)
16:59:54 	at org.eclipse.ui.part.PageBookView.partActivated(PageBookView.java:718)
16:59:54 	at org.eclipse.ui.internal.console.ConsoleView.lambda$0(ConsoleView.java:412)
16:59:54 	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
16:59:54 	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:185)


And the build time outs -
16:59:54 	... 46 more
17:17:50 Waiting for evaluation to start..
18:06:04 Build timed out (after 70 minutes). Marking the build as aborted.
Comment 27 Sarika Sinha CLA 2019-01-29 10:13:22 EST
And the build locally passes fine on Windows 7.
@Andrey,
Can you try on linux locally?
Comment 28 Andrey Loskutov CLA 2019-01-29 11:38:23 EST
(In reply to Sarika Sinha from comment #27)
> And the build locally passes fine on Windows 7.
> @Andrey,
> Can you try on linux locally?

I observe lot of NPE's on Linux if running the code in debugger. I've highlighted the locations of those NPE's in the gerrit. Creation and closing of the debug hover is the problem. No idea why it does not fail on Windows.
Comment 30 Andrey Loskutov CLA 2019-01-31 03:11:39 EST
(In reply to Andrey Loskutov from comment #10)
> I must confess, this way how it is implemented right now it is more
> distracting than helping, and I think this must be fixed or reverted. 

I'm still fighting with the current state. It is not usable from my opinion.

> My biggest issue is that it seems to automatically remember all *selected*
> values and adds them to the "proposal" list. 

OK, now it "only" remembers if user *clicked/focused* the text area. Still it makes no sense to do this "on click", it should be "on save".

> So while a short debugging
> session just selecting variables fills the list with nonsense "proposals".

So for example if one wants to select and copy/paste some value from ""this" referenced from" entry, the history automatically remembers the noncense value of "72 references: ...".

> After that, one looses an overview *what* is the *actual* value if one
> selects something in the list.

The irritating thing is also, that after using combo to select one element, the same selected value is shown twice: once in the combo and once in the details area below. It makes no sense.

> Second: if the variables are on the right stack, the combo and the proposal
> list is to narrow to be useful, can't be resized and so can't be fully read
> / used (if it would contain real expressions).

At least on Windows this proposal is scrollable now, but still can't be resized. This is unusable.
 
> Third: it allows to select those nonsense values to absolutely unrelated
> variables of different types and does not show that the value is actually
> not applied yet. There is no indication that the text in the detail pane is
> need to be "saved" first. I know this was like this before, but now it is
> really confusing.

Still there.

I will attach some pictures with comments inside where I found strange results.

Beside the already mentioned combo limitations (size can't be changed, to narrow, too alien), I have also problems to understand how the entry list and "global history" are computed:

- The current value is always shown as "previously selected" in the list.
- Not sure what "Global history" is doing - it just contains some *but not all* previously selected entries. What is the filter criteria?
- "Global history" contains same elements multiple times.

In general, I think this can be fixed by:

1) Remove combo box and use the "light bulb" + content assist instead. Combo is the wrong UI approach here and causes major usability issues.
2) Restrict this "light bulb" to work only with *really* editable entries. So do not offer this on "references" or "no method return value" elements.
3) Do not remember on selection or focus events, remember on "apply". Only if user actively changes the value, we know it was an expression worth to be remembered. Just focusing/selecting something is not an indicabtion the value is an expression  entered/changed by user.
4) Remembered elements should be filtered to not have duplicates.
5) Rework (and may be rename / hide) the "Global History". It is not clear why there should be a difference between "local" history and "global", and what this "global" is good for. Offer a way to cleanup this history from nonsense values, or restrict the "memory" to 10 last elements, so that the older will just disappear some time.
Comment 31 Andrey Loskutov CLA 2019-01-31 03:19:56 EST
Created attachment 277393 [details]
combo issues
Comment 32 Andrey Loskutov CLA 2019-01-31 03:20:30 EST
Created attachment 277394 [details]
works with not editable values
Comment 33 Andrey Loskutov CLA 2019-01-31 03:21:08 EST
Created attachment 277395 [details]
shows some recently used selection in unrelated context
Comment 34 Sarika Sinha CLA 2019-02-04 03:11:58 EST
I will go through the comments and get back within couple of days. As far as I understood -
1. Resizing problem is no different from breakpoints view.
2. I will investigate on Global history
3. We can add the original value in the list if required - intention was to show the values which user had added.
Comment 35 Dani Megert CLA 2019-02-07 10:22:21 EST
I looked at this. The general direction is OK.

Let's keep in mind that the introduced combo is not new. It is exactly the same that we have since 8 years in the Breakpoints view.

I agree to the following issues that we have in both views and they should be fixed for both at the same time:
- Can't resize the combo value window (new size should be persisted) ==> bug 544232.
- Should not be able to choose the global history entry (like in Open Type) ==> bug 544234.

Andrey, the global history is what's shared between open Variables/Breakpoints views. If it is not correct, a separate bug with steps should be filed.

Some bugs I found in the Variables view:
- "for debugging" should be removed from the combo text.
- On save (Ctrl+S) it should reset the combo text to <Choose.... (like in the Breakpoints view).
- The history also seems to remember things that aren't entered by the user. Here's one example:
1. Start new workspace and paste the following into the Package Explorer:

public class T {
	public static void main(String[] args) {
		
		int i= 0;
		
		System.out.println(i++);
		System.out.println(i++);
		System.out.println(++i);
	}
}
2. Set a breakpoint on the secons sysout
3. Start debugging and allow perspective switch
4. Click on 'i' in the Variables view
5. Click on 'args' in the Variables view
==> detail formatter error is shown and remembered in the history.


Filed unrelated bug report:
Bug 544228: Variables view: content assist hint (light bulb) missing
Comment 36 Dani Megert CLA 2019-02-08 08:14:09 EST
(In reply to Dani Megert from comment #35)
> - The history also seems to remember things that aren't entered by the user.
> Here's one example:
> ...
Sorry, that example is not enough to reproduce the problem reliably, but I was able to see the issue again today and also Sarika could see it. She will post better steps.
Comment 37 Sarika Sinha CLA 2019-02-11 01:50:06 EST
Global History - It stores the history across all variables (breakpoints have a separate global history)

I think the main problem is that right now it adds the history on "FocusLost" which is adding unintended items in the drop down. Conditional breakpoint there is no default string coming from any where so save on focus lost doesn't create problem. Points to consider -
1. Like Ctrl + S, provide another shortcut and context menu to add items to the combo.
2. We can not rely on user's typing as user can select a value from Global history.
3. We can check with the original value of the variable and save only if the value has changed 

@Dani, @Andrey
Let me know your thoughts !
Comment 38 Andrey Loskutov CLA 2019-02-11 02:48:36 EST
(In reply to Sarika Sinha from comment #37)
> Global History - It stores the history across all variables (breakpoints
> have a separate global history)
> 
> I think the main problem is that right now it adds the history on
> "FocusLost" which is adding unintended items in the drop down. Conditional
> breakpoint there is no default string coming from any where so save on focus
> lost doesn't create problem. Points to consider -
> 1. Like Ctrl + S, provide another shortcut and context menu to add items to
> the combo.

Why not use Ctrl+S as a trigger for this? It is exact the time where user decided to apply some change. Or is the code shared between breakpoint edit dialog and variables view? I can't remember Ctrl+S works in the breakpoint edit dialog? If this is the problem, we should be able to know in which context we are right now and decide.

> 2. We can not rely on user's typing as user can select a value from Global
> history.

I didn't get this.

> 3. We can check with the original value of the variable and save only if the
> value has changed 

This makes sense in any case.
Comment 39 Sarika Sinha CLA 2019-02-11 03:02:55 EST
> We can not rely on user's typing as user can select a value from Global
> history.

This was for the case if we wanted to add the values only if user typed a new value, but a user can choose a new value from the global history ( value of different variable)
Comment 40 Dani Megert CLA 2019-02-11 04:33:26 EST
(In reply to Sarika Sinha from comment #37)
> Global History - It stores the history across all variables (breakpoints
> have a separate global history)
You mean "Variables views", right?
Comment 41 Sarika Sinha CLA 2019-02-11 04:39:54 EST
(In reply to Dani Megert from comment #40)
> (In reply to Sarika Sinha from comment #37)
> > Global History - It stores the history across all variables (breakpoints
> > have a separate global history)
> You mean "Variables views", right?

Yes, Variables view has its own Global history.
Breakpoints view Condition history has it's own Global History.
Comment 42 Sarika Sinha CLA 2019-02-11 04:53:44 EST
If we add the original value of the variable to the combo box, then the error message like detail formatter error will also be added, and the list will lose the purpose of remembering the expression used by the user previously.

If we use Ctrl +S, it will remember the expression used to assign the value and not the expressions which were executed at that moment for debugging.
Comment 43 Andrey Loskutov CLA 2019-02-11 09:30:55 EST
(In reply to Sarika Sinha from comment #42)
> If we add the original value of the variable to the combo box, then the
> error message like detail formatter error will also be added, and the list
> will lose the purpose of remembering the expression used by the user
> previously.
> 
> If we use Ctrl +S, it will remember the expression used to assign the value
> and not the expressions which were executed at that moment for debugging.

I must confess, I've misunderstood the original comment 0 and now after re-reading this again I see that the use case is different from what I had in mind. So not only "assigned" values should be remembered, but all possible evaluations?

So user can write something inside the Variables and run evaluation on it, *without* saving, and this should be remembered? Why? For this use case we have "Debug Shell" view which remembers all the expressions.

But anyway, if we want do this, shouldn't we ask evaluation engine for the list of recent evaluations? 
Something like List<String> IEvaluationEngine.getRecentEvaluations()?
Comment 44 Dani Megert CLA 2019-02-11 12:00:31 EST
Relying on Ctrl+S in the standard scenario is not practical. The user doesn't even know that this is available.

What we have to do is that we don't remember values which haven't been entered by the user, like the error message.

Also, I've seen issues with the global history when multiple Variables views are open and used.
Comment 45 Eclipse Genie CLA 2019-02-12 02:53:38 EST
New Gerrit change created: https://git.eclipse.org/r/136743
Comment 47 Sarika Sinha CLA 2019-02-12 04:02:00 EST
(In reply to Eclipse Genie from comment #45)
> New Gerrit change created: https://git.eclipse.org/r/136743

This takes care of 
- the formatter error or any unwanted items in the history.
- "For debugging" has been removed
- history is consistent with more than 1 variables 

Please try in a new workspace or remove dialog_settings.xml from o.e.jdt.debug.ui metadata .

Note - If there is no local history, only global history items are shown with the label "Global History"
Comment 48 Sarika Sinha CLA 2019-02-14 01:20:12 EST
(In reply to Andrey Loskutov from comment #43)
> (In reply to Sarika Sinha from comment #42)
> > If we add the original value of the variable to the combo box, then the
> > error message like detail formatter error will also be added, and the list
> > will lose the purpose of remembering the expression used by the user
> > previously.
> > 
> > If we use Ctrl +S, it will remember the expression used to assign the value
> > and not the expressions which were executed at that moment for debugging.
> 
> I must confess, I've misunderstood the original comment 0 and now after
> re-reading this again I see that the use case is different from what I had
> in mind. So not only "assigned" values should be remembered, but all
> possible evaluations?
> 
> So user can write something inside the Variables and run evaluation on it,
> *without* saving, and this should be remembered? Why? For this use case we
> have "Debug Shell" view which remembers all the expressions.
> 
> But anyway, if we want do this, shouldn't we ask evaluation engine for the
> list of recent evaluations? 
> Something like List<String> IEvaluationEngine.getRecentEvaluations()?

This bug was prioritized based on the survey related to debugging experience in 2016.  
Something like List<String> IEvaluationEngine.getRecentEvaluations() will give the expressions evaluated across - inspect & display in editor, debug shell or any where.
Comment 49 Eclipse Genie CLA 2019-02-14 01:39:06 EST
New Gerrit change created: https://git.eclipse.org/r/136889
Comment 51 Sarika Sinha CLA 2019-02-19 01:07:14 EST
Verified using
Eclipse SDK

Version: 2019-03 (4.11)
Build id: I20190218-1800
Comment 52 Eclipse Genie CLA 2019-02-19 05:04:53 EST
New Gerrit change created: https://git.eclipse.org/r/137190
Comment 54 Sarika Sinha CLA 2019-02-19 06:33:58 EST
N&N added.
Comment 55 Andrey Loskutov CLA 2019-02-19 15:46:26 EST
I still have only garbage in the combo box and I have *nothing* evaluated at all. I will attach a picture showing the combo as it was after 10 minutes of debugging, without a single evaluation.

I honestly would prefer to revert this change for M3 because I haven't seen anything except confusion so far from trying to use it.
Comment 56 Andrey Loskutov CLA 2019-02-19 15:49:09 EST
Created attachment 277624 [details]
Unusable combo with meaningless proposals
Comment 57 Sarika Sinha CLA 2019-02-19 22:28:23 EST
Are you using a new workspace or deleted the file mentioned earlier? As I said the old workspace might have got cluttered.

Unless you have typed that in detail pane, it will not show up in the combo box.
Comment 58 Andrey Loskutov CLA 2019-02-20 00:45:43 EST
(In reply to Sarika Sinha from comment #57)
> Unless you have typed that in detail pane, it will not show up in the combo
> box.

I haven't typed in the Variables view, only used upper part to select variables to see their values in the details part. The values in the combo seem to be those which I've selected during the debug session. Only thing I remember was that I've used "Display" in the Java editor to see values, but never in the details part of the Variables view.
Comment 59 Dani Megert CLA 2019-02-20 03:52:30 EST
Andrey, please try to provide reproducible steps.
Comment 60 Andrey Loskutov CLA 2019-02-20 06:50:31 EST
Created attachment 277632 [details]
Another attempt to use it

(In reply to Dani Megert from comment #59)
> Andrey, please try to provide reproducible steps.

Just my current debugging session: I've put a breakpoint at line 62 in org.eclipse.jdt.launching.sourcelookup.containers.JavaSourcePathComputer.computeSourceContainers(ILaunchConfiguration, IProgressMonitor) and start debugger from debuggee (I'm validating patch https://git.eclipse.org/r/137257).

Before debugger returns from the method, inspect the content of the getSourceContainers() return - this is an array. Select an array entry there. *Click once in the details pane*, don't select anything, just click. Select another entry. Click in the combo: it shows previously selected value. Funny enough, combo shows *different* proposals list on different selection *on the same* breakpoint/frame. I can't explain why and have no idea why proposals should differ depending on the selected value.

Means: clicking inside the details pane is enough to add the entry to the combo.  This should not happen.
Comment 61 Dani Megert CLA 2019-02-20 09:13:16 EST
(In reply to Andrey Loskutov from comment #60)
> Means: clicking inside the details pane is enough to add the entry to the
> combo.
That doesn't happen for me using eclipse-SDK-I20190219-1800-win32-x86_64.

As said, would be good to have reproducible steps, starting with a new workspace and best captured in a new bug report.
Comment 62 Dani Megert CLA 2019-02-20 09:13:53 EST
(In reply to Dani Megert from comment #61)
> (In reply to Andrey Loskutov from comment #60)
> > Means: clicking inside the details pane is enough to add the entry to the
> > combo.
> That doesn't happen for me using eclipse-SDK-I20190219-1800-win32-x86_64.
Maybe you're not on Windows and it is an OS specific issue?
Comment 63 Sarika Sinha CLA 2019-02-20 09:21:34 EST
I think it happens only for strings.

for String the previous value comes as "oldValue" as it is shown in the Variables view  Value column and that is why it is treating it as different to oldValue.
Comment 64 Dani Megert CLA 2019-02-20 09:36:28 EST
(In reply to Sarika Sinha from comment #63)
> I think it happens only for strings.
> 
> for String the previous value comes as "oldValue" as it is shown in the
> Variables view  Value column and that is why it is treating it as different
> to oldValue.
Indeed with strings I see the issue. It also resulted in [] ending up in the history. And then [] dropped out again. So, this definitely needs a fix for 4.11.

I leave it to you Sarika, whether you reopen this bug or file a new one.
Comment 65 Eclipse Genie CLA 2019-02-20 09:40:11 EST
New Gerrit change created: https://git.eclipse.org/r/137295
Comment 66 Sarika Sinha CLA 2019-02-20 09:41:24 EST
I have the fix, so will fix it by this bug itself.

Thanks Andrey for providing the steps and attachment!
Comment 67 Dani Megert CLA 2019-02-20 09:42:56 EST
(In reply to Sarika Sinha from comment #66)
> I have the fix, so will fix it by this bug itself.
Reopening then.

 
> Thanks Andrey for providing the steps and attachment!
Yes, thanks!
Comment 69 Dani Megert CLA 2019-02-20 11:24:54 EST
(In reply to Eclipse Genie from comment #68)
> Gerrit change https://git.eclipse.org/r/137295 was merged to [master].
> Commit:
> http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=e0d2c6bdf3adc29a192c52aec550ef0d9b468ab1
> 
This fix is not complete. Still stores e.g. String arrays, e.g. [1, 2].
Comment 70 Dani Megert CLA 2019-02-20 11:26:56 EST
(In reply to Dani Megert from comment #69)
> (In reply to Eclipse Genie from comment #68)
> > Gerrit change https://git.eclipse.org/r/137295 was merged to [master].
> > Commit:
> > http://git.eclipse.org/c/jdt/eclipse.jdt.debug.git/commit/?id=e0d2c6bdf3adc29a192c52aec550ef0d9b468ab1
> > 
> This fix is not complete. Still stores e.g. String arrays, e.g. [1, 2].
Same problem with int arrays and probably all other arrays.
Comment 71 Dani Megert CLA 2019-02-20 11:44:34 EST
I don't understand the latest fix. I would expect a fix that ensures that there is never a "save" just when clicking. We must only save when the user actively types in the details pane, as you said yourself in comment 57.
Comment 72 Eclipse Genie CLA 2019-02-20 22:38:02 EST
New Gerrit change created: https://git.eclipse.org/r/137329
Comment 73 Sarika Sinha CLA 2019-02-20 22:58:46 EST
(In reply to Dani Megert from comment #71)
> I don't understand the latest fix. I would expect a fix that ensures that
> there is never a "save" just when clicking. We must only save when the user
> actively types in the details pane, as you said yourself in comment 57.

I had tried before but ModifyListener sends event for each letter modification  and we don't want the bits and pieces in the combo values.
but now I think it looks ok with the combination of modify and my previous solution.

New gerrit take care of adding to the list only if modified.
Comment 74 Dani Megert CLA 2019-02-21 05:06:37 EST
(In reply to Sarika Sinha from comment #73)
> (In reply to Dani Megert from comment #71)
> > I don't understand the latest fix. I would expect a fix that ensures that
> > there is never a "save" just when clicking. We must only save when the user
> > actively types in the details pane, as you said yourself in comment 57.
> 
> I had tried before but ModifyListener sends event for each letter
> modification  and we don't want the bits and pieces in the combo values.
> but now I think it looks ok with the combination of modify and my previous
> solution.
> 
> New gerrit take care of adding to the list only if modified.
Tried it out and seems to work so far.
Comment 75 Dani Megert CLA 2019-02-22 04:05:07 EST
Andrey, can you also test the proposed fix? Thanks.
Comment 76 Andrey Loskutov CLA 2019-02-22 10:49:13 EST
(In reply to Dani Megert from comment #75)
> Andrey, can you also test the proposed fix? Thanks.

You probably mean patch https://git.eclipse.org/r/137329.

The good news is that it seem not to add "unrelated" selections to the history, almost. I've again managed to push some not edited values into the history, but only once, can't reproduce yet. So I assume the biggest issue I had before is almost gone.

Still there are number of usability issues I personally have with the feature.

1) I see that sometimes selected combo values aren't applied. I have "a", "b", "c" elements in the combo. Select "a" in the combo => text is set. *Delete* text from the details pane and select "a" again => no effect at all. Looks like the value is applied only if the combo selection changes. It looks also that at some point in time the inserted values do NOT match selected, at least it happened to me twice, but I could not understand the preconditions for that. I've also managed to have same proposals multiple times in the "local" history. Here too, no idea how to reproduce.

2) I still can't really understand why do we have global/local history differentiation, especially because the current implementation leaks all "remembered" variables for the entire session to implement this. 

3) The global history confuses me - it always contains "local" elements twice (once in the "local" and once in the "global" history). It would probably make sense if one would run evaluation on different variables multiple times, but then the "global" history should not contain "local" entries. So I don't see a value to have "local" history if it is duplicated in the global one.

4) Most "remembered" "global" expressions can't be evaluated in the context of another variables/frames (an error appears trying to do so). So the usefulness of the global history is questionable in context of Variables view. This is different to the Breakpoints view, where we don't have "current" frame and don't evaluate immediately.

5) 3+4 together requirements are conflicting. Also the "global" history is not shared between Variables and Breakpoints views, so it is not "global" in that sense.

6) If I select "a" in the combo, I see the "a" in the details area and in the combo, the text "<Choose a previously selected expression>" disappears. It looks strange at least, to see "a" twice.

7) After selection of the value the value is not applied. So selecting a value, resize the view => the new value is replaced with the real value of the variable. This is surprising at least.

8) Related to the previous point: if I change the value of conditional breakpoint condition, the Breakpoints view shows asterisk "*" to indicate a "save" is needed. This is not the case in the Variables view. It is not introduced in this bug, but it is just another inconsistency we will expose with the combo.

9) Long values make the combo more or less unusable in the default Variables view position (right stack next to the editor area). Selection of values from such combo is not nice UI experience.

10) On save (Ctrl+S) the combo text is not reset to <Choose... (like in the Breakpoints view).

Summary: I'm not convinced how UI works. I don't like the combo. I don't understand the use model (in the context of Variables view, especially with the differentiation of global/local history). I believe the entire feature is not usable for the average user and also does not provide a value for most of the users. It just doesn't feel "right" and makes the debugger UI more complex.

I personally believe that we should not ship this feature to the end users in 4.11 and remove (or hide) in RC1.
Comment 77 Dani Megert CLA 2019-02-22 11:02:31 EST
(In reply to Andrey Loskutov from comment #76)
I did not yet go through your items, but are you aware that we have exactly the same in the Breakpoints view since 2011? And we did not get any complaints. We would probably have to work on both views together.
Comment 78 Andrey Loskutov CLA 2019-02-22 11:19:44 EST
(In reply to Dani Megert from comment #77)
> (In reply to Andrey Loskutov from comment #76)
> I did not yet go through your items, but are you aware that we have exactly
> the same in the Breakpoints view since 2011? 

The code is not same, definitely not, especially the implementation of the local/global history.

> And we did not get any
> complaints. We would probably have to work on both views together.

The one works. Another doesn't. The views have also different use model. Breakpoints view is "detached" from any context, Variables view is tightly connected to the current selected frame.
Comment 79 Dani Megert CLA 2019-02-24 10:55:20 EST
> especially because the current implementation leaks all "remembered" variables for the entire session to implement this.
Sarika, this is a real concern.
Comment 80 Sarika Sinha CLA 2019-02-24 22:02:56 EST
The Local and Global history implementation is exactly same as the Breakpoints, but yes the context is different.
In breakpoints, local means expressions used for that particular breakpoint and global means expressions used in other breakpoints.
In Variables, local means expressions used for the selected variable and the global means for other variables.


@Andrey
>>2) I still can't really understand why do we have global/local history >>differentiation, especially because the current implementation leaks all >>"remembered" variables for the entire session to implement this. 

What do you mean by this ?
Comment 81 Sarika Sinha CLA 2019-02-24 22:40:28 EST
Local history has a maximum length of 10 for Breakpoints and Variables.

Both have the same persistence strategy for Breakpoint/Variables and their locla values.

We can do away with Local/Global differentiation in totality from both the views if we want.
Comment 82 Andrey Loskutov CLA 2019-02-25 01:04:38 EST
(In reply to Sarika Sinha from comment #80)
> @Andrey
> >>2) I still can't really understand why do we have global/local history >>differentiation, especially because the current implementation leaks all >>"remembered" variables for the entire session to implement this. 
> 
> What do you mean by this ?

https://git.eclipse.org/r/#/c/137329/2/org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/variables/JavaVariablesDetailPane.java@218

(In reply to Sarika Sinha from comment #81)
> Local history has a maximum length of 10 for Breakpoints and Variables
> We can do away with Local/Global differentiation in totality from both the
> views if we want.

This would fix some of the mentioned issues. I believe that "global" history of size 20 would be enough.
Comment 83 Sarika Sinha CLA 2019-02-25 11:50:00 EST
I will be going ahead in dropping local history for Variables view. There will be only a global history with 20 items at max. All other things will remain the same.
Comment 84 Sarika Sinha CLA 2019-02-26 01:34:39 EST
(In reply to Sarika Sinha from comment #83)
> I will be going ahead in dropping local history for Variables view. There
> will be only a global history with 20 items at max. All other things will
> remain the same.

Gerrit is updated after removing Local History.
Comment 86 Sarika Sinha CLA 2019-02-27 05:13:41 EST
Verified using
Eclipse SDK

Version: 2019-03 (4.11)
Build id: I20190226-1800