This Bugzilla instance is deprecated, and most Eclipse projects now use GitHub or Eclipse GitLab. Please see the deprecation plan for details.
Bug 296478 - [UI] Model tweaking...round 2
Summary: [UI] Model tweaking...round 2
Status: RESOLVED FIXED
Alias: None
Product: e4
Classification: Eclipse Project
Component: UI (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-30 09:37 EST by Eric Moffatt CLA
Modified: 2010-01-04 11:42 EST (History)
6 users (show)

See Also:


Attachments
Patch to refactor the UIElement class (163.00 KB, patch)
2009-12-07 14:09 EST, Eric Moffatt CLA
no flags Details | Diff
Patch to refactor the UIItem class (210.73 KB, patch)
2009-12-10 13:18 EST, Eric Moffatt CLA
no flags Details | Diff
Patch removing unnecessary IDE elements (683.97 KB, patch)
2009-12-10 14:18 EST, Eric Moffatt CLA
no flags Details | Diff
Patch to change the 'visible' attribute to match the SWT 'setVisible' (197.01 KB, patch)
2009-12-14 16:22 EST, Eric Moffatt CLA
no flags Details | Diff
Patch to fix the GenericTile 'weights' (190.22 KB, patch)
2009-12-16 14:36 EST, Eric Moffatt CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Moffatt CLA 2009-11-30 09:37:35 EST
I've started updating the UI Model wiki and before going too far down the path there are a number of tweaks we've discussed making to the model:

- Collapse all containers into the UIElement container (i.e. no separate stack/sash types). It would have a new field 'stack' which is true if the rendered widget is to exhibit stack-like behavior (i.e. only the active child is 'visible').

- As part of the above remove the current 'weights' list and replace it with a string attribute 'containerInfo' which can be used by the rendered container to store/retrieve whatever information it needs to correctly host its children. For a SashForm container this would be the current weights but for an ExpandBar it might be the child's preferred 'height'...

- Change UIItem's class name to avoid confusion with the other 'item' classes. The current guess is to change it to UILabel and also change the 'name' attribute to 'label'.

- Remove the current 'visible' boolean and replace it with either a set of states, a straight bit-set or some combination of both. Exact set of options TBD.

Any others that folks want to add?
Comment 1 Eric Moffatt CLA 2009-11-30 10:14:40 EST
One addition; remove the 'horizontal' attribute from the TrimContainer and infer the orientation from the 'side' value.
Comment 2 Eric Moffatt CLA 2009-11-30 11:55:31 EST
We also need some extra state in the Containers to be able to have them manage the UI state correctly.

- a flag indicating whether or not the container should be removed from the presentation when it has no visible children (ViewStacks do this but not the editor area).

- a flag indicating whether an element should be removed from its container when it's removed from the UI (ViewStacks leave the placeholder behind but EditorStacks remove it completely when the editor closes).
Comment 3 Remy Suen CLA 2009-11-30 12:36:27 EST
(In reply to comment #0)
> I've started updating the UI Model wiki

The link is here, by the way.
http://wiki.eclipse.org/E4/UI/Modeled_UI
Comment 4 Eric Moffatt CLA 2009-11-30 13:20:09 EST
Thanks Remy, that's the page I wrote on the weekend...I'll be working on it over the next few weeks to flesh it out..suggestions for info that you'd like to see on it welcome...;-)
Comment 5 Eric Moffatt CLA 2009-12-03 11:02:59 EST
See bug 289000 (from comment 22 down) for more info on possible changes.

Also see bug 295004 for more details about changing the 'visible' flag to something more state like.

Tom, once I get the et of changes in it would likely be the right time for you to apply the EMF package split and genmodel tweaking...
Comment 6 Remy Suen CLA 2009-12-03 15:57:02 EST
Will a MRectangle/MBounds/MPoint be introduced or are we sticking with discrete x/y/width/height attributes?
Comment 7 Eric Moffatt CLA 2009-12-04 12:43:16 EST
I expect that using a Rectangle would be more convenient for our users. Note that we'd have to tweak the UIEventPublisher to forward setRectangle notifications even if 'isTouch()' is true. This is because we can reasonably expect the pattern for its use to be:

Rectangle rect = me.getRectangle();
rect.setX(200);
...
me.setRectangle(rect)

...this would be treated by EMF as a 'NO-OP' (i.e. isTouch() == true) since it's the same Rectangle element.
Comment 8 Eric Moffatt CLA 2009-12-04 13:08:20 EST
Also, we should meet to determine which elements are going to be used to implement the compatibility layer and remove any extra elements. The current 'IDE' model elements are legacy-centric and were in the model to demonstrate a different containment structure.

The work that's been done with Commands and that is currently being done for perspective handling both end up with the person/code defining the model having to 'buy in' to the more complex functionality by using different 'advanced' model elements.

We should remove the IDE model elements now but ensure that we provide the necessary set of 'advanced' elements to meet the needs of the compatibility implementation. This way someone starting to develop an application can either use the simple mechanisms (i.e. DirectMenuItem...) or, should they desire more advanced functionality like key-bindings they can switch over to using the advanced structures.

If we go this route then the 'package' structure for the model becomes far less RCP vs IDE (removing the naming issues...;-) and looks more like this:

<Data Layer>
<Part Model>
<Command Model>
<SharedElements Model>

You can write a complete app without having to use the bottom two models but they're available if the need arises.

One advantage of structuring things this way is that we can always add new 'advanced' elements if they become necessary. These models are generally orthogonal so clients don't have to make a hard choice between RCP/IDE but can adopt advanced model elements to meet their specific needs.
Comment 9 Eric Moffatt CLA 2009-12-07 14:09:33 EST
Created attachment 153951 [details]
Patch to refactor the UIElement class


Changes:

'visible' -> 'toBeRendered' : Means that this element should be rendered once its parent is. Avoids confusion...see 'visible' below

'factory' -> 'renderer' : That's what we call them now.

Added:

'visible' : a boolean which is true iff that element can be seen by a user (i.e. matches the 3.x meaning of the PartVisible event.
Comment 10 Eric Moffatt CLA 2009-12-07 14:11:11 EST
Committed in >20091207. Applied the patch.
Comment 11 Oleg Besedin CLA 2009-12-07 15:27:11 EST
A general comment from the viewpoint of extensibility.

Say, we want to add a new view. We can create an XMI "snippet" that describes that view and add it via an extension point that specifies a parent for the XMI "snippet".

That works well for model parts, but breaks down for, say, menu items.

Consider a "New Album" menu item in Photo Demo. It is linked to the command that does the work by using:

   command="//@commands.1"/

In order to add a menu item we'd need 3 XMI snippets (menu, command, handler) to go under different parents. However, even that won't work as we would have no idea as to what node number will happen to be assigned to the command.

This is really a general problem with this style of references, not limited to commands.
Comment 12 Remy Suen CLA 2009-12-09 11:08:11 EST
Do we expect the containment model to be "all-inclusive" (probably not the right word) from the application-level and down? At the moment, the code below will throw an exception.

MApplication app = MApplicationFactory.eINSTANCE.createApplication();
MWindow window = MApplicationFactory.eINSTANCE.createWindow();
app.getChildren().add(window);

MPart part = MApplicationFactory.eINSTANCE.createPart();
window.getChildren().add(part);

MMenu menu = MApplicationFactory.eINSTANCE.createMenu();
part.getMenus().add(menu);

Resource resource = new XMIResourceImpl(URI.createFileURI("test.xmi"));
resource.getContents().add((EObject) app);
// resource.getContents().add((EObject) menu);
resource.save(null);

If you uncomment the second last line then all is well. The code fails because the menu is not "known" or "contained" in the resource. The same thing happens for a part's tool bar. There is no problem for a window's (main) menu though.

If I try to use the reflective EMF editor to create a new child of a part, I only get 'Handlers Handler' and 'Bindings Key Binding', I would expect to get 'Menus Menu' and 'Toolbar Tool Bar' (or whatever the naming convention is).
Comment 13 Emeric Kwemou CLA 2009-12-09 12:46:59 EST
I all,
please can you provide a date when the UI Model will be frozen?
Thank you
Emeric
Comment 14 Paul Webster CLA 2009-12-09 12:59:03 EST
(In reply to comment #13)
> I all,
> please can you provide a date when the UI Model will be frozen?

It will definitely be frozen by our release date (July 2010, although it might possibly be earlier this year).

There are no plans to "freeze" it during development until it is both stable and functional.  If you are asking about model churn, most of the major churn (but not minor churn) should be finalized by e4 1.0 M3, which is beginning of Jan.

PW
Comment 15 Eric Moffatt CLA 2009-12-09 13:24:47 EST
Emeric, the exact date is TBD but we expect to get many of the less controversial changes (i.e. changing UIItem to UILabel, removing unnecessary elements...) in place soon. I opened this bug to gather up the remaining requirements and to let everyone discuss what should be done in the expectation that this will be the last 'deep' refactoring for the model.

It'll be more of a 'ratcheting down' effort than an insta-freeze. The current plans are to converge fairly early in the new year but not to 'lock it down' until after EclipseCon (so as not to impose extra churn for folks needing changes for their demos/talks...). 

At some point well before release (Boris?) we will start to treat the model as API, meaning that we will no longer be able to simply re-do the various demo Application.xmi files as we make 'breaking' changes but *require* that the previous version of the model be capable of being consumed and upgraded to the new structure seamlessly. This is non-optional in the sense that we'll need time to develop the techniques to do this and ensure that they're sufficient to handle real world cases...
Comment 16 Oleg Besedin CLA 2009-12-09 13:41:34 EST
By the way, is there a localization story for the XMI models?
Comment 17 Eric Moffatt CLA 2009-12-09 14:13:43 EST
This is a reply to Remy's comment 12 and Emeric's bug 297365...

Good pickup ! I'm very much in favor of being able to re-use chunks of the model in different places. This is a natural and demonstrably useful extension to the modeling paradigm.

There is some overlap between this and the perspective management code which effectively allows the *same element* to be referenced in the model from different levels. The main difference is that when using 'shared elements' you don't need to worry about the internal structure of the element (i.e. which editors are open or where they are in the editor area, it'll be the same for every perspective that that EA shows up in.

In this case we likely want to 'clone' the referenced model snippet and replace the original reference with the root of the newly copied element, avoiding the restriction on shared elements that only one 'real' version can be visible to the User at any time (we re-use the SWT widget so we *can't* show it in two places). Think macro pre-processor...

Does that sound OK to you folks ?
Comment 18 Remy Suen CLA 2009-12-09 14:53:48 EST
(In reply to comment #17)
> In this case we likely want to 'clone' the referenced model snippet and replace
> the original reference with the root of the newly copied element, avoiding the
> restriction on shared elements that only one 'real' version can be visible to
> the User at any time (we re-use the SWT widget so we *can't* show it in two
> places).

Where would these miscellaneous model objects live in the application tree?

As an aside, it would please me greatly if MKeyBinding extended from MApplicationElement. That's the only leaf model object that is not id'd.
Comment 19 Eric Moffatt CLA 2009-12-09 15:10:59 EST
+1 for making KeyBindings an ApplicationElement (it is one after all...;-).

As to where the correct level to store the snippets the most obvious place would be in the Application. Are there *any* scenarios we can think of where this doesn't make sense?

Note that this is again different from perspectives because the shared elements are actually shared on a per WBW basis (i.e. each instance of a WBW has its own 'real' view that it shares between *its* perspectives).
Comment 20 Eric Moffatt CLA 2009-12-10 08:00:07 EST
The word from Ed is..

<merks>	[#eclipse-modeling] emoffatt: The *.edit project has a plugin.properties file where all the things that need localizing are stored.

Guess we should take a look to make sure it'll handle our needs.
Comment 21 Paul Webster CLA 2009-12-10 09:47:28 EST
(In reply to comment #17)
> This is a reply to Remy's comment 12 and Emeric's bug 297365...
> 
> Good pickup ! I'm very much in favor of being able to re-use chunks of the
> model in different places. This is a natural and demonstrably useful extension
> to the modeling paradigm.

It seems to me the pattern that we're supporting is:

1) read in the static model

2) apply extensions ... like Oleg's model extensions, any other extension point that needs to be placed in the model (commands, handlers, etc)  This seems to me to be where Model snippets (for example, generated by the model extension) would be introduced in the model

3) apply the change recorder deltas (user changes, courtesy of Remy :-)

Whether a model extension creates a model snippet that it can clone and return or builds the model snippet on the fly is potentially left up to the contributor.

PW
Comment 22 Paul Webster CLA 2009-12-10 09:48:51 EST
(In reply to comment #18)
> 
> Where would these miscellaneous model objects live in the application tree?

Given that we support extension points for adding to the model (in phase 2) I'd be hesitant to "model" these in the model itself ... that seems like a duplication of something we have to support.

PW
Comment 23 Eric Moffatt CLA 2009-12-10 10:37:09 EST
Paul, good synopsis of our expected startup cycle. I thought the approach we were going to take for extensions is that they would be used to populate an area of the model specific to the extension.

With that in mind this looks like a candidate for testing out whether this approach will work. We already have a 'snippet' extension so we should create the corresponding model class and create a container for those elements in the MApplication. This will put us in the position to implement the start up logic as you described and see how it works in practice.

I'd suggest two more steps:

2b (code extensions): a pluggable way for someone to gain access to the populated model to add to it or tweak it programmatically. 

2c (integration support): a pluggable way for someone to gain access to the populated model to *remove* unwanted info. the reason to make this a separate step is that it ensures that folks handling integration issues (i.e. think Mule) can see the *complete* model.

Also, all of these steps should take place without eventing; either before we create the UIEventPublisher or with it disabled.
Comment 24 Eric Moffatt CLA 2009-12-10 10:42:19 EST
It turns out that the answer supplied by Ed for the localization in EMF isn't appropriate for our needs; it had to do with localizing the generated editor rather than the model's 'content' (i.e an MPart's label...).

A second query got this reply:

<merks>	The question could be asked: now does one normally do this without using EMF?
<merks>	Because then you could take that answer and use EMF to implement it.
<merks>	Generally it involves things like "keys" and a separate data structure where the keys are mapped to localized values.

So it looks like we're on our own for this...if any knows of a 'standard' pattern for doing this we'd appreciate the input.
Comment 25 Eric Moffatt CLA 2009-12-10 13:18:46 EST
Created attachment 154263 [details]
Patch to refactor the UIItem class


UIItem has been renamed to UILabel to avoid confusion with UIItem.

The 'name' attribute has been changed to 'label' 

Also slips in the change making KeyBinding a subclass of ApplicationElement in order to give it an id for merging purposes.
Comment 26 Eric Moffatt CLA 2009-12-10 13:21:05 EST
Committed in >20091210. Applied the patch.
Comment 27 Eric Moffatt CLA 2009-12-10 14:18:13 EST
Created attachment 154269 [details]
Patch removing unnecessary IDE elements


Also re-organizes the elements into new 'packages' (currently my ugly 'V____' pseudo-classes until Tom does his EMF magic) that have the base 'RCP' package and then separate 'advanced' packages for Commands, Trim and SharedElements.

This model structure quite a bit closer to the finished shape I think.
Comment 28 Eric Moffatt CLA 2009-12-10 14:26:21 EST
Committed in >20091210. Applied the patch.
Comment 29 Eric Moffatt CLA 2009-12-10 14:30:36 EST
While removing the EditorStack and ViewStack I had to remove some code that was managing the close buttons on CTabFolders (used in the old compatibility layer).

We'll need to come up with new attributes on the MStack to be able to replace this lost functionality:

showClose: boolean: indicates whether the close buttons appear at all
showCloseAlways: boolean: true means all tabs get a close button (like Editor tabs), otherwise only the selected tab gets one (like View tabs).
Comment 30 Eric Moffatt CLA 2009-12-10 14:45:07 EST
We likely also want to re-introduce the 'CompositePart', an MPart which is also an ElementContainer<PSCElement> to allow for Parts that want internal stacks/sashes...
Comment 31 Eric Moffatt CLA 2009-12-14 16:22:35 EST
Created attachment 154440 [details]
Patch to change the 'visible' attribute to match the SWT 'setVisible'


It turns out that we really need something that allows rendering of controls what are not necessarily visible (in the SWT sense). This started with a request from Yves for some way to control the Alpha value for newly created MWindows (see bug 297539). This model changes should cover his needs but we'd have needed this capability in any case. For example we will be using this to control the visibility of Detached Windows (which appear and disappear based on which perspective is active but who specifically do *not* get re-rendered every time the become visible).

This patch changes the intent of the existing 'visible' attribute (again) and uses its state to set the SWT visibility state, allowing for the rendering of widgets that are not initially visible 'on screen'.

There is a new attribute 'onTop' that will be used to match the current PartVisible state in eclipse.

Also updates the MWindow tests to verify that this is working for at least the top level shells.
Comment 32 Eric Moffatt CLA 2009-12-15 10:09:28 EST
Committed in >20091215. Applied the patch.

Yves, could you let me know if this is sufficient to solve the issues you were having with the designer ?
Comment 33 Eric Moffatt CLA 2009-12-15 10:34:48 EST
Committed in >20091215. Made the 'menus' and 'toolbar' attributes be containers (i.e. set the 'containment' == true.

This should help with Remy's merging work as well as help out with bug 297365.
Comment 34 Eric Moffatt CLA 2009-12-15 13:54:43 EST
Committed in >20091215. Removed the 'horizontal' attribute from the TrimContainer, it was unused because it was being inferred from the 'side' attribute.
Comment 35 Yves YANG CLA 2009-12-15 18:52:46 EST
(In reply to comment #32)
> Committed in >20091215. Applied the patch.
> 
> Yves, could you let me know if this is sufficient to solve the issues you were
> having with the designer ?
Eric, I'm sorry. It doesn't work in my case. I definitively need setAlpha(0). Otherwise, we see the Shell is opened. Don't worry, I found a way to override the standard WBWRenderer.
Comment 36 Eric Moffatt CLA 2009-12-16 14:36:10 EST
Created attachment 154588 [details]
Patch to fix the GenericTile 'weights'


This patch removes the 'weights' attribute from GenericTile and replaces it with a new attribute 'containerData' in UIElement.

This will allow different containers to define what information they need and give them a consistent place to associate this information with a particular model element. Our current SashForm implementation stores the 'weight' there but an 'ExpandBar' stack may wish to use this slot to store the preferred height of the 'open' control...

Also updated the merge tests...
Comment 37 Eric Moffatt CLA 2009-12-16 14:38:30 EST
Committed in >20091216. Applied the patch.
Comment 38 Eric Moffatt CLA 2009-12-16 15:16:34 EST
Yves, if you set the 'visible' attribute to false the resulting shell should not appear. See the code in the current WBWRenderer:

wbwShell.setVisible(element.isVisible());

If you do a setVisible(false) on the MWindow before calling createGui then it shouldn't be showing up should it ?

I'd have expected something like this to work (given an MWindow called 'mWindow'):

mWindow.setVisible(false);
Shell newShell = renderer.createGui(mWindow);
newShell.setAlpha(0);
mWindow.setVisible(true);

There's a 'visibilityListener' in the WBWRenderer that will cause the SWT.setVisible(true) to happen as a result of changing the model so you should end up with a 'visible' shell with an alpha already set to 0.
Comment 39 Eric Moffatt CLA 2009-12-16 16:01:16 EST
Committed in >20091216. Added a 'style' attribute to the ApplicationElement.

This is intended to be used both by the CSS styling engine and the rendererFactory to allow for the same UI model classes to be rendered differently.
Comment 40 Yves YANG CLA 2009-12-16 17:38:06 EST
(In reply to comment #38)
> Yves, if you set the 'visible' attribute to false the resulting shell should
> not appear. See the code in the current WBWRenderer:
> 
> wbwShell.setVisible(element.isVisible());
> 
> If you do a setVisible(false) on the MWindow before calling createGui then it
> shouldn't be showing up should it ?
> 
> I'd have expected something like this to work (given an MWindow called
> 'mWindow'):
> 
> mWindow.setVisible(false);
> Shell newShell = renderer.createGui(mWindow);
> newShell.setAlpha(0);
> mWindow.setVisible(true);

This is exactly what I did. 

> 
> There's a 'visibilityListener' in the WBWRenderer that will cause the
> SWT.setVisible(true) to happen as a result of changing the model so you should
> end up with a 'visible' shell with an alpha already set to 0.
I have spent sometime to place shell.setAlpha(0) line by line to identify where the shell flash occurs. I have noticed it occurs if I place the code after this block 

  if (element instanceof MElementContainer) {
	factory.processContents((MElementContainer<MUIElement>) element);
  }
of the class PartRenderingEngine (line 292). 

It seems when the children get created. It is too late.
Comment 41 Eric Moffatt CLA 2009-12-17 13:26:33 EST
OK, I'll take a look into this...perhaps some other part of the rendering protocol is incorrectly setting the SWT visibility back to 'true'.
Comment 42 Yves YANG CLA 2009-12-17 13:47:47 EST
(In reply to comment #41)
> OK, I'll take a look into this...perhaps some other part of the rendering
> protocol is incorrectly setting the SWT visibility back to 'true'.
I'll prepare a psf file, you can check out the e4 Designer easily and test it.
Comment 43 Eric Moffatt CLA 2009-12-17 14:45:21 EST
Thanks Yves, you may want to ping Paul to see if we should add this PSF to the other ones we have in 'releng'.
Comment 44 Eric Moffatt CLA 2009-12-17 14:46:03 EST
I just tried the refactoring to replace the MWindow's x/y/width/height with a new MBounds element but after seeing code like this:

mWindow.setWidth(600);
mWindow.setHeight(400);

turn into:

MBounds bounds = mWindow.getBounds();
bounds.setWidth(600);
bounds.setHeight(400);
mWindow.setBounds(bounds);

I'm no longer gonna stick by my comment #7, it appears that it's not more convenient for our clients...

The listener code will have to be 'smart' (asynch exec, UIJob?) about avoiding flicker when some combination of the values change at the same time.
Comment 45 Yves YANG CLA 2009-12-17 14:52:20 EST
(In reply to comment #43)
> Thanks Yves, you may want to ping Paul to see if we should add this PSF to the
> other ones we have in 'releng'.
Sure, it is what I intent to do. But I think it is not yet ready to be in the build.
Comment 46 Eric Moffatt CLA 2009-12-17 15:15:12 EST
OK, I think round 2 is done (and I hope/expect that there won't be a round 3...;-).

Thanks for the input folks !!

This does not mean that the model is done, just that the changes that affect our current demos and the general structure of the model are complete. The remaining changes can be dealt with in separate defects I think.

I'll mark this one as 'FIXED'...finally...;-).

Tom, I think we've got a 'package' structure that makes some sense now. Feel free to do your EMF magic whenever you have the time...
Comment 47 Yves YANG CLA 2009-12-17 18:43:55 EST
(In reply to comment #45)
> (In reply to comment #43)
> > Thanks Yves, you may want to ping Paul to see if we should add this PSF to the
> > other ones we have in 'releng'.
> Sure, it is what I intent to do. But I think it is not yet ready to be in the
> build.

I have created a bug 298137 about the psf file