Bug 107443 - [Presentations] Ability for differing view stack presentations within the one presentation extension
Summary: [Presentations] Ability for differing view stack presentations within the one...
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.1   Edit
Hardware: PC Windows 2000
: P3 normal (vote)
Target Milestone: 3.3 M5   Edit
Assignee: Paul Webster CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2005-08-19 10:35 EDT by Chris Gross CLA
Modified: 2007-02-06 12:29 EST (History)
2 users (show)

See Also:


Attachments
first draft patch (20.09 KB, patch)
2006-12-22 19:02 EST, Chris Gross CLA
no flags Details | Diff
Updated patch (18.07 KB, patch)
2007-01-05 14:52 EST, Chris Gross CLA
no flags Details | Diff
layout properties v03 (18.00 KB, patch)
2007-01-16 08:29 EST, Paul Webster CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Gross CLA 2005-08-19 10:35:48 EDT
Currently, when creating a presentation, you can differ how views are 
presentation in only a few ways.  Standalone vs regular views and within 
standalone, with or without a title.  For many applications this is 
insufficient for branding or other reasons.  

I would like to see the ability to request the views be presented in a specific 
way.  I see this as being implemented by the perspective by passing in some 
untyped object.  The presentation API would interogate that object to determine 
how the view stack is created.
Comment 1 Chris Gross CLA 2005-08-19 10:38:56 EDT
I should mention that this is similar to another request I entered as bug 89834 
but it is different.  89834 requests that parts be able to supply the 
presentation with supplemental properties that may affect how the part is 
displayed (i.e. an extra image or string of text) within the stack.  This 
request asks for the perspective to be able to determine how a particular stack 
itself is represented.
Comment 2 Paul Webster CLA 2006-09-28 15:15:17 EDT
Is this still a problem in 3.3?

PW
Comment 3 Chris Gross CLA 2006-09-28 15:21:50 EDT
I'm not sure.  Has 3.3 added a feature for differing stack presentations (i.e. the ability to show one stack as tabs and another as an expandbar or something)?
Comment 4 Paul Webster CLA 2006-09-28 19:14:04 EDT
There is no planned work for presentation in 3.3, but we'll still accept patches if the community wants this badly enough.

PW
Comment 5 Chris Gross CLA 2006-12-20 13:18:52 EST
Paul, 

I am considering working on this and providing a patch but I would like to know if this would be considered for M5 if I get it in soon.  I think the work would be relatively easy and I have some time over Xmas.

My approach is:
Add overloaded varieties of all the methods on IPageLayout that always create stacks: createFolder, createPlaceholderFolder, createStandaloneView, createStandaloneViewPlaceholder.
These methods would take an extra parameter of type Object as presentationData.
Add overloaded methods to AbstractPresentationFactory for createViewPresentation and createStandaloneViewPresentation that take this Object - presentationData.
These two new methods in AbstractPresentationFactory would delegate to the older methods by default (throwing away the presentationData).
Update all existing presentation/perspective glue code to call these new methods on AbstractPresentationFactory (with null as the presentationData if not supplied).

This would ensure that the old API works and if you want the new API you just have to override the two new overloaded versions.  It would probably make sense to deprecate the older methods on AbstractPresentationFactory but its not necessary.
Comment 6 Paul Webster CLA 2006-12-21 07:35:44 EST
Some things to think about:

1. I would consider putting String->String properties on IPlaceholderFolderLayout (which means IFolderLayout would get them).  Just like an IViewLayout has setMoveable(*).  

2. What about views that are added with addView(*)?  They can create folders.  Should we put the same properties on an IViewLayout, such that when a view is added its properties get set on the IFolderLayout?  Or would we add a method to IPageLayout like getFolderForView(*).

3. if we do a getFolderForView(*), that would also return the IFolderLayout for a standalone view and making everything depend on the IFolderLayout properties bag.

It appears you could then add the properties the the PartStack, which is wrapped by a folder in the perspective.

You could hook your presentation properties into the saveState(*)/restoreState(*) lifecycle to survive session restarts.

Then I guess AbstractPresentationFactory methods could be augmented to take a Map to allow anybody to retrieve layout properties.


