Community
Participate
Working Groups
I am developing a generic binding class, and I would like to call modify on contexts that allow it, and set on contexts that do not. I currently need to find out 'the hard way' by catching an IllegalArgumentException.
Steven: Sounds interesting. Can you contribute a patch? http://wiki.eclipse.org/Platform_UI/How_to_Contribute
Modify isn't part of our main usecase/support (it was added early on for contexts to be used in a certain way, and then abandoned), and I was thinking of deprecating it (but this wasn't a high priority for me). Before we add more API around it, we would need to understand how it is used/why it is used. PW
In my @PostContextCreate I declare modifiable all of my shared workspace scoped topics (using the workbench context) This is the only context plugins can use to communicate with each other. Of course the event broker is in the OSGI layer, and I use that to post/send events, however the value is not persisted. My design issue comes down to this: If I have an event that only affects current injected objects, I use the event broker. But when future objects will be instantiated/injected and rely on the last event's value, I use the workbench context. If there was a system-wide OSGI context, I would just use that, but each plugin's OSGI context is isolated. The workbench context seems to be the common denominator for an RCP application. I could always reference the workbench context and call set(...), but it is nice to just call modify and not have know what parent context will take responsibility. It seems that a cornerstone of the e4 approach is to reduce dependencies. I can only assume this is the reason why modify(...) is there in the first place.
I have another system (not e4) using the IEclipseContext interface, but I removed the modify(...) method completely, and reimplemented the set(...) When calling set(...), if the context is modifiable, it bubbles up and sets it on the closest modifiable ancestor (the old modify(...) impl). If I want to set the value on the current context, I simple declare that context as modifiable (overriding), then set it. This is working very well, because not having the modify(...) call makes my code not need refactoring when converting a key to be global. It also allows me to declare a context function on demand if I want to track access.
(In reply to Paul Webster from comment #2) > Modify isn't part of our main usecase/support (it was added early on for > contexts to be used in a certain way, and then abandoned), and I was > thinking of deprecating it (but this wasn't a high priority for me). > > Before we add more API around it, we would need to understand how it is > used/why it is used. > > PW It has its purpose and should be kept! See http://tomsondev.bestsolution.at/2013/11/21/what-ieclipsecontextdeclaremodifiable-is-good-for/ and http://tomsondev.bestsolution.at/2013/11/21/writing-ieclipsecontext-less-code/ -1 for deprecating / removing
I don't want to remove declareModifiable(...)! I want to make modify(...) the default behavior when calling set(...).
(In reply to Steven Spungin from comment #6) > I don't want to remove declareModifiable(...)! I want to make modify(...) > the default behavior when calling set(...). I don't think this is a good idea and I have my doubts you can change this without breaking backwards compat.
In my case, the ContextImpl has a static member that will allow the old behavior. In regards to refactoring e4 code, anyone not using declareModifiable will have nothing to worry about, and all calls to modify would simply be changed to set(...). In the case of set(...) calls, there would be problems. But I feel the caller of set should explicitly call declareModifiable if they DO NOT want the behavior. It's the same principle as a virtual function; If the base class does NOT want the virtual called, it calls its own method, otherwise derived classes are free to override without 'asking'. So yes, Thomas, there would be issues, but sometimes it makes sense to do something the right way. So whether our not your team decides to go this route, can we at least discuss if it is a better design?
One other case to consider for the current state of the declareModifiable architecture. In Java, a base class has a member. By default sub classes are free to access that member. If a sub class wishes to modify the member without affecting the base class value, a new member with the same name must be declared explicitly. The IEclipse context seems to have this backwards. To mirror this behavior in the IEclipseContext: a) declareModifiable would be the default for every entry and would not be necessary b) set(...) by default would actually be modify(...) c) set(..., [bOverride]) would be the current set(...), so the context and child contexts would use this object, essentially re-declaring the member as in Java) d) get(...) would stay the same So IMHO, the cleanest architecture would be to remove declareModifiable(...) and modify(...), and simply add an override parameter to set(...)
(In reply to Steven Spungin from comment #8) > In the case of set(...) calls, there would be problems. But I feel the > caller of set should explicitly call declareModifiable if they DO NOT want > the behavior. This is a non-starter as 1) set(*) is the primary use in IEclipseContext in the 4.x SDK and 2) it would be a breaking change to make it work a completely different way (it's working as designed). PW
I don't see much use for the modifiable because the way IEclipseContext was designed to be used in the SDK. If you have 2 windows, each with a part that has a handled tool item and a handler for that command is on the application: 1) each part should be able to see the correct data, set at the window level and 2) the handler will see the correct data from each window level when processing its @CanExecute in each part's context. Basically the handler will always apply context.getActiveLeaf() before processing its @CanExecute, so that it can answer correctly in each part, even though one window will be active and the other window won't be. If window1 is active it will post its declared modifiable piece of data in the application ... but that can't be used for the handler @CanExecute, so it would have to start with each handled tool item and go to the activeLeaf() anyway (this is true for RATs as well). I'm not saying the declareModifiable isn't of use to someone, just that it's not used in our main usecases for the IEclipseContext and it seems a parallel system that doesn't work for us, and that's why I'm asking for usecases to understand what exactly it does. PW
[....] > I'm not saying the declareModifiable isn't of use to someone, just that it's > not used in our main usecases for the IEclipseContext and it seems a > parallel system that doesn't work for us, and that's why I'm asking for > usecases to understand what exactly it does. declareModifiable allows me to change the value of a key without knowing in which context it is located. I gave you my usecase for it. Or are you asking for a use case for isModifiable()
(In reply to Thomas Schindl from comment #12) > > declareModifiable allows me to change the value of a key without knowing in > which context it is located. I gave you my usecase for it. Or are you asking > for a use case for isModifiable() I was saying that's why I brought it up in the first place, thank you for that usecase. PW
@Paul, isModifiable() seems rather harmless and certainly won't break anything. And if you decide to deprecate, it's still going to affect those who already use modify(...).
(In reply to Steven Spungin from comment #14) > @Paul, isModifiable() seems rather harmless and certainly won't break > anything. And if you decide to deprecate, it's still going to affect those > who already use modify(...). I would be in favor of a new isModifiable() method. If we provide such a state we should provide API to check if it is set.
adding boolean isModifiable(String name) would walk up the parent chain twice for most successful cases (which I would guess is most of them). But I'd consider a contribution + tests. See org.eclipse.e4.core.internal.tests.contexts.EclipseContextTest.testModify() for examples. PW
@Paul, I wrote a patch for this, and ran into something unexpected when writing the unit tests. My aim was to have isModifiable return false anytime an exception would be thrown if calling modify. EclipseContext context = new EclipseContext(null); context.set("foo", "manchu"); // // will call set context.modify("foo", "manchu"); will throw an IllegalArgumentException. context.modify("bar", "manchu"); // WILL CALL SET context.modify("bar", "manchu"); // will throw an IllegalArgumentException. Issue #1: I would expect an IllegalArgumentException to always be thrown, whether or not the context is set. If declareModifiable is not called, modify should never be allowed. Issue #2: If Issue #1 is the desired behavior, it is concerning that this is an un-checked exception given the fact that the context could be unset between invocations. I personally would like to get a compiler warning if not checking for the exception. Before I submit the patch, please let me know if you really want modify to work for an unset value that has not been declared.
(In reply to Steven Spungin from comment #17) > @Paul, I wrote a patch for this, and ran into something unexpected when > writing the unit tests. My aim was to have isModifiable return false > anytime an exception would be thrown if calling modify. > > EclipseContext context = new EclipseContext(null); > > context.set("foo", "manchu"); // // will call set > context.modify("foo", "manchu"); will throw an IllegalArgumentException. > > context.modify("bar", "manchu"); // WILL CALL SET > context.modify("bar", "manchu"); // will throw an IllegalArgumentException. > > > Issue #1: I would expect an IllegalArgumentException to always be thrown, > whether or not the context is set. If declareModifiable is not called, > modify should never be allowed. I think that works as designed. If the variable can not be found in the context hierarchy modify is the same as set + declareModifiable. Only if someone explicitely set a value without calling declareModifiable you get an error.
The problem is that the current implementation, if not set and not declared, is to call Set, but NOT call DeclareModifiable. So calling modify twice works the first time but not the second.
(In reply to Steven Spungin from comment #19) > The problem is that the current implementation, if not set and not declared, > is to call Set, but NOT call DeclareModifiable. > > So calling modify twice works the first time but not the second. Ah, I see. We should fix that to be consistent. I add Tom Schindl for the discussion. Tom / Paul, what should ctx.modify() do if the variable is not yet in the IEclipseContext?
The javadoc for modify does state: * The value is modified in the context in which it has been previously set. If none of the * contexts on the parent chain have a value set for the name, the value will be set in this * context. It probably would have been better if this had been disallowed from the beginning. i.e. Always declareModifiable(*) or modify(*) will throw the exception. But it's gone out this way, so this is API. Maybe if we call set we call declareModify(*) for them, so 2 calls don't get an IAE. It's not a risk for the rest of the stack, because the setting the variable on the context means other modifies will never go up the parent chain (IIRC). PW
I strongly feel that modify should never work unless declareModifiable is called; This clearly defines the context that should 'receive' the value. A race condition where 2 threads both call modify on different hierarchies of a context chain will give inconstant results, as the first caller to Modify on the unset key gets to declare it without the other caller even being able to check if modifiable has been set. So to not go against the API docs, I will adjust modify to call DeclareModifiable on the context if the key is unset, so the second call will not IAE. With this done, my test cases for isModifiable can now return false in every situation modify would throw.
Here's completely different issue my unit tests uncovered: IEclipseContext grandparent = new EclipseContext(null); IEclipseContext parent = grandparent.createChild(); IEclipseContext son = parent.createChild(); grandparent.declareModifiable("foo"); grandparent.modify("foo", "bar"); parent.set("foo", "bar"); son.modify("foo", "bar"); Current implementation throws on son.modify because it hits parent that was set but not declared, even though grandparent is declared modifiable. So basically a rogue intermediate context can call SET instead of MODIFY and cause child contexts to start throwing unchecked exceptions. Based on this: API: The value is modified in the context in which it has been previously set. If none of the contexts on the parent chain have a value set for the name, the value will be set in this context. I would think: Parent should be set because grandparent declared; The API states that: DelareModifiable 'Declares the named value as modifiable by descendants of this context. If the value does not exist in this context, a null value added for the name'. To resolve this ambiguity, my interpretation of the API is simply: 1. Modify always fails unless DeclareModifiable is set by self-or-ancestor 2. First ancestor with value already set gets the new value 3. If no ancestor is found, the value is set in the calling context I can see how this might break the current implementation, but certainly not the API. The JavaDoc also states: The value has to be declared as modifiable by the original context before this method can be used. If the variable with this name has not been declared as modifiable, an IllegalArgumentException will be thrown. Throwing an exception on the first modify seems to fall into the API spec. There seems to be a paradox here, and it is making it difficult for isModifiable to make any sense!
https://git.eclipse.org/r/#/c/24464/ At the very least the unit test can help resolve the ambiguity.
(In reply to Steven Spungin from comment #23) > > Based on this: > API: The value is modified in the context in which it has been previously > set. If none of the contexts on the parent chain have a value set for the > name, the value will be set in this context. > I would think: > Parent should be set because grandparent declared; The API states that: > DelareModifiable 'Declares the named value as modifiable by descendants of > this context. If the value does not exist in this context, a null value > added for the name'. > > To resolve this ambiguity, my interpretation of the API is simply: > 1. Modify always fails unless DeclareModifiable is set by self-or-ancestor > 2. First ancestor with value already set gets the new value If it's declared modifiable by the grandparent and then modified in the child context, the expectation is that the value change will be visible in the grandparent. That seems to offer 2 choices: Option 1) go up the parent chain and set it on every context that has it set in a local value until we hit the context that declared it modifiable and set it Option 2) go up the parent chain, remove it from contexts that have it set (but not modifiable) until we hit the context that declared it modifiable and set it. > 3. If no ancestor is found, the value is set in the calling context Which is what we do today, so probably OK. PW
(In reply to Paul Webster from comment #25) > (In reply to Steven Spungin from comment #23) > > > > Based on this: > > API: The value is modified in the context in which it has been previously > > set. If none of the contexts on the parent chain have a value set for the > > name, the value will be set in this context. > > I would think: > > Parent should be set because grandparent declared; The API states that: > > DelareModifiable 'Declares the named value as modifiable by descendants of > > this context. If the value does not exist in this context, a null value > > added for the name'. > > > > To resolve this ambiguity, my interpretation of the API is simply: > > 1. Modify always fails unless DeclareModifiable is set by self-or-ancestor > > 2. First ancestor with value already set gets the new value > > If it's declared modifiable by the grandparent and then modified in the > child context, the expectation is that the value change will be visible in > the grandparent. > > That seems to offer 2 choices: > > Option 1) go up the parent chain and set it on every context that has it set > in a local value until we hit the context that declared it modifiable and > set it > > Option 2) go up the parent chain, remove it from contexts that have it set > (but not modifiable) until we hit the context that declared it modifiable > and set it. > > > > 3. If no ancestor is found, the value is set in the calling context > > Which is what we do today, so probably OK. > > PW @Paul If we look at this as a type of 'virtual function' for data, the problem lies in the context's inability to choose to pass the child's value up the chain AKA super, as there is no logic involved. Should an exception be thrown on son.modify as in the current implementation? I don't think so. There is an expectation that the grandparent should receive modify calls from the children. Should an exception be thrown on parent.set? I think that this deliberate act should possibly be invalid if an ancestor has declared. This would solve the former issue. 1a. Require the child to call modify. If they want the buck to stop there, see #2 1b. Once a context chain has been marked modifiable, treat set() as modify(). --- Now there would only be 1 authority for the data. --- 2. Children would be required to call declareModifiable to override their ancestor; Hence, the data would set at the closest ancestor that specified declareModifiable. The bottom line is that if I declareModifiable on my top level context, I will not have to worry about any child ever intercepting my data (via set) without explicitly calling declareModifiable. Does anyone have a use case on why you would ever want to call set instead of declareModifiable on an intermediate context?
(In reply to Steven Spungin from comment #26) > Should an exception be thrown on son.modify as in the current > implementation? I don't think so. There is an expectation that the > grandparent should receive modify calls from the children. > > Should an exception be thrown on parent.set? > I think that this deliberate act should possibly be invalid if an ancestor > has declared. This would solve the former issue. Are these musings? I assume so, so the current behaviour doesn't change. > > 1a. Require the child to call modify. If they want the buck to stop there, > see #2 > 1b. Once a context chain has been marked modifiable, treat set() as > modify(). > > --- Now there would only be 1 authority for the data. --- > > 2. Children would be required to call declareModifiable to override their > ancestor; Hence, the data would set at the closest ancestor that specified > declareModifiable. So here we're saying that we need to check if something is declared modifiable on ancestor or self? If it is, turn the set into a modify? I think that it would be less performant than walking up the parent chain, changing the value if it was set until we hit the ancestor with the modify set. Also, if set is called on a child and then declare modifiable is called on an ancestor later we'd have the current behaviour. PW
(In reply to Paul Webster from comment #27) > (In reply to Steven Spungin from comment #26) > > Should an exception be thrown on son.modify as in the current > > implementation? I don't think so. There is an expectation that the > > grandparent should receive modify calls from the children. > > Should an exception be thrown on parent.set? > > I think that this deliberate act should possibly be invalid if an ancestor > > has declared. This would solve the former issue. > > Are these musings? I assume so, so the current behaviour doesn't change. Not musings, facts and opinions: Current implementation throws exception when grandparent has declared, son has set, and child calls modify. Not my preference. Current implementation allows parent to set although grandparent has declared. I think this is confusing, unclear, and can lead to a very hard to understand and debug system, especially during runtime for child contexts of parent. Regardless of how it is implemented, a table of the combination of grandparent-parent-child cases will certainly help me implement isModifiable. @Paul, can you please fill in the ?'s parent: set foo=parent grandparent: decalareModifiable(foo) set->foo=grandparent child: get->foo = ? child: set->foo=child child: get->foo = ? parent: get->foo = ? grandparent: get->foo = ? and this parent: set foo=parent grandparent: decalareModifiable(foo) set foo=grandparent child: get->foo = ? child: modify->foo=child child: get->foo = ? parent: get->foo = ? grandparent: get->foo = ?
(In reply to Steven Spungin from comment #28) > (In reply to Paul Webster from comment #27) > > (In reply to Steven Spungin from comment #26) > > > Should an exception be thrown on son.modify as in the current > > > implementation? I don't think so. There is an expectation that the > > > grandparent should receive modify calls from the children. > > > Should an exception be thrown on parent.set? > > > I think that this deliberate act should possibly be invalid if an ancestor > > > has declared. This would solve the former issue. > > > > Are these musings? I assume so, so the current behaviour doesn't change. > > Not musings, facts and opinions: > > Current implementation throws exception when grandparent has declared, son > has set, and child calls modify. Not my preference. opinions are musings. I'm trying to determine if you mean them as work items (which would break things and so can't be) or "the system should have worked this way from the beginning" which is probably true, but not actionable. PW
(In reply to Paul Webster from comment #29) > (In reply to Steven Spungin from comment #28) > > (In reply to Paul Webster from comment #27) > > > (In reply to Steven Spungin from comment #26) > > > > Should an exception be thrown on son.modify as in the current > > > > implementation? I don't think so. There is an expectation that the > > > > grandparent should receive modify calls from the children. > > > > Should an exception be thrown on parent.set? > > > > I think that this deliberate act should possibly be invalid if an ancestor > > > > has declared. This would solve the former issue. > > > > > > Are these musings? I assume so, so the current behaviour doesn't change. > > > > Not musings, facts and opinions: > > > > Current implementation throws exception when grandparent has declared, son > > has set, and child calls modify. Not my preference. > > opinions are musings. I'm trying to determine if you mean them as work > items (which would break things and so can't be) or "the system should have > worked this way from the beginning" which is probably true, but not > actionable. > > PW Thanks for clearing that up @Paul, I though you were suggesting that I was thinking out loud :) You could always add an additional method to the interface that might be less vague and more useful. Besides, the current implementation is broken; Calling modify twice from the same context should not give 2 different results. And an ancestor calling set should not suddenly make a child start throwing exceptions. I don't see this in the API docs. I'm not trying to rock the boat here. I use this interface in a large, 4 year effort, GWT project. Most method parameters were replaced with a single IEclipseContext. Much of my GWT code is shared with Eclipse and I have a vested interest in making it kick-a$$. @Thomas very eloquently pointed out the power of modify on his blog. I was hoping this thread could at least decide what is ideal, establish that there are issues with the current implementation, and proceed from there. This started out as isModifiable, but as I have found out, that is a VERY difficult thing to define.
(In reply to Steven Spungin from comment #28) > > @Paul, can you please fill in the ?'s > > parent: set foo=parent > grandparent: decalareModifiable(foo) set->foo=grandparent > child: get->foo = parent > child: set->foo=child becomes modify->foo=child, parent set->foo=child, grandparent set->foo=child > child: get->foo = child > parent: get->foo = child > grandparent: get->foo = child > > and this > > parent: set foo=parent > grandparent: decalareModifiable(foo) set foo=grandparent > child: get->foo = parent > child: modify->foo=child, parent set->foo=child, grandparent set->foo=child > child: get->foo = child > parent: get->foo = child > grandparent: get->foo = child PW
(In reply to Paul Webster from comment #31) > (In reply to Steven Spungin from comment #28) > > > > @Paul, can you please fill in the ?'s > PW That really makes it clear. Thanks.
OK I coded this logic. One more question came up: Concerning when passing through middleman context that had set a value, without declaring: As the modify call passes up to the first immediate declared modifiable context, how do we handle value-change notifications? My first though was to only invalidate when immediate declared modifiable context. The intermediate context and the final context may not have the same value; one or both may change. So even if the top level did not change value, the intermediate one did. Should we wait until setting entire branch, and then ensure that 1 and only 1 notification is sent if any value along the way changed?
(In reply to Steven Spungin from comment #33) > OK I coded this logic. One more question came up: > > Concerning when passing through middleman context that had set a value, > without declaring: > > As the modify call passes up to the first immediate declared modifiable > context, how do we handle value-change notifications? My first though was > to only invalidate when immediate declared modifiable context. The > intermediate context and the final context may not have the same value; one > or both may change. Ideally we would walk up the parent chain similar to org.eclipse.e4.core.internal.contexts.EclipseContext.internalModify(String, Object, Set<Scheduled>), invalidating things in each context with the Set<Schedule>. Then at the end of the modify(*) call after we've done all of the modifies up the parent chain, we can call org.eclipse.e4.core.internal.contexts.EclipseContext.processScheduled(Set<Scheduled>) I think if we can it would better to process all of the invalidate/change notifications at the end. PW
> parent: set foo=parent > grandparent: decalareModifiable(foo) set->foo=grandparent > child: get->foo = parent > child: set->foo=child becomes modify->foo=child, parent set->foo=child, grandparent set->foo=child So if this is reversed: > grandparent: decalareModifiable > parent: set foo=parent The parent's set becomes a modify?
https://git.eclipse.org/r/#/c/24665/1
(In reply to Steven Spungin from comment #35) > > So if this is reversed: > > grandparent: decalareModifiable > > parent: set foo=parent > > The parent's set becomes a modify? Because set(*) has "set it right here" semantics, I think we're going to have to back off on allowing a set(*) to become a modify(*). Now that I think about it, we can allow set(*) to work normally. But a modify(*) on a modifiable variable will walk up the parent chain, and call set(*) on each parent that has set the variable until it reaches the first parent that declares the variable modifiable. Good: 1) we don't change the semantics for set(*) 2) modify(*) on a child will work as expected, the end result being that the grandparent correctly sees the changed value. More iff-y: 1) if you call grandparent.modify(foo) and the parent foo is set, the child won't see the change. But that behaviour is predictable from the parent.set(*), and perhaps wanted by a consumer. It's a consequence of mixing set and modify ... they are not complimentary APIs, they are different ways of working. PW
> Because set(*) has "set it right here" semantics, I think we're going to > have to back off on allowing a set(*) to become a modify(*). Now that I > think about it, we can allow set(*) to work normally. That's how I implemented it in the patch. Except modify will no longer throw if parent is set but grandparent declared. I had a feeling you were going to say that :( > 1) if you call grandparent.modify(foo) and the parent foo is set, the child > won't see the change. But that behaviour is predictable from the > parent.set(*), and perhaps wanted by a consumer. It's a consequence of > mixing set and modify ... they are not complimentary APIs, they are > different ways of working. > This is why I like setGlobal. It unifies the semantics of set and modify, and eliminates intermediate contexts doing nasty things; every node is guaranteed the same key->object.
Mass move to 4.7.
