Community
Participate
Working Groups
The property view should allow to open multiple instances. Use cases/requirements/ideas can be found at http://wiki.eclipse.org/Platform_UI/Multi-instance_Properties_View
CC'ing Eric as owner of the PropertiesView component, and Anthony as the owner of the TabbedProperties component.
Created attachment 113604 [details] draft v1 draft version of the patch, it's work in progress. So far it does allow: - Pin/Unpin a property sheet (PS) - Create multiple instances of PS (pinning the parent) - Allows to overwrite the default PropertySheetPage via an adapter - Closes (hides) all secondary instances of PS during workbench shutdown - Doesn't create new API (bundle minor has been incremented to 3.4 any for both o.e.ui.views as well as o.e.ui.tests to indicate this new feature) It misses: - More tests - Documentation/Help - "Show in" > PS - ... - Comments :)
Marking Bug #248742 as a blocker to not duplicate icons (draft v1 includes pin icons though).
Marking Bug #248926 as a blocker because it's required to implement "Show in" > "Properties"
Created attachment 114388 [details] New patch version ready for review
Created attachment 114389 [details] Even newer removing a commented field in PropertySheet Btw. the new.gif is a copy of org.eclipse.ui.console/icons/full/elcl16/new_con.gif
(In reply to comment #6) > Created an attachment (id=114389) [details] There are types missing from this patch, such as NewPropertySheetHandler and PropertyShowInContext. Could you please upload a complete patch? Thanks!
(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=114389) [details] [details] > > There are types missing from this patch, such as NewPropertySheetHandler and > PropertyShowInContext. Could you please upload a complete patch? Thanks! That's odd. They appear to be there (in attachment 114389 [details]). I mean, if I scroll to the bottom I can see both of those classes listed at the bottom of the attachment? Perhaps there is something wrong with the patch application? I'll re-sync with HEAD and take a look at work tomorrow.
Created attachment 115122 [details] Missing icon which is referenced in plugin.xml
Created attachment 115123 [details] Screenshot depicting the missing files. (In reply to comment #7) > There are types missing from this patch, such as NewPropertySheetHandler and > PropertyShowInContext. Could you please upload a complete patch? Thanks! Boris, you weren't just referring to the "(file does not exist)" message from the patch wizard, I hope? That just tells you the file isn't in your workspace and that the patch is going to add it, no? You should override the org.eclipse.ui.views/icons/full/elcl16/new.gif with Markus's attachment 115122 [details] or SWT will crash (at least, it does for me on Windows XP) because it can't load the (corrupted) file referenced by the bundle.
Sorry, I should have updated the bug after talking to Remy on IRC. All is well, I was able to apply the patch (after manually removing the binary content).
Btw, the patch looks good from a code quality point of view, and thanks a lot for writing tests! I would like to see the use cases documented that you used as the basis for your design decisions. Could you update the wiki page with short paragraphs for each of the cases that you think are important? I am looking for relatively concrete use cases, maybe based on well-known examples like the library.ecore file, or example GEF/GMF editors - this will make it possible for others to reproduce the use case "steps".
I gave the patch a test run. Cool feature. With the GEF logic editor, multiple properties view and pinning seemed to work fine. Undo / Redo, changing values from both the editor and properties view syncs up fine. I also tested with the EMF Ecore editor. Everything also seemed fine but I did get one exception: WARNING: Prevented recursive attempt to activate part org.eclipse.ui.views.PropertySheet while still in the middle of activating part org.eclipse.ui.views.PropertySheet Other issues I noticed: When I restart the workbench, the second properties view is not there. When I unpin, the current selection should immediately be shown. When I select a new item after unpinning, the properties view flashes to the previous selection and then the new one. The patch does not seem to support an adaptable for IPropertySheetPage. So the tabbed properties view and another IPropertySheetPage example I have do not work at all in a second properties view. Hopefully the platform guys have the official IPropertySheetPage adapter to test out. The tabbed properties view is also broken when pinned, but I think the issue is in the GEF logic editor with tabbed properties view support.
(In reply to comment #13) > I gave the patch a test run. Cool feature. Thanks for testing it! > I also tested with the EMF Ecore editor. Everything also seemed fine but I did > get one exception: > WARNING: Prevented recursive attempt to activate part > org.eclipse.ui.views.PropertySheet while still in the middle of activating part > org.eclipse.ui.views.PropertySheet Do you have the stack trace that comes with this? We'd be able to see how it was caused.
(In reply to comment #14) > Do you have the stack trace that comes with this? We'd be able to see how it > was caused. !ENTRY org.eclipse.ui.workbench 4 0 2008-10-15 10:47:21.986 !MESSAGE WARNING: Prevented recursive attempt to activate part org.eclipse.ui.views.PropertySheet while still in the middle of activating part org.eclipse.ui.views.PropertySheet !STACK 0 java.lang.RuntimeException: WARNING: Prevented recursive attempt to activate part org.eclipse.ui.views.PropertySheet while still in the middle of activating part org.eclipse.ui.views.PropertySheet at org.eclipse.ui.internal.WorkbenchPage.setActivePart(WorkbenchPage.java:3441) at org.eclipse.ui.internal.WorkbenchPage.requestActivation(WorkbenchPage.java:3034) at org.eclipse.ui.internal.PartPane.requestActivation(PartPane.java:272) at org.eclipse.ui.internal.PartPane.handleEvent(PartPane.java:236) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1003) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1027) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1008) at org.eclipse.swt.widgets.Shell.setActiveControl(Shell.java:1353) at org.eclipse.swt.widgets.Control.sendFocusEvent(Control.java:2443) at org.eclipse.swt.widgets.Widget.wmSetFocus(Widget.java:2266) at org.eclipse.swt.widgets.Control.WM_SETFOCUS(Control.java:4414) at org.eclipse.swt.widgets.Tree.WM_SETFOCUS(Tree.java:6846) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3855) at org.eclipse.swt.widgets.Tree.windowProc(Tree.java:5791) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4528) at org.eclipse.swt.internal.win32.OS.SetFocus(Native Method) at org.eclipse.swt.widgets.Control.forceFocus(Control.java:974) at org.eclipse.swt.widgets.Control.setFocus(Control.java:2811) at org.eclipse.swt.widgets.Composite.setFocus(Composite.java:927) at org.eclipse.ui.views.properties.PropertySheetPage.setFocus(PropertySheetPage.java:538) at org.eclipse.ui.part.PageBookView.setFocus(PageBookView.java:869) at org.eclipse.ui.internal.PartPane.setFocus(PartPane.java:325) at org.eclipse.ui.internal.WorkbenchPage$3.run(WorkbenchPage.java:622) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:37) at org.eclipse.core.runtime.Platform.run(Platform.java:880) at org.eclipse.ui.internal.WorkbenchPage.activatePart(WorkbenchPage.java:617) at org.eclipse.ui.internal.WorkbenchPage.setActivePart(WorkbenchPage.java:3487) at org.eclipse.ui.internal.WorkbenchPage.requestActivation(WorkbenchPage.java:3034) at org.eclipse.ui.internal.PartPane.requestActivation(PartPane.java:272) at org.eclipse.ui.internal.PartPane.handleEvent(PartPane.java:236) at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:84) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1003) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1027) at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1008) at org.eclipse.swt.widgets.Shell.setActiveControl(Shell.java:1353) at org.eclipse.swt.widgets.Shell.WM_MOUSEACTIVATE(Shell.java:2195) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3830) at org.eclipse.swt.widgets.Canvas.windowProc(Canvas.java:337) at org.eclipse.swt.widgets.Decorations.windowProc(Decorations.java:1576) at org.eclipse.swt.widgets.Shell.windowProc(Shell.java:1937) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4541) at org.eclipse.swt.internal.win32.OS.DefWindowProcW(Native Method) at org.eclipse.swt.internal.win32.OS.DefWindowProc(OS.java:2366) at org.eclipse.swt.widgets.Scrollable.callWindowProc(Scrollable.java:79) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3877) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4541) at org.eclipse.swt.internal.win32.OS.DefWindowProcW(Native Method) at org.eclipse.swt.internal.win32.OS.DefWindowProc(OS.java:2366) at org.eclipse.swt.widgets.Scrollable.callWindowProc(Scrollable.java:79) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3877) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4541) at org.eclipse.swt.internal.win32.OS.DefWindowProcW(Native Method) at org.eclipse.swt.internal.win32.OS.DefWindowProc(OS.java:2366) at org.eclipse.swt.widgets.Scrollable.callWindowProc(Scrollable.java:79) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3877) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4541) at org.eclipse.swt.internal.win32.OS.DefWindowProcW(Native Method) at org.eclipse.swt.internal.win32.OS.DefWindowProc(OS.java:2366) at org.eclipse.swt.widgets.Scrollable.callWindowProc(Scrollable.java:79) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3877) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4541) at org.eclipse.swt.internal.win32.OS.DefWindowProcW(Native Method) at org.eclipse.swt.internal.win32.OS.DefWindowProc(OS.java:2366) at org.eclipse.swt.widgets.Scrollable.callWindowProc(Scrollable.java:79) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3877) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4541) at org.eclipse.swt.internal.win32.OS.DefWindowProcW(Native Method) at org.eclipse.swt.internal.win32.OS.DefWindowProc(OS.java:2366) at org.eclipse.swt.widgets.Tree.callWindowProc(Tree.java:1438) at org.eclipse.swt.widgets.Tree.windowProc(Tree.java:5698) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4541) at org.eclipse.swt.internal.win32.OS.CallWindowProcW(Native Method) at org.eclipse.swt.internal.win32.OS.CallWindowProc(OS.java:2274) at org.eclipse.swt.widgets.Tree.callWindowProc(Tree.java:1529) at org.eclipse.swt.widgets.Control.windowProc(Control.java:3877) at org.eclipse.swt.widgets.Tree.windowProc(Tree.java:5791) at org.eclipse.swt.widgets.Display.windowProc(Display.java:4541) at org.eclipse.swt.internal.win32.OS.PeekMessageW(Native Method) at org.eclipse.swt.internal.win32.OS.PeekMessage(OS.java:2880) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3417) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2382) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2346) at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2198) at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:493) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:288) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:488) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:113) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:193) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:386) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:79) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:618) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:549) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:504) at org.eclipse.equinox.launcher.Main.run(Main.java:1236) at org.eclipse.equinox.launcher.Main.main(Main.java:1212)
(In reply to comment #13) > When I restart the workbench, the second properties view is not there. That's intentional because the property sheet doesn't store states (e.g. current selection) and reopening all empty property sheet instances is kind of pointless. > When I unpin, the current selection should immediately be shown. When I select > a new item after unpinning, the properties view flashes to the previous > selection and then the new one. That's not easily possible as the property sheet has no real way of knowing what to show, because it depends on the active part which, in this case, is just the property sheet. @see org.eclipse.ui.views.properties.PropertySheet.saveState(IMemento) > The patch does not seem to support an adaptable for IPropertySheetPage. So the > tabbed properties view and another IPropertySheetPage example I have do not > work at all in a second properties view. Hopefully the platform guys have the > official IPropertySheetPage adapter to test out. Can you provide steps or screens? org.eclipse.ui.views.properties.tabbed appears to work fine here.
(In reply to comment #16) > (In reply to comment #13) > > When I restart the workbench, the second properties view is not there. > > That's intentional because the property sheet doesn't store states (e.g. > current selection) and reopening all empty property sheet instances is kind of > pointless. I suppose it's arguable as to whether it is pointless or not. If I open multiple 'Console' views and restart Eclipse, they all show up. We could consider introducing a boolean preference or possibly just let them stay up, like the 'Console' view, for consistency.
(In reply to comment #16) > (In reply to comment #13) > > When I restart the workbench, the second properties view is not there. > > That's intentional because the property sheet doesn't store states (e.g. > current selection) and reopening all empty property sheet instances is kind of > pointless. > I did not think this was a properties view issue, this was a perspective issue. If I customize my perspective with two properties views, do they not get saved in the perspective properly?
(In reply to comment #16) > > The patch does not seem to support an adaptable for IPropertySheetPage. So the > > tabbed properties view and another IPropertySheetPage example I have do not > > work at all in a second properties view. Hopefully the platform guys have the > > official IPropertySheetPage adapter to test out. > > Can you provide steps or screens? org.eclipse.ui.views.properties.tabbed > appears to work fine here. > What implementation of the tabbed proeprties view are you testing with? Try the GEF logic editor with tabbed properties view support. It is in CVS at /cvsroot/eclipse/org.eclipse.ui.examples.views.properties.tabbed/org.eclipse.ui.examples.views.properties.tabbed.logic
Created attachment 115414 [details] Example of a view providing an IPropertySheetPage The comment about the IPropertySheetPage not working is not correct. I created a simple example and the pinning and multiple properties view work correctly. The example provides a name property for the nodes in the tree. Check out the getAdapter() method to toggle the two modes. public Object getAdapter(Class adapter) { /* TODO: comment this line out if you want the regular properties view */ if (adapter == IPropertySheetPage.class) { return new ExamplePropertySheetPage(); } return super.getAdapter(adapter); }
If a plugin wants to create a new instance of the view, (say in my GEF editor, select two elements -> right click -> compare with each other), it can invoke the NewPropertySheetHandler command. But there is no way to get hold of the new instance. Either the command's execute can return the sheet instead of null or the command can take the secondary id as an optional parameter. Or both :)
(In reply to comment #21) > Either the command's execute can return the sheet instead of null or > the command can take the secondary id as an optional parameter. Well, changing the handler to return the view would be breaking the contract of IHandler [1] so I don't think we want to do this. [1] - http://help.eclipse.org/stable/nftopic/org.eclipse.platform.doc.isv/reference/api/org/eclipse/core/commands/IHandler.html#execute(org.eclipse.core.commands.ExecutionEvent)
(In reply to comment #22) > Well, changing the handler to return the view would be breaking the contract of > IHandler [1] so I don't think we want to do this. Interesting! I always thought its OK to return a value from a command. Never cared to look into the java doc of that method.
(In reply to comment #22) > Well, changing the handler to return the view would be breaking the contract of > IHandler [1] so I don't think we want to do this. It might be time for the contract to be updated, though. The declarative command allows a return type to be specified, and the javadoc for ParameterizedCommand and Command mention that the return type *may* be null. It might be that the IHandler javadoc needs to be updated, so that it can return an Object whose type is compatible with the returnTypeId. PW
Created attachment 115890 [details] version 4 Revised patch that: - not simply reuses one of the pinned PV with the same input randomly but searches for the one that is on top first - fixes PropertyShowInContext to not inherit from ShowInContext anymore as inheriting the input field causes problems with equality What needs to be discussed though is, if closing secondary instances makes sense or not. It definitely causes problem with PVs in inactive perspectives (PVs aren't closed).
(In reply to comment #13) > > The tabbed properties view is also broken when pinned, but I think the issue is > in the GEF logic editor with tabbed properties view support. > I forgot to send out the confirmation that the tabbed properties view works with the pinning feature. A number of clients are caching a single instance of the tabbed properties sheet page, which works when there is one properties view, but breaks when there are multiple properties views. This was exactly the issue with the GEF logic editor I mentioned above. Clients will need to fix their code if they want their editors to work with the pinning feature.
(In reply to comment #12) > I would like to see the use cases documented that you used as the basis for > your design decisions. Could you update the wiki page with short paragraphs for > each of the cases that you think are important? I am looking for relatively > concrete use cases, maybe based on well-known examples like the library.ecore > file, or example GEF/GMF editors - this will make it possible for others to > reproduce the use case "steps". This is still important to me. I would like to use one of our weekly status calls to run through the scenarios, and collect feedback from the team.
(In reply to comment #27) > This is still important to me. I would like to use one of our weekly status > calls to run through the scenarios, and collect feedback from the team. If necessary, I could attend tomorrow's status call. Markus
(In reply to comment #28) > (In reply to comment #27) > > > This is still important to me. I would like to use one of our weekly status > > calls to run through the scenarios, and collect feedback from the team. > > If necessary, I could attend tomorrow's status call. Thanks - it would be good to prepare that though. Would you be able to list some use cases before the call, or should we do it next week?
(In reply to comment #29) > > Thanks - it would be good to prepare that though. Would you be able to list > some use cases before the call, or should we do it next week? Lets do it next week then as I need to finish other stuff up first. Markus
(In reply to comment #30) > Lets do it next week then as I need to finish other stuff up first. Markus, ping me when you want to talk about this. I want this done before I'm back in Canada or else Boris will be on my heels when I head back since I don't have the Atlantic protecting me from him.
Created attachment 119782 [details] version 5 Applies against latest HEAD (doesn't include the icons)
Created attachment 119785 [details] version 6 v5 contained parts of prototype code.
(In reply to comment #33) > Created an attachment (id=119785) [details] > version 6 PropertySheet's getShowInContext() and show(ShowInContext) needs to update its javadocs with @since tags (I'm getting compiler errors from API tooling). While the bundle version has upped to 3.4, this will only be in 3.5, so I guess one needs to ask whether these two should be tagged @since 3.4 or @since 3.5.
(In reply to comment #30) > Lets do it next week then as I need to finish other stuff up first. We whipped this up a little bit ago. Hopefully, it's what was desired. http://wiki.eclipse.org/Platform_UI/Multi-instance_Properties_View#Use_cases Anthoy, I don't think I've heard from you on the conference call yet, will you be joining us tomorrow?
(In reply to comment #35) > > Anthony, I don't think I've heard from you on the conference call yet, will you > be joining us tomorrow? > Yes.
(In reply to comment #33) > Created an attachment (id=119785) [details] > version 6 Is the 'Show In' action intended to spawn a new view even if there's an existing view that's rendering the current selection?
(In reply to comment #35) > We whipped this up a little bit ago. Hopefully, it's what was desired. > http://wiki.eclipse.org/Platform_UI/Multi-instance_Properties_View#Use_cases I updated the page with some steps. I'm not entirely convinced that this is sufficient though. Markus, please add more.
Much better now, thanks for writing this up Remy.
Kevin, could you have a look at this too?
I really the idea of the multi-instances property views! Having played with it a little, it seems that there are few things that users like me would expect: => High-level <= 1) It would be nice to be able to differentiate between multiple property views. As it is, they all are called "Properties" and only can be differentiated from properties they contain or location of the views on the screen. I realize that this is not an easy change and probably will require cooperation of property providers. Alternatively, it would be nice to provide a link button to go back to the originating element. 2) The number of property views grows fast. It would be nice to have "Close all", "Close others" menus for them. 3) It seems that property views do not update when the originating element gets updated (unless selction changed as a result of, say, file rename). This is not a big deal for one view tied to a selected element, but will become an issue for pinned views. At the very least, views could provide a "refresh" button. Better yet would have been updates occurring automatically. => Implementation <= - It seems that all changes are done as APIs. Is this desirable? - PropertyShowInContext - is it needed? - [Bug?] Using "Show In" twice on the same source opens two property views => Questions <= - The new property views do not inherit originating view's settings (advanced, categories) - is this the desired outcome? - "Show In" does not work for debug threads. Is this as expected?
(In reply to comment #41) > I really the idea of the multi-instances property views! Means "I really like the idea of the multi-instances property views!" :-). +100 for ability to edit one's comments in BugZilla :-).
(In reply to comment #41) > 2) The number of property views grows fast. It would be nice to have "Close > all", "Close others" menus for them. +1
(In reply to comment #41) > 1) It would be nice to be able to differentiate between multiple property > views. > As it is, they all are called "Properties" and only can be differentiated from > properties they contain or location of the views on the screen. > I realize that this is not an easy change and probably will require cooperation > of property providers. > Alternatively, it would be nice to provide a link button to go back to the > originating element. > > 2) The number of property views grows fast. It would be nice to have "Close > all", "Close others" menus for them. Both features sound reasonable to me, but I'd like to track them in separate bugs. > 3) It seems that property views do not update when the originating element gets > updated (unless selction changed as a result of, say, file rename). This is not > a big deal for one view tied to a selected element, but will become an issue > for pinned views. > At the very least, views could provide a "refresh" button. Better yet would > have been updates occurring automatically. See Bug #96838 > => Implementation <= > > - It seems that all changes are done as APIs. Is this desirable? Since the patch bumps up the bundle minor, (compatible) API changes are allowed. And it makes the code much more understandable/readable if done this way. > - PropertyShowInContext - is it needed? > - [Bug?] Using "Show In" twice on the same source opens two property views This might be an issue with active workbench parts. The property view's content depends on which workbench part is active at creation time. > => Questions <= > > - The new property views do not inherit originating view's settings (advanced, > categories) - is this the desired outcome? Depending how the view instance has been opened, a real parent to inherit settings from might not be available (except randomly picking one). > - "Show In" does not work for debug threads. Is this as expected? Do you have steps to reproduce?
The time is getting short to put new items into M5. Let's have a look at it during tomorrow's UI meeting? Tue, January 13, 11:00am – 11:20am 613-787-5018 / 1 866-842-3549 / 800-4444-7070 (global toll free) Conference id: 8864297# I'll try to get a laptop working with the screen sharing program: 1. Go to the URL - http://www.webdialogs.com 2. Click 'Join a Meeting' button in the top right corner of the page 3. Enter the Conference ID: "ESCREEN" 4. Enter your name and email address 5. Click the 'Log In' button
(In reply to comment #45) > The time is getting short to put new items into M5. Let's have a look at it > during tomorrow's UI meeting? I'll be there. Markus
Combined notes from Boris and myself from today's discussion: - There should be some way to identify the object whose properties are in the view. + View title to be formatted as "Foo Properties" - Properties view does not update on changes (see bug 96838) - Bug: opening new view does not show current selection's properties until selection changed - Bug: strange behavior for tabbed properties view and IPropertySheetPage example from Antony. Strange behavior on changing perspective. - Design points: + unpinning should close the view? + do not open new views automatically (why "show in" open pinned view?) - Kevin volunteered to review other implementations of the "pin" action to see if we can come up with a common pattern - Once we have the design we agree upon we'll try to fit the new functionality in the M5 and fix details in M6+. Proposed combined design (as I understand it - feel free to correct): The view will have a "new" ["capture"] button, no "pin" button. Pressing "new" button will open a new properties view pinned to the current selection. Those secondary pinned views will not be preserved on Eclipse restart and will not have a "new" button. The secondary views will have a "pin" icon decorating regular view icon. "Show in" will open the "primary" properties view if none is open, otherwise update the existing view.
(In reply to comment #47) > Proposed combined design (as I understand it - feel free to correct): > > The view will have a "new" ["capture"] button, no "pin" button. Pressing "new" > button will open a new properties view pinned to the current selection. Those > secondary pinned views will not be preserved on Eclipse restart and will not > have a "new" button. The secondary views will have a "pin" icon decorating > regular view icon. "Show in" will open the "primary" properties view if none is > open, otherwise update the existing view. This works for us and it also simplifies the implementation a lot. Do we make sure the pinned instance opens up on the same view stack like the master property view?
(In reply to comment #48) > (In reply to comment #47) > > Proposed combined design (as I understand it - feel free to correct): > > > > The view will have a "new" ["capture"] button, no "pin" button. Pressing "new" > > button will open a new properties view pinned to the current selection. Those > > secondary pinned views will not be preserved on Eclipse restart and will not > > have a "new" button. The secondary views will have a "pin" icon decorating > > regular view icon. "Show in" will open the "primary" properties view if none is > > open, otherwise update the existing view. I don't mean to be contrary, but I have a few concerns with this approach: 1) It's a new paradigm different from the 5 styles listed on the wiki page referenced above. 2) I'm not sure it's the right paradigm. The use case is that people want to preserve the values in the properties view, not "I should make a new one so the old one won't change". The new prop view is a byproduct of the first not changing. Think "freeze". 3) Similarly, I think its odd that prop view I'm in gets pinned as a result of creating a new one. I said "New", why did the one I was in change state as a result? Thus I'd rather see us stick with Pin, and have the behaviour be that next resource you click on opens the new one. This to me is similar to current pinning of say the History. For pinned History, the new one appears when I say "Show in History". It just happens that the way that the property view works is that you never need to use the menu item to trigger it, you're always saying "Show in Prop View" once it's open. This makes slightly more sense in the case of the user closing the unpinned one. As soon as they click on a resource, a new unpinned one will open, because trying to change a pinned one causes that as a byproduct. What happens if I drag a resource and drop it onto a pinned prop view? Again, it's about it not changing (the results are shown in the existing unpinned one or a new unpinned one if none exist). I'll admit that the new prop view opening "for free and later" feels a bit surprising, so I'm not ecstatic about the suggestion, but I think it's slightly better if only because its the closest to the existing paradigms. The rest of the proposal WRT persistance etc. I'm good with. In both my suggestion and the one above though we must handle the case of shutdown/restart if the single unpinned one had been closed just before shutdown. I'm assuming I'll get an unpinned one on startup but not sure how we detect the case and make it happen (presumably must do something special on shutdown). > Do we make sure the pinned instance opens up on the same view stack > like the master property view? +1
(In reply to comment #49) > I don't mean to be contrary, but I have a few concerns with this approach: > > 1) It's a new paradigm different from the 5 styles listed on the wiki page > referenced above. > > 2) I'm not sure it's the right paradigm. The use case is that people want to > preserve the values in the properties view, not "I should make a new one so the > old one won't change". The new prop view is a byproduct of the first not > changing. Think "freeze". > > 3) Similarly, I think its odd that prop view I'm in gets pinned as a result of > creating a new one. I said "New", why did the one I was in change state as a > result? The proposal was to open a new pinned one, as in: the original properties view continues to track the selection, and the newly opened one is frozen right away. What you argue against is the reverse, freezing the current one and opening a new one that tracks the selection. I agree that this would be odd.
(In reply to comment #50) > The proposal was to open a new pinned one, as in: the original properties view > continues to track the selection, and the newly opened one is frozen right > away. What you argue against is the reverse, freezing the current one and > opening a new one that tracks the selection. I agree that this would be odd. I missed that, I guess because I find it also odd :) So it's really "clone", right? But if the new view is the pinned one, it's the one that will have focus, yet it's now frozen. As soon as I click somewhere else, we'll have to bring the old one (that I was in when I hit [New]) to the front, covering over the one I just made, so it can continue to track selection. But the frozen one (which is likely now obscured) was the one I presumably wanted to keep looking at to compare values with. Part of my reluctance might just be the word "New", which suggests one like the you started with. But in this case the one is frozen in time, not like opening a new Property View at all. It's a shame, but I think what the user really wants to have happen is that the view area gets split in two, with the frozen prop view on one side and the live prop view on the other.
(In reply to comment #47) > The view will have a "new" ["capture"] button, no "pin" button. Pressing "new" > button will open a new properties view pinned to the current selection. Those > secondary pinned views will not be preserved on Eclipse restart and will not > have a "new" button. The secondary views will have a "pin" icon decorating > regular view icon. Thinking about it, no pin button on the primary makes use cases unnecessarily hard where one simply wants to pin the primary. Browse to a different resource. Execute a certain command on the resource based on the current property sheet's content.
(In reply to comment #52) > Thinking about it, no pin button on the primary makes use cases unnecessarily > hard where one simply wants to pin the primary. Browse to a different resource. > Execute a certain command on the resource based on the current property sheet's > content. Yes that's a good use case, agree. It's also in line with why I believe the intended action is "keep this from changing". I think pin, with an explicit action required to open a new non-pinned prop sheet, is enough at least initially and we see how it plays. That sequence actually matches the comment #52 use case.
Kevin, would you mind loading the patch to see if it's good enough to put into HEAD as is (from the usability perspective, not code/API perspective)? A positive answer would mean that we can start the IP process.
Tried the patch, a few comments: OK not bad but there's a few issues. (1) *Bug*: Menu "Show in Properties" seems to always create a new pinned prop view, even if no other prop view is open. This surprises me and is a different behaviour than it used to be and not what I think we want. It leads to a proliferation of pinned prop views which I think is a problem. Follow the lead for the History view. A) If there is no prop view open, open a new one (unpinned) B) If there is a prop view open, but not pinned, just refill it with the new content C) If there are only pinned prop views, open a new one (unpinned). What I believe the likely usage pattern will be is: a) I have something whose properties I want to keep checking b) I want to compare them against another resource's c) So I pin one and open a new one. d) When the comparison is complete, I'm likely to close the pinned one. I think it's unlikely that the user will more than two prop views open at once. (2) It's difficult to figure out which prop view is for which resource. We should follow the lead of History and other multi instance views and put identifying information on the LHS of the view toolbar. History does it always but maybe we should only do it for the pinned, with the idea that it becomes even easier to spot when the view is pinned (note that History needs to put it there always because it's otherwise impossible to figure out who the resource is. For us, the properties themselves tell you that but it requires some reading). (3) There's no notion of a pinned prop view being "the view" for a resource. That is, A) open a prop view (Q) on a resource X, pin the view B) open a second prop view (R) on another resource Y C) select resource X -> the prop view (R) will fill with the properties of X. So I now have two prop views showing me the same thing. If I want to get back to the pinned view (Q) for X, I need to dig through them. We should again follow the lead of the History view. It works as so: A) Show in History on X, Hist view Q opens B) pin it C) Show in History on Y, new Hist R view opens D) Show in History on X, existing Hist view Q comes to front. (4) I don't like the big honk'in "Open new Properties" button and question the need for it. If we really thought we needed ok we could try to find an icon but I think there are enough ways to open a new prop view (Ctrl-3, Show in Properties" that this isn't required. (5) Enhancement: there is no good way to get back to the resource whose property you're viewing. The <-_> button in the Navigator only works for editors. I don't think I've seen a multi instance view that handles this, but it's worth us thinking about. I think (3) compounds it because I keep generating new views so can't ever trigger the association of resource to pinned prop view in the forward direction either.
Markus, time is of the essence if we want to make the Galileo IP deadline (end of January). Will you be able to address Kevin's issues with a new patch? If not, what would you propose we do?
(In reply to comment #56) > Markus, time is of the essence if we want to make the Galileo IP deadline (end > of January). Will you be able to address Kevin's issues with a new patch? If > not, what would you propose we do? I'm not entirely sure I'll be able to address all of Kevin's comments by the end of the month. What about if you guys go ahead and create a CQ with the current patch (or the state of January 31st) and then we work from there with additional (significantly smaller) patches.
Sounds good. It would be great if you could give us one more patch on (or before) January 31st which we would submit to the Foundation. We can then take it from there.
(In reply to comment #58) > Sounds good. It would be great if you could give us one more patch on (or > before) January 31st which we would submit to the Foundation. We can then take > it from there. Agree, this is the best approach. I think the core is fine and the issues I brought up are refinements that can be handled on a small patch basis.
This will not make it into M5 but I am keeping the target milestone for now, as a reminder to submit the CQ next week.
Created attachment 124014 [details] version 7 (In reply to comment #55) > (1) *Bug*: Menu "Show in Properties" seems to always create a new pinned prop > view, even if no other prop view is open. This surprises me and is a different > behaviour than it used to be and not what I think we want. It leads to a > proliferation of pinned prop views which I think is a problem. > > Follow the lead for the History view. > > A) If there is no prop view open, open a new one (unpinned) > B) If there is a prop view open, but not pinned, just refill it with the new > content > C) If there are only pinned prop views, open a new one (unpinned). > > What I believe the likely usage pattern will be is: > a) I have something whose properties I want to keep checking > b) I want to compare them against another resource's > c) So I pin one and open a new one. > d) When the comparison is complete, I'm likely to close the pinned one. > I think it's unlikely that the user will more than two prop views open at once. I've updated the patch so that new instances aren't automatically pinned any longer of opened from show view. The parent is still pinned when the new button is used though. Btw. I found that one is easily struck by bug #257188 which causes e.g. package explorer to not to get focus when just right-clicked. > (2) It's difficult to figure out which prop view is for which resource. We > should follow the lead of History and other multi instance views and put > identifying information on the LHS of the view toolbar. History does it always > but maybe we should only do it for the pinned, with the idea that it becomes > even easier to spot when the view is pinned (note that History needs to put it > there always because it's otherwise impossible to figure out who the resource > is. For us, the properties themselves tell you that but it requires some > reading). Not sure what you mean by LHS here. But if you suggest to set the content description of the view, then we face the problem that PropertySheet only knows org.eclipse.jface.viewers.ISelection. So we either call toString() on ISelection or delegate somehow (addition to IPropertySheetPage or an adapter) to the corresponding IPropertySheetPage which knows how to make sense out of the ISelection. The new patch simply calls toString() which I left in for debugging. > (3) There's no notion of a pinned prop view being "the view" for a resource. > That is, > > A) open a prop view (Q) on a resource X, pin the view > B) open a second prop view (R) on another resource Y > C) select resource X -> the prop view (R) will fill with the properties of X. > So I now have two prop views showing me the same thing. If I want to get back > to the pinned view (Q) for X, I need to dig through them. > > We should again follow the lead of the History view. It works as so: > A) Show in History on X, Hist view Q opens > B) pin it > C) Show in History on Y, new Hist R view opens > D) Show in History on X, existing Hist view Q comes to front. I don't like the idea if selection (e.g. in package explorer) fiddles with my view stack of e.g. property views. And the scenario only makes sense if Q is not visible/buried under other property views. If it is visible but not the current active view, do you want it to become the new active view? > (4) I don't like the big honk'in "Open new Properties" button and question the > need for it. If we really thought we needed ok we could try to find an icon > but I think there are enough ways to open a new prop view (Ctrl-3, Show in > Properties" that this isn't required. http://wiki.eclipse.org/Platform_UI/Multi-instance_Properties_View#Constructing_new_instances_from_the_.27Properties.27_view_itself It simply saves me from a few clicks when comparing two (or more) elements. > (5) Enhancement: there is no good way to get back to the resource whose > property you're viewing. The <-_> button in the Navigator only works for > editors. I don't think I've seen a multi instance view that handles this, but > it's worth us thinking about. I think (3) compounds it because I keep > generating new views so can't ever trigger the association of resource to > pinned prop view in the forward direction either. I suggest we track this issue in separate enhancement requests?
Markus, regarding the "version 7" patch - did you author 100% of it, or were there other contributors?
CQ 3033 filed.
(In reply to comment #62) > Markus, regarding the "version 7" patch - did you author 100% of it, or were > there other contributors? Yes, I author 100% of the code in patch version 7.
version 7 released to HEAD. I made a few changes: - moved "Open new Properties view" from the view toolbar to the view menu - content description is only shown when the view is pinned - content description is now "Selection from <part title>" - the toString() on the selected object does not work, toString() returns a String commonly good for debugging purposes, but not for end users
Comment on attachment 124014 [details] version 7 added missing iplog+ flag
