Community
Participate
Working Groups
Follow-up on change 536308. In ToolBarManagerRenderer, for each and every toolbar contribution that has a visibleWhen expression, a separate context RunAndTrack is scheduled. It also scales non-linearly, because toolbar contributions also check their children. The current implementation also adds variables to the central updateVariables Set for which a central RunAndTrack is executed, but never removes them again, leaking unnecessary context updates. In situation where a lot of context modifications happen, e.g. closing and opening of editors, this can cause thousands and thousands of update calls to the handlers' visibility / enabled state.
Sebastian/ Tobias, knowing you guys, I assume you will attach a patch soon. Looking forward to review it.
New Gerrit change created: https://git.eclipse.org/r/132297
(In reply to Eclipse Genie from comment #1) > Sebastian/ Tobias, knowing you guys, I assume you will attach a patch soon. (In reply to Eclipse Genie from comment #2) > New Gerrit change created: https://git.eclipse.org/r/132297 ROFL! .. and WeakReference is totally cool. I can't understand how I could have completely missed WeakReference and SoftReference over all those years. I can't recall having ever seen it in Eclipse code.
We are not sure whether this approach might break something. In fact, I am not sure anymore if https://git.eclipse.org/r/#/c/129560/ even is the right approach. Both changes move the calculation of enabledWhen / visibleWhen outside of the RunAndTrack runnable and therefore detach it from the context. For a completely imperatively modeled enabledWhen / visibleWhen via e4, which depends on arbitrary values in the IEclipseContext, this might cause the enabled / visible state to be inconsistent. E.g.: - Two toolbar buttons - 1st button sets a value in the IEclipseContext - 2nd button's visibleWhen depends on that value in the context I think this never really surfaced as a big problem, because the global RunAndTrack in ToolBarManagerRenderer updates every contribution item on any change of ACTIVE_SELECTION. This happens quite often if the UI is used -- any therefore "repairs" a potentially inconsistent enabled state. I am not really sure how to go on with this problem -- the whole IEclipseContext is very complex. Some thoughts on why this is such a big performance problem especially for the ToolBar: - The toolbar lives in the Window's context - The items in the toolbar usually depend on information in children of that context, e.g. the active selection in the active context. - This means that the contributions have to be updated every time the variables active* change in the window context. @Thomas Schindl Do you have an idea on how the situation here can be improved while keeping the RunAndTracks and context changes consistent?
My two cents regarding my experience with the changes in this area 1) There are RCP applications which use the API's provided by ToolBarManagerRenderer and any changes here need to be tested with few RCP applications which have toolbar and contribute to the toolbar.(I donot have any sample RCP applicatiosn with me that I can point to.) 2) There are other apps which are built using eclipse and contribute to the toolbar their own menu items. We should be careful not to break their behavior. I have observed that in such cases, Eclipse toolbar is fine but the dependent application toolbar is broken. If possible we will need to document what the applications need to do to fix their problem similar to what has been done in Eclipse. I have not looked at the code. These are my general observations regarding what needs to be taken care of when making major changes to ToolBarManagerRenderer.
(In reply to Kalyan Prasad Tatavarthi from comment #5) > My two cents regarding my experience with the changes in this area > > 1) There are RCP applications which use the API's provided by > ToolBarManagerRenderer and any changes here need to be tested with few RCP > applications which have toolbar and contribute to the toolbar.(I donot have > any sample RCP applicatiosn with me that I can point to.) > > 2) There are other apps which are built using eclipse and contribute to the > toolbar their own menu items. We should be careful not to break their > behavior. > I have observed that in such cases, Eclipse toolbar is fine but the > dependent application toolbar is broken. If possible we will need to > document what the applications need to do to fix their problem similar to > what has been done in Eclipse. > > I have not looked at the code. These are my general observations regarding > what needs to be taken care of when making major changes to > ToolBarManagerRenderer. Regarding > If possible we will need to > document what the applications need to do to fix their problem similar to > what has been done in Eclipse. What I meant here was to document the usage of new API.
Mass change, please reset target if you still planning to fix this for 4.11.
Please set the target milestone back to 4.12 if you intend to fix this for 4.12.
Adding Rolf, as he is now the master of visibility for tool items.
Sebastian, any more work planned here?
Not at the moment, no :( Changes in this area need to be well-conceived as they have the chance for major side effects.