Other things to consider ... should these properties be dynamic and propogate changes (I'm guessing no, but think about it).

PW
Comment 7 Chris Gross CLA 2006-12-21 10:36:13 EST
Originally I hadn't thought about persistence between sessions.  A string map make sense.  I had also just assumed that if you wanted to use this feature you would have to create your folder seperately.  So you wouldn't use addView because it wouldn't necessary create a folder.  But getFolderForView sounds better and seems like a lighter API addition.

I'll investigate more and post back later.
Comment 8 Chris Gross CLA 2006-12-22 19:02:54 EST
Created attachment 56116 [details]
first draft patch

Heres my first pass at a patch.  I followed your guidance and it turned out to be pretty straightforward.  I'm still testing but I think its complete.

The resulting API seems good.  There is one weirdness.  The getFolderForView returns IPlaceholderFolderLayout since thats the base type.  This is a little weird when (most of the time) neither the view you are passing in nor the folder you are retrieving are place holders.  

Anyway, merry Xmas.  Hope you guys find sometime to look at this before M5.
Comment 9 Paul Webster CLA 2007-01-05 12:58:04 EST
It looks pretty good so far, with the following comments:

IPageLayout:
for the javadoc "@param id " add "Must not be <code>null</code>."

IPlaceholderFolderLayout:
Just a note about the property id not being null.  Also, will doing a set value==null remove the property, or is it not allowed (you'll have to not save/restore null values)

Also for your thought ... you might want to add a note that these properties are for the IPerpspectiveFactory to create.  It's a one-shot deal and they don't fire any notification if they were changed after creation.

PageLayout#createFolder(*):
make sure you add both folder layouts to your map.  Same in PageLayout#createPlaceholderFolder(*)

PageLayout#getFolderForView(*):
If the map doesn't create a folder, do you really want to create one?  I would just return null (which is consistent with your javadoc).

PartStack#copyAppearanceProperties(*)
Is this necessary?  What are the usecases?
1) if you drag a view out of your only "expandbar stack" you wouldn't want it to create another expandbar stack
2) if you drag a view into an existing stack, you wouldn't want to augment the properties
3) should it clear the target properties view properties? (I don't think so, but what would be a usecase)
I realize the PartSashContainer seems to imply this is a good idea ... Nick, do you have any comments?

PresentationFactoryUtil#createPresentation(*)
Should the Map not also be sent to createEditorPresentation(*)?

Chris, if you want to update the patch I'll take another look at it.

Now that I've seen the patch I have another suggestion.  Instead of updating AbstractPresentationFactory to take a map, why don't we just update IStackPresentationSite with a String getProperty(String id) method.  The PartStack$DefaultStackPresentationSite can just implement it using PartStack.this.getProperty(id) ... the IStackPresentationSite is already passed to the creation methods in AbstractPresentationFactory and PresentationFactoryUtil.


PW
Comment 10 Chris Gross CLA 2007-01-05 13:46:48 EST
(In reply to comment #9)
> It looks pretty good so far, with the following comments:
> IPageLayout:
> for the javadoc "@param id " add "Must not be <code>null</code>."
> IPlaceholderFolderLayout:
> Just a note about the property id not being null.  Also, will doing a set
> value==null remove the property, or is it not allowed (you'll have to not
> save/restore null values)
> Also for your thought ... you might want to add a note that these properties
> are for the IPerpspectiveFactory to create.  It's a one-shot deal and they
> don't fire any notification if they were changed after creation.
> PageLayout#createFolder(*):
> make sure you add both folder layouts to your map.  Same in
> PageLayout#createPlaceholderFolder(*)
> PageLayout#getFolderForView(*):
Ok.  Will update patch.

> If the map doesn't create a folder, do you really want to create one?  I would
> just return null (which is consistent with your javadoc).
I was thinking all views would be guaranteed to have a folder.  Certainly they all end up in a folder/stack.  If I return null then this feature only works when someone first creates a folder and then sticks a view in it.  In other words, adding views with addView(*).  The null comment in the javadoc is referencing a situation where a user is asking for a folder for a view that was never added.  Maybe I'm missing something.


> PartStack#copyAppearanceProperties(*)
> Is this necessary?  What are the usecases?
> 1) if you drag a view out of your only "expandbar stack" you wouldn't want it
> to create another expandbar stack
> 2) if you drag a view into an existing stack, you wouldn't want to augment the
> properties
> 3) should it clear the target properties view properties? (I don't think so,
> but what would be a usecase)
> I realize the PartSashContainer seems to imply this is a good idea ... Nick, do
> you have any comments?
The copy appearance stuff only happens when you are only dragging the individual view and you are dropping it in an empty area (i.e. a new stack will be created).  So if I drag my 'expandbar view' to an empty area I would expect to still look like an expandbar.  So this doesn't (at least its not my intention to) change the look of an existing stack.  


> PresentationFactoryUtil#createPresentation(*)
> Should the Map not also be sent to createEditorPresentation(*)?
> Chris, if you want to update the patch I'll take another look at it.
> Now that I've seen the patch I have another suggestion.  Instead of updating
> AbstractPresentationFactory to take a map, why don't we just update
> IStackPresentationSite with a String getProperty(String id) method.  The
> PartStack$DefaultStackPresentationSite can just implement it using
> PartStack.this.getProperty(id) ... the IStackPresentationSite is already passed
> to the creation methods in AbstractPresentationFactory and
> PresentationFactoryUtil.
> PW
Yea.  I was thinking about this as I was writing the patch.  I will include that in the new patch.
Comment 11 Chris Gross CLA 2007-01-05 13:50:31 EST
> I was thinking all views would be guaranteed to have a folder.  Certainly they
> all end up in a folder/stack.  If I return null then this feature only works
> when someone first creates a folder and then sticks a view in it.  In other
> words, adding views with addView(*).  The null comment in the javadoc is
> referencing a situation where a user is asking for a folder for a view that was
> never added.  Maybe I'm missing something.


