Community
Participate
Working Groups
AFAIK container data must be a number, currently we accept all input, e.g. letters too. I suggest to restrict this to numbers. @Eric/ Tom: Are integers only ok?
Well if it is the child of a SashPart-Container your are right but it really depends on the container you are in. IMHO we should have made the ContainerData a Map with key-value pairs instead of a dumb-string, this would make it easier to spec min/max values in future. If we would have a model upgrade story we could try to address this before we make the model API, not sure if the reason I give here are sufficient since this container-Data has been like this from the first day. Eric, we should set it on our discussion list.
I discussed this some time ago with Eric in bug 385371
I agree with Tom that a map with key-value pairs would be helpful. I close this bug as INVALID. Tom, please open a new bug report for the map with key/value pairs.
We could also *remove* 'containerData' as a specific attribute now that we have the 'persistentData' map...the contract is totally at the discretion of the *owning* (i.e. parent) container how / what it wants to store to correctly handle its children... The SashRenderer could just add a new key 'sashWeight' to the map and use it, making the current attribute unnecessary.
+1 for removal of the container data. @Eric: Is this still an option for Eclipse 4.3?
Removing containerData must be discussed on P/UI.
No, basically due to the fact that we would have to be able to handle reading / updating existing saved models. I've marked this for a look in Luna but this in one of those cases where backwards compatibility (even in 4.x...;-) may rule out doing it in Luna. Eventually I know we're going to change the model in some breaking manner but we have nobody here that knows the right hoops to jump through to dynamically upgrade a model on 'read'...
I'm moving this off to the side for Luna...it's not causing any issues and nothing is preventing a renderer from skipping the use of this field and directly using keyed values in the persistentData map...
If I'm going to do anything here it'll most likely be to just mark the 'containerData' as deprecated and point towards the persistentData approach... Does anybody want to make a strong case that the field *must* be removed ?
We should do something here or defer this until post-Luna. If it effects API we would have to deprecate it and if we want to remove it, make that claim. PW
(In reply to Eric Moffatt from comment #9) > If I'm going to do anything here it'll most likely be to just mark the > 'containerData' as deprecated and point towards the persistentData > approach... > > Does anybody want to make a strong case that the field *must* be removed ? Eric did you manage to deprecate the fields and use persistentData?
I mark this as WONTFIX. No need to adjust the current implementation.
Steven started to work on this in Bug 361731. Reopening.
We should support the existing container data but deprecated it (issue a message if it is used) and replace it with persistent state.
(In reply to Lars Vogel from comment #14) > We should support the existing container data but deprecated it (issue a > message if it is used) and replace it with persistent state. Is the SashLayout the only class using ContainerData. Are their any non-SWT renderers or classes that use it? How likely will a new model using persistentData instead be opened on an older build that is still looking for containerData? The new patch allows for an integer or value such as (size:50%;min:20px;max:100px) I could keep those values separate with persistentData, but it is trivial to parse the string, and simple to enter it into the UI. If we are going to restrict or validate the input, it might make sense to just keep the containerData. I think it is specialized enough to warrant its own setting on the main tab. If we do remove containerData, I would suggest still having a separate text box in the editor that just modifies the relevant keys in the map; I was going to put in a tooltip to describe the new ContainerData format.
Here is a reason we may want to keep ContainerData as a discrete attribute: SashRenderer subscribes to changes: eventBroker.subscribe(UIEvents.UIElement.TOPIC_CONTAINERDATA, sashWeightHandler); Does EMF allow us to subscribe to a single key in the map?
(In reply to Steven Spungin from comment #15) > (In reply to Lars Vogel from comment #14) > > We should support the existing container data but deprecated it (issue a > > message if it is used) and replace it with persistent state. > > Is the SashLayout the only class using ContainerData. Are their any non-SWT > renderers or classes that use it? > e(fx)clipse JavaFX renderer use containerData as well as persistedState so we can already work with persitedState and track changes in there as well. If upstream changes to persistedState we are fine with that and will support it as well in our sash-renderer.
(In reply to Steven Spungin from comment #16) > Here is a reason we may want to keep ContainerData as a discrete attribute: > > SashRenderer subscribes to changes: > > eventBroker.subscribe(UIEvents.UIElement.TOPIC_CONTAINERDATA, > sashWeightHandler); > > Does EMF allow us to subscribe to a single key in the map? I think we can do it similar to the event handler in StackRenderer for transient data.
(In reply to Thomas Schindl from comment #17) > e(fx)clipse JavaFX renderer use containerData as well as persistedState so > we can already work with persitedState and track changes in there as well. > If upstream changes to persistedState we are fine with that and will support > it as well in our sash-renderer. I would suggest refactoring the calculation logic out of the SWT renderer so both platforms can benefit. Where would we put this shared code?
(In reply to Steven Spungin from comment #16) > Here is a reason we may want to keep ContainerData as a discrete attribute: > > SashRenderer subscribes to changes: > > eventBroker.subscribe(UIEvents.UIElement.TOPIC_CONTAINERDATA, > sashWeightHandler); > > Does EMF allow us to subscribe to a single key in the map? Yes it allows you to listen do changes in a Map-Attribute what you get as an event has: event.getProperty(UIEvents.EventTags.ATTNAME) == UIEvents.ApplicationElement.PERSISTEDSTATE; java.util.Map.Entry<String,String> e = event.getProperty(UIEvents.EventTags.NEW_VALUE);
(In reply to Steven Spungin from comment #19) > (In reply to Thomas Schindl from comment #17) > > > e(fx)clipse JavaFX renderer use containerData as well as persistedState so > > we can already work with persitedState and track changes in there as well. > > If upstream changes to persistedState we are fine with that and will support > > it as well in our sash-renderer. > > I would suggest refactoring the calculation logic out of the SWT renderer so > both platforms can benefit. Where would we put this shared code? if there's code you want to share it would be somewhere in the ui.workbench at the same level where you have IPresentationEngine. The question is what you want to return from such a service. JavaFX SplitPane works completely different to e4-SWT-SashLayout so I don't think that makes a whole lot of sense. Beside that e(fx)clipse renderers internally work completely different than the their SWT counter parts with respect to how informations are transfered into the rendering pipeline.
(In reply to Thomas Schindl from comment #21) > if there's code you want to share it would be somewhere in the ui.workbench > at the same level where you have IPresentationEngine. The question is what > you want to return from such a service. JavaFX SplitPane works completely > different to e4-SWT-SashLayout so I don't think that makes a whole lot of > sense. I am referring to code that calculates the current weights during layout and resizing based on min/max values and the other relative/absolute values in the container. Also code that parses and regenerates the container data. The current implementation has all that logic in the SWT renderer. It makes sense to me that e(fx)clipse renderers (and other renderers) would not need to reinvent that stuff.
Another reason to keep ContainerData discrete. Now that constraints and absolute sizes can be set, a part can declare its size. It will be simple for users to set ContainerData vs needing to know the map key, etc. It would also be nice if a part-stack without container data set would fall through and consider the container data in the parts it contains, if their data is set. In addition, if we store this in the map, all other map listeners are going to receive MANY callbacks when the sash is resized if we can't subscribe to just one key. If this values was not written to often, and did not have to do with size, I would be more convinced to migrate it to a map.
Many implementations (addons, etc) clone a part's ContainerData; storing discrete values would make it difficult to only copy those fields. Could we deprecate containerData in favor of using PersistedData, and also add a new member to MUIElement. This new member could be more focused as to the size/resize behavior of the element; It could either be a map<string,object> or have discreet members. (I suggest string,object because it would optimize calculations).
(In reply to Steven Spungin from comment #24) > Could we deprecate containerData in favor of using PersistedData, and also > add a new member to MUIElement. This new member could be more focused as to > the size/resize behavior of the element; It could either be a > map<string,object> or have discreet members. (I suggest string,object > because it would optimize calculations). I think persistedData is the write place as we need to persist the data between application restarts. I think it is very important to keep the model as simple as possible and adding a persisted map<string,object> is not a good way. We could use transientData to store any calculated data. That is already map<string,object>.
(In reply to Lars Vogel from comment #25) > (In reply to Steven Spungin from comment #24) > > > Could we deprecate containerData in favor of using PersistedData, and also > > add a new member to MUIElement. This new member could be more focused as to > > the size/resize behavior of the element; It could either be a > > map<string,object> or have discreet members. (I suggest string,object > > because it would optimize calculations). > > I think persistedData is the write place as we need to persist the data > between application restarts. I think it is very important to keep the model > as simple as possible and adding a persisted map<string,object> is not a > good way. We could use transientData to store any calculated data. That is > already map<string,object>. Questions: 1. So how does an addon that only wants to clone the size data get to just those items? Clone the transient data? I guess we could we store an sizing object in transient data, and just clone that. 2. Where would the best place in the code to load/save the persistedData to transientData be? 3. Are we OK with these keys: <prefix>.resizeMode, <prefix>.size, and <prefix>.unit? What prefix to use? 4. Can we add a new widget in on the main tab of the editor for these 3 values, right after the containerData? 5. What's the strategy for legacy containerData values?
(In reply to Steven Spungin from comment #26) > Questions: > > 1. So how does an addon that only wants to clone the size data get to just > those items? Clone the transient data? I guess we could we store an sizing > object in transient data, and just clone that. The base data would be in persistedState and the converted data in transient data. The model add-in would pick whatever it like. > 2. Where would the best place in the code to load/save the persistedData to > transientData be? If required, I think SashRenderer is the correct place. > 3. Are we OK with these keys: <prefix>.resizeMode, <prefix>.size, and > <prefix>.unit? What prefix to use? Sounds good for an initial implementation. What about sashlayoutdata as prefix? > > 4. Can we add a new widget in on the main tab of the editor for these 3 > values, right after the containerData? I suggest to first use the existing tooling, once this is implemented we can enhancement the tooling. > 5. What's the strategy for legacy containerData values? I must still support it. Maybe convert it at runtime to the new values and issue a warning message to the log?
Created attachment 247011 [details] using persisted state to set part size information
Created attachment 247012 [details] equivalant container data shortcut It is MUCH easier to still specify as a string. Lateset patch will automatically convert this to persistent data.
Created attachment 247013 [details] perspective showing runtime values
Mass move to 4.7 as M7 is approaching. Please move back in case you are planning to fix it for Neon.
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
