Comment 1 Martin Oberhuber 2012-12-12 12:56:25 EST

Hi Randy,

what is the end-user workflow / value of adding exactly the same expression multiple times ? Would you want different renderers per entry (eg show one as binary, hex the other one) ? Sorting of entries ? Anything else ?

From my initial POV, trying to add exactly the same expression again I'd also be OK with getting the existing expression highlighted (thus notifying me of the existing entry) rather than adding a duplicate.

Comment 2 Randy Rohrbach 2012-12-12 13:15:30 EST

Martin

   As far as I have seen there is not single implementation of the 
   Expression view where duplicate entries are combined. They seem
   redundant, but so be it. 

   In our existing Wind River product we implemented a form of individual
   debuggee association ( e.g. multiple cores running the same software
   in an AMP configuration ) where different debuggees had different values
   for the same variables ( e.g. packet transfer counts on different cores
   handling different network interfaces ).

   In addition the workflow is very confusing, with the current implementation.
   If you add the variable "randy" 3 times you will see it once. You select
   "randy" and hit the delete ICON. It does not go away. You do the same 
   operation and it still does not go away. Finally you do it a third time and
   it goes away. This is very confusing and not obvious. Highlighting it as 
   you suggested below still does not tell you how to manage the deletion,
   although you at least have a clue that there are others.

   So in essence the look and feel is different from any other implementation
   and does not allow for differentiating the entries. Without other changes
   to TCF the differentiation will not be possible ( like it was in DSF ). But
   at least the workflow interaction can be consistent with all other 
   implementations.

   Perhaps this is a conscious decision and not a bug. I an curious to see what 
   Eugene thinks about this.

Randy

Comment 3 Pawel Piech 2012-12-12 13:36:37 EST

I agree with Martin that there really isn't a valid use case for adding duplicate expressions, but I agree with Randy, that showing the duplicate is a more rational solution than hiding them.

The patch is rather verbose though, I'll try to see if there's a cleaner way to accomplish the same end.

Comment 4 Eugene Tarassov 2012-12-12 13:49:59 EST