"In other words, adding views with addView(*)." should have been "In other words, adding views with addView(*) wouldn't allow a user to set any properties."
Comment 12 Paul Webster CLA 2007-01-05 14:00:42 EST
(In reply to comment #11)
> "In other words, adding views with addView(*)." should have been "In other
> words, adding views with addView(*) wouldn't allow a user to set any
> properties."
> 

I see what you mean ... addView(*) creates the ViewStack but FolderLayout is not actually saved anywhere ... it's a throw-away proxy.  That should be OK then.

BTW: what happens when someone adds a placeholder instead of a view?  Does it work the same?

PW
Comment 13 Chris Gross CLA 2007-01-05 14:28:52 EST
> BTW: what happens when someone adds a placeholder instead of a view?  Does it
> work the same?
> PW

Yup.  If you create a placeholder folder, you can add views and set the folder properties.  When that stack is finally created, those properties are passed to the presentation factory.
Comment 14 Chris Gross CLA 2007-01-05 14:52:48 EST
Created attachment 56487 [details]
Updated patch

Here's an updated patch.  This patch removes the modifications to the presentation factory and makes the properties acccessible via the site.
Comment 15 Paul Webster CLA 2007-01-05 16:01:40 EST
(In reply to comment #13)
> 
> Yup.  If you create a placeholder folder, you can add views and set the folder
> properties.  When that stack is finally created, those properties are passed to
> the presentation factory.

I mean addPlaceholder(*) instead of createPlaceholderFolder(*).  Does addPlaceholder(*) make it work the same as addView(*)?

BTW: The new patch looks good.  Could you just check removePlaceholder(*)?  It seems to be removing some items from those maps.  Is there anywhere we have to clean up our folder to folder map?

I should get another chance to review it on Tuesday.

PW
Comment 16 Chris Gross CLA 2007-01-06 21:44:02 EST
> I mean addPlaceholder(*) instead of createPlaceholderFolder(*).  Does
> addPlaceholder(*) make it work the same as addView(*)?
Hmm.  I didn't realize this but it doesn't work the same.  When adding a regular placeholder, a ViewStack isn't created until the view is opened (ln306 of ViewSashContainer).  This means I can't create a FolderLayout to return from getFolderForView.  I'm going to look at what it would take to move this into PageLayout but I'm not sure what side-effects this may have.

Comment 17 Paul Webster CLA 2007-01-07 09:43:36 EST
(In reply to comment #16)
> Hmm.  I didn't realize this but it doesn't work the same.  When adding a
> regular placeholder, a ViewStack isn't created until the view is opened (ln306
> of ViewSashContainer).  This means I can't create a FolderLayout to return from
> getFolderForView.  I'm going to look at what it would take to move this into
> PageLayout but I'm not sure what side-effects this may have.

For this we need Eric's input.  Our usecase is with a placeholder, the ViewSashContainer creates the ViewStack when the View shows up ... and then what happens to the ViewStack when that one view goes away.  Does it get replaced with the old placeholder?  Or just a new placeholder with an ID?  Or with a placeholder for the ViewStack itself?

Is a potential solution to temporarily store the properties on the Placeholder?  Although I'm skeptical about this, when the ViewStack is instantiated the properties could be copied over.  Then we'd have to create a "fake" PlaceholderFolder to allow the perspective to set the properties.

Eric, thoughts?

PW
Comment 18 Chris Gross CLA 2007-01-09 12:39:15 EST
For what its worth, adding properties to the placeholder is not important to me.  I think its an acceptable workaround to require a user to create a placeholder folder if they want to apply stack properties.  I could just explicitly mention this in the javadoc as not supported.
Comment 19 Paul Webster CLA 2007-01-15 08:32:35 EST
I would probably be able to live with the "put a placeholder in a folder to add layout properties" restriction.

Just waiting for some input from Eric.

PW
Comment 20 Paul Webster CLA 2007-01-16 08:29:42 EST
Created attachment 56954 [details]
layout properties v03

Chris, I've made some minor formatting changes to your patch, and changed 2 lines in createFolder(*) and createPlaceholderFolder(*).

Could you check it over and see that it works for you as you would expect.

Then I think this is ready to go in, so it'll be in for next week's I build.

PW
Comment 21 Chris Gross CLA 2007-01-16 10:56:52 EST
Yea it looks good. I should have realized I didn't need to create a new folder layout for those existing parts in the those two methods.  

I've tested it with my examples and everything works as expected.

Comment 22 Paul Webster CLA 2007-01-17 08:50:17 EST
Released Chris' patch >20070117
PW
Comment 23 Chris Gross CLA 2007-01-17 09:09:20 EST
Thanks for all your help and hard work!
Comment 24 Paul Webster CLA 2007-02-06 12:29:08 EST
I20070206-0010
PW