Community
Participate
Working Groups
Based on bug 202556 and bug 395909, it shouldn't be very hard to enable per-variable and per-expression formatting.
I've pushed a patch to Gerrit: https://git.eclipse.org/r/30192 It builds upon the solution for bug 395909 (registers) and makes it a generic solution for DSF. 1- New AbstractElementVMProvider which any VMProvider can extend to easily support individual number formatting 2- ExpressionVMProvider, RegisterVMProvider and VariableVMProvider extend AbstractElementVMProvider but keep the individual format feature off by default (IElementFormatProvider.supportFormat() returns false by default). This approach makes it very easy for other DSF-based integration to use the feature, but does not force them to do so. 3- GdbExpressionVMProvider, GdbRegisterVMProvider and GdbVariableVMProvider enable the feature 4- New ElementFormatPersistableFactory and ElementFormatPersistable in DSF to support this feature for any type of element 5- I also moved the PDA example to use AbstractElementVMProvider and the rest of the infrastucture Here is how the feature behaves with this patch. a- Each element of the Register, Variables, or Expressions view can have its own format b- in the registers view, the option is disabled for nodes that are not registers (e.g., register groups) c- in variables or expressions views, the option is disabled for enhanced expressions d- each view instance has its own format for an element. So, if we clone a view, we can show a different format for the same element for each view. Also, the expression view can show one format while the variables view can show another for the same variable. e- the same variable shown more than once in the same expressions view instance will have the same chosen format f- closing a view and re-opening it persists the format selections g- restarting eclipse persists the format selections behaviour that could be improved: h- a variable with the same name from two different stack frames will have the same format selection. So if main() has variable 'int i' and main calls foo() which has variable 'char i', setting the format for 'i' will affect both variables. Same goes for a method bar() with 'bool i' and main calls foo(), then main() calls bar(); 'i' will have the same format for both foo::i and bar::i. This is because the format is associated to the expression string ('i' in this case). We could be more specific and use stack-depth as well as function name to qualify further the name 'i' but I wasn't sure it was worth it. i- when setting the format for one element, the entire view is refreshed, including the relevant services. This is overkill and could be optimized. It does not affect the user much, except for a possible performance impact on slow targets. j- setting the format for an element will affect a similarly named element of another running session, but will not refresh its view. This is poor behaviour when it does occur. I'm still trying to fix this.
(In reply to Marc Khouzam from comment #1) > i- when setting the format for one element, the entire view is refreshed, > including the relevant services. This is overkill and could be optimized. > It does not affect the user much, except for a possible performance impact > on slow targets. I've pushed a small update that allows to only refresh the view, not the services. I don't understand the AbstractCachingVMProvider well enough to be able to refresh only a single element, and I'm not sure that is even possible. But I think avoiding to refresh the services which communicate with GDB was the main necessary improvement.
(In reply to Marc Khouzam from comment #2) > I've pushed a small update that allows to only refresh the view, not the > services. I don't understand the AbstractCachingVMProvider well enough to > be able to refresh only a single element, and I'm not sure that is even > possible. But I think avoiding to refresh the services which communicate > with GDB was the main necessary improvement. I've pushed patchset 8 which no longer uses the refresh() call but uses the ElementFormatEvent to properly only refresh the proper element. I'm still looking into ElementFormatEvent.getApplyDepth() which is not used in my patch but probably should.
Another issue I hadn't mentioned is the format "String" which we don't support in DSF-GDB. DSF shows that option and allows the user to select it, although it will go back to the preference (although showing String as selected in the menu). I wanted to look into that to see if we could improve that behaviour, like maybe not showing the "string" option.
(In reply to Marc Khouzam from comment #4) > Another issue I hadn't mentioned is the format "String" which we don't > support in DSF-GDB. DSF shows that option and allows the user to select it, > although it will go back to the preference (although showing String as > selected in the menu). This issue was discussed in Bug 371012. Furthermore, as part of a patch in bug 351479, Pawel coded a solution that will work here. I have extracted the relevant parts from the patch of bug 351479 and will make a new commit on top of what I have now.
(In reply to Marc Khouzam from comment #5) > This issue was discussed in Bug 371012. Furthermore, as part of a patch in > bug 351479, Pawel coded a solution that will work here. I have extracted > the relevant parts from the patch of bug 351479 and will make a new commit > on top of what I have now. Like Pawel said in bug Bug 351479 comment 4, I ran into a 5 seconds delay when trying to edit a cell using the new technique. Quoting him: "However I've hit one major problem. The element number format support does not interact well with the cell editor and currently I experience a 5sec. deadlock every time I try to edit one of the register values. To fix this problem I'll need to extend the flexible hierarchy API in platform..." I'll have to figure out how to work around this...
(In reply to Marc Khouzam from comment #6) > (In reply to Marc Khouzam from comment #5) > Like Pawel said in bug Bug 351479 comment 4, I ran into a 5 seconds delay > when trying to edit a cell using the new technique. > I'll have to figure out how to work around this... The reason for the 5 second delay is that WatchExpressionCellModifier#queryElementFormat() uses a Query with a 5 seconds timeout. A Query must be used because the interface to ICellModifier (from JFace) is synchronous. So what happens is that the call to WatchExpressionCellModifier#queryElementFormat() locks the UI thread to fetch the current format of the cell; however, in Pawel's implementation, the UI thread is needed to fill VM node properties which are used in the 'fetch format' logic; this obviously causes a deadlock, which causes the 5 sec timeout to hit; at which point the cell being modified (in the e.g., expressions view) will not show the proper format. What I will do is simplify the way we 'fetch format' in such a way that we don't need the UI thread.
I've pushed version 13 to Gerrit: https://git.eclipse.org/r/30192 It is a hybrid solution between what I had and what Pawel had proposed in bug 351479, where he added some very nice aspects that I hadn't thought of. Here is a new description of the important points of the latest feature implementation: 0- Feature is enabled by default for DSF. Pawel had it enabled by default for DSF so I did the same. It can be turned off by extenders by overriding IElementFormatProvider.supportFormat() to return false in their *VMProvider classes. 1- New AbstractElementVMProvider which any VMProvider can extend to easily support individual number formatting 2- ExpressionVMProvider, RegisterVMProvider and VariableVMProvider extend AbstractElementVMProvider. 3- Use of a specific class ElementNumberFormatProvider to provide the per-element format implementation thanks to Pawel. This is much better than putting it directly in AbstractElmeentVMProvider as it keeps the API isolated to that class, which can be changed/replaced by extenders much more easily. 4- Use of SimpleMapPersistable and SimpleMapPersistableFactory for persistence of all formats 5- The list of available formats is now fetched from the services in ElementNumberFormatsContribution, again thanks to Pawel. So we don't show the 'string' format anymore, but do show 'details' in the case of DSF-GDB, which is nice. 6- I decided to keep the PDA example behaving as before as it provides a slightly different way of doing things and can serve as a second example The feature still behaves the same as in the previous patches: > a- Each element of the Register, Variables, or Expressions view can have its > own format > > b- in the registers view, the option is disabled for nodes that are not > registers (e.g., register groups) > > c- in variables or expressions views, the option is disabled for enhanced > expressions > > d- each view instance has its own format for an element. So, if we clone a > view, we can show a different format for the same element for each view. > Also, the expression view can show one format while the variables view can > show another for the same variable. > > e- the same variable shown more than once in the same expressions view > instance will have the same chosen format > > f- closing a view and re-opening it persists the format selections > > g- restarting eclipse persists the format selections but also: h- when setting the format for one element we only update that element for that view Remaining issues I'm aware of: i) a variable with the same name from two different stack frames will have the same format selection. So if main() has variable 'int i' and main calls foo() which has variable 'char i', setting the format for 'i' will affect both variables. Same goes for a method bar() with 'bool i' and main calls foo(), then main() calls bar(); 'i' will have the same format for both foo::i and bar::i. This is because the format is associated to the expression string ('i' in this case). We discussed that issue internally and someone pointed to the fact that programmers have a tendency to use similar names for similar variables, so when using 'i' it will probably end up being the same type for any instance and will actually benefit from automatically being formatted as the other 'i' variable. I plan on leaving this as is and waiting for feedback to know if it needs to be improved. ii) setting the format for an element will affect a similarly named element of another running session, but will not refresh its view. This is poor behaviour when it does occur. I've spent quite some time investigating a solution to this and I have a couple of working suggestion. I will post that fix as a separate patch so it can be discussed on its own. So, patchset 13 is ready for review :)
(In reply to Marc Khouzam from comment #8) > ii) setting the format for an element will affect a similarly named element > of another running session, but will not refresh its view. This is poor > behaviour when it does occur. I pushed patchset 14 which fixes this issue. I tried many things and I found one that I believe is very nice. The original solution (patch 13) would pass an ElementFormatEvent to its own view only using: ((AbstractVMProvider)getVMProvider()).handleEvent(...) This has the advantage of only affecting the view where the format changed occurred. The problem was that it didn't notify other sessions using that same view. I then looked at using DsfSession#dispatchEvent() and doing it for every active DSF session. The problem there was that the event would be received and trigger an update of every view (variables, expressions and registers, and any clones) although it was only affecting a single one. I then looked at how we were able to change the format of an entire view (using the view menu) but it was only updating that view but for all sessions. I saw that it used IPresentationContext.setProperty(). This was a viable solution in our case. The problem there was that instead of triggering an ElementFormatEvent, it would trigger a PropertyChangedEvent for a new property; this meant having to update the different VMNodes to listen for that event and property. I didn't like that, especially since the fact that our format change affecting all sessions is an implementation consequence and may not be true for other implementations. Finally, I looked into using DsfSession#dispatchEvent() but with a filter. The filter would allow to only send events to the view that it was meant for. The other benefit is that the entire support for this is isolated in ElementNumberFormatProvider which makes sense since it is its implementation that required the solution; another implementation of IElementFormatProvider may not require this fix and therefore should not have to deal with extra filter or special events that cross session boundaries. So patchset 14 implements this solution. Patchset 14 provides a complete feature in my mind and I'm ready to commit it after it being reviewed.
(In reply to Marc Khouzam from comment #9) > Patchset 14 provides a complete feature in my mind and I'm ready to commit > it after it being reviewed. I've committed this after Alvaro's review. I'm updating the N&N worthy now.