(In reply to comment #2)
> Martin

   As far as I have seen there is not single implementation of the 
> Expression view where duplicate entries are combined. They seem
  
> redundant, but so be it.

We don't have to repeat mistakes of other implementations :)

   In our existing Wind River product we
> implemented a form of individual
   debuggee association ( e.g. multiple
> cores running the same software
   in an AMP configuration ) where different
> debuggees had different values
   for the same variables ( e.g. packet
> transfer counts on different cores
   handling different network interfaces
> ).

I'm not sure how this is related. The debugger never combines expression nodes from different debug contexts. In any case, only *identical* expressions are supposed to be combined.

   In addition the workflow is very confusing, with the current
> implementation.
   If you add the variable "randy" 3 times you will see it
> once. You select
   "randy" and hit the delete ICON. It does not go away.
> You do the same 
   operation and it still does not go away. Finally you do
> it a third time and
   it goes away. This is very confusing and not obvious.
> Highlighting it as 
   you suggested below still does not tell you how to
> manage the deletion,
   although you at least have a clue that there are
> others.

This looks like a bug and needs to be fixed.

   So in essence the look and feel is different from any other
> implementation
   and does not allow for differentiating the entries.
> Without other changes
   to TCF the differentiation will not be possible (
> like it was in DSF ). But
   at least the workflow interaction can be
> consistent with all other 
   implementations.

   Perhaps this is a
> conscious decision and not a bug. I an curious to see what 
   Eugene thinks
> about this.

Combining identical expressions improves performance and reduces target traffic. Note that expression cache is used for both the Expressions view and mouse hover expressions. Mouse hover can produce large number of identical expressions, so combining is especially beneficial there. I don't really care that much if we show identical entries in the Expression view, but same TCFNodeExpression must be reused for each entry.

Comment 5 Randy Rohrbach 2012-12-12 14:38:24 EST

(In reply to comment #4)
> (In reply to comment #2)
> > Martin
> 
>    As far as I have seen there is not single implementation of the 
> > Expression view where duplicate entries are combined. They seem
>   
> > redundant, but so be it.
> 
> We don't have to repeat mistakes of other implementations :)
> 

You could argue this either way. Until now every implementation has
chosen to mirror the content of the Expression Manager in the view.
There are filtering bugzillas being worked on, but without filtering
they show one-to-one. Our testers noticed this right away as an anomaly.
So an internal defect was filed. That's why I looked in to it.
If you are saying this is TCF's vision for this use case. The the deletion
defect needs to be addressed. 

Personally, I like seeing the duplicates.

>    In our existing Wind River product we
> > implemented a form of individual
>    debuggee association ( e.g. multiple
> > cores running the same software
>    in an AMP configuration ) where different
> > debuggees had different values
>    for the same variables ( e.g. packet
> > transfer counts on different cores
>    handling different network interfaces
> > ).
> 
> I'm not sure how this is related. The debugger never combines expression
> nodes from different debug contexts. In any case, only *identical*
> expressions are supposed to be combined.
> 

If this is true, then you are right this is not relevant here. I did not 
test with multiple debuggees. I will need to.

>    In addition the workflow is very confusing, with the current
> > implementation.
>    If you add the variable "randy" 3 times you will see it
> > once. You select
>    "randy" and hit the delete ICON. It does not go away.
> > You do the same 
>    operation and it still does not go away. Finally you do
> > it a third time and
>    it goes away. This is very confusing and not obvious.
> > Highlighting it as 
>    you suggested below still does not tell you how to
> > manage the deletion,
>    although you at least have a clue that there are
> > others.
> 
> This looks like a bug and needs to be fixed.

I agree this is a bug with the current implementation. Do not see
how to fix it offhand. But your familarity is obviously much
greater than mine.

> 
>    So in essence the look and feel is different from any other
> > implementation
>    and does not allow for differentiating the entries.
> > Without other changes
>    to TCF the differentiation will not be possible (
> > like it was in DSF ). But
>    at least the workflow interaction can be
> > consistent with all other 
>    implementations.
> 
>    Perhaps this is a
> > conscious decision and not a bug. I an curious to see what 
>    Eugene thinks
> > about this.
> 
> Combining identical expressions improves performance and reduces target
> traffic. Note that expression cache is used for both the Expressions view
> and mouse hover expressions. Mouse hover can produce large number of
> identical expressions, so combining is especially beneficial there. I don't
> really care that much if we show identical entries in the Expression view,
> but same TCFNodeExpression must be reused for each entry.

My current implementation ( albeit a crude one ) was written to make sure
that only the original TCFNodeExpression was used when accessing the data
from the target. Essentially I created tracking TCDNodeExpressions ( with 
different IDs ) that reference the original for data extraction purposes.

It would have been far simpler to create different nodes, that distinguish
by expression. This would cause duplicate target access, which could be
expensive ( since the agent does not cache anything ). Perhaps this is enough
of an edge case, that the duplication is unlikely. But when I was working
on it I tried to be as efficient as possible.

Randy

Comment 6 Pawel Piech 2012-12-12 15:05:12 EST

Created attachment 224645 [details]
Simpler fix.

I propose simpler fix.  It would create separate TCFNodeExpression instances for the duplicate entries, but as Martin stated this is not a real-world use case so the trade off is worth it IMO.

Comment 7 Pawel Piech 2012-12-12 15:07:17 EST

Eugene, are you OK with the possible duplicate nodes in the simpler patch?

Comment 8 Eugene Tarassov 2012-12-12 15:19:14 EST

(In reply to comment #7)
> Eugene, are you OK with the possible duplicate nodes in the simpler patch?

I like the simplicity of the patch more then I’m concern with the possible duplicate nodes.

Comment 9 Randy Rohrbach 2012-12-12 15:27:11 EST

(In reply to comment #8)
> (In reply to comment #7)
> > Eugene, are you OK with the possible duplicate nodes in the simpler patch?
> 
> I like the simplicity of the patch more then I’m concern with the possible
> duplicate nodes.

Unless I am misreading Pawel's patch, this will create separate TCFNodeExpression
for every duplicate entry. Which will create separate agent access for each duplicate. I am fine with this. Your comment before seemed to indicate you 
were not.

"I don't really care that much if we show identical entries in the Expression view, but same TCFNodeExpression must be reused for each entry."

So if this is OK, then Pawel's patch is good for me.
Randy

Comment 10 Randy Rohrbach 2012-12-12 15:42:14 EST

(In reply to comment #6)
> Created attachment 224645 [details]
> Simpler fix.
> 
> I propose simpler fix.  It would create separate TCFNodeExpression instances
> for the duplicate entries, but as Martin stated this is not a real-world use
> case so the trade off is worth it IMO.

Based on your patch I think the findScript routine should look like this.

private TCFNodeExpression findScript(String text, IExpression e) {
        for (TCFNode n : getNodes()) {
            TCFNodeExpression node = (TCFNodeExpression)n;
            if (text.equals(node.getScript()) && (node.getPlatformExpression() == null || node.getPlatformExpression().equals(e)) ) {
                return node;
            }
        }
        return null;
    }

You need to be referencing the getPlatformExpression() method not the 
getExpression() method.

Randy

Comment 11 Eugene Tarassov 2012-12-12 15:47:08 EST

(In reply to comment #9)

Unless I am misreading Pawel's patch, this will create separate
> TCFNodeExpression
for every duplicate entry. Which will create separate
> agent access for each duplicate. I am fine with this. Your comment before
> seemed to indicate you 
were not.

If we cannot have both simplicity and no duplicates, I vote for simplicity. At the moment I don't have a good idea how to get both in this case - something to think about later.

Comment 12 Pawel Piech 2012-12-12 16:00:22 EST

We probably already spilled more ink on this bug than it's worth :-)  Thank you both for the reviews.

Comment 13 Martin Oberhuber 2013-05-23 18:06:08 EDT

Comment on attachment 224624 [details]
Creates tracking expression nodes for duplicate entries

Marking Randy's patch obsolete since it was not committed.

The final commit was

https://git.eclipse.org/c/tcf/org.eclipse.tcf.git/commit/?id=97150272a3f9925848b6fbaf4839f3fcdb110822