Bugzilla – Full Text Bug Listing
| Summary: | [ViewMgmt] Placeholder is lost when a multi-view instanced view is opened | ||
|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Matthew Hatem <Matthew_Hatem> |
| Component: | UI | Assignee: | Matthew Hatem <Matthew_Hatem> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | blocker | ||
| Priority: | P1 | CC: | n.a.edgar |
| Version: | 3.0 | ||
| Target Milestone: | 3.0 M9 | ||
| Hardware: | PC | ||
| OS: | Windows 2000 | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Matthew Hatem
Matt is working on a fix (not a workaround). There are a few ways to solve this: 1) Update the current implementation such that place holders for views that allow multiple instances are never replaced. 2) Introduce new API to allow for adding place holders with secondary ids. A place holder with a secondary id of '*' would never be replaced. addPlaceholder(String primaryId, String secondaryId); 3) Update the current implementation to support place holders with secondary ids without introducing new API: a) Adding a place holder with an explicit secondary id is done by using a '/' to separate primary from secondary ids layout.addPlaceholder (org.eclipse.ui.browser.view/org.eclipse.ui.browser.instance.intro); b) Adding a place holder for multi-instanced views with unknown secondary ids is done by using a '*' as the secondary id. layout.addPlaceholder(org.eclipse.ui.browser.view/*); After discussing this with Nick we have decided that 3 is the best option. If someone disagrees please update this bugzilla report with your proposal. -m@ Yes, I favour (3). A few extra points: - I think if we're going to support wildcards, then we should support it for primary ids and partial matches as well. E.g. "*", "org.eclipse.ui.docView*". - If a placeholder has a wildcard, then it should not be replaced when the view is opened since another matching view may come along later (whether or not it's a multi-instance view). - If the rule is "keep the placeholder if a matching view can come along later" then perhaps we should do (1) too, even if there's no wildcard. - Still need to define the behaviour for multi-instance views if there's no placeholder, but there is a pre-existing view. Should it be placed in the same spot or in the default location at bottom right? If the former, then we may not need to do (1) as well (as long as the placeholder is replaced when the view is closed). However, see next point. Having it appear in the default location is not necessarily a bad thing, and may be better than having it appear over the prior instance for the cases where the view is being cloned to show different aspects of the same model that the user wants to see simultaneously. - Currently when views are closed, they are replaced with a placeholder. Does the replacement placeholder for multi-instance views include the secondary id? If so, then the next instance may not match, and would end up in the default location. Also, there's no need to leave a placeholder if there's already a matching wildcard placeholder in the same folder (the view may have been moved to a different folder though). - Suggest using a different delimiter than '/', e.g. ':'. If we ever support nested parts, which is not unlikely, we'll want to use '/' for referring to them. Of course, it wouldn't make sense to add a placeholder for a nested part at the top level, so maybe there's no ambiguity after all. But I think ':' would be clearer. Should use this internally too. Based on the above, it seems that the semantics are made more complicated by trying to match multi instances to a placeholder with just the primary id, especially when it comes to replacing the placeholder. Note: that we can't just always leave the placeholder, since views can be moved between folders and we want a replacement placeholder to be left where the view last was before it was closed. How about: - a view reference is either <primaryId> or <primaryId>:<secondaryId> - placeholders are expressed as view references, with wildcards allowed in either part, and allowing partial matching - regular string pattern matching is used to match a view ref to a placeholder - so this means that a multi-instance view with non-null secondary id does not match a placeholder for just the primary id - if there's no placeholder, or just a placeholder for the primary id, then views with a secondary id will go in the default location - implementation note: can use o.e.ui.internal.misc.StringMatcher to do the matching - non-wildcard placeholders must be considered first - e.g. "org.someorg.SomeView" is matched before "*" - more-specific (longer) wildcard matches must be considered before less-specific (shorter) ones - e.g. "org.someorg.SomeView:*" is matched before "*" - when a view is shown, the matching placeholder (if any) is replaced if and only if it has no wildcards - when a view is closed, a placeholder for its view ref is left (with no wildcards) if and only if there is no matching placeholder in the same folder (this condition is just an optimization, but could be significant for the doc view case, particulary since placeholders are persisted across sessions) > But I think ':' would be clearer. Should use this internally too.
Does this mean you will be changing the existing multi-instance view API, ie.
ViewReference and showView?
The / does not show up anywhere currently in API, so I wasn't planning on making any changes here. Created attachment 9271 [details]
Just a preview of pattern matching placeholder support
This is a preview of the pattern matching placeholder support. This is only a
preview.
I am aware of the bugs. If you find any I'm not aware of please let me know
<g>.
Test case to follow.
Created attachment 9272 [details]
test case - patch for RCP Browser example
Under the new "show" menu you will see a list of test actions. See the output
in the console and see that the ids printed match the pattern described in the
() in the associated menu item.
All placeholders are to the left of the fixed browser view.
Placeholders for views with secondary ids are lost after the view is closed.
Views that match placeholders with a '*' do not have a working sash. Must be
related to the use of stack()
I know I need a better test suite, if you have suggestions please let me know.
-m@
Created attachment 9339 [details]
updated preview patch, fixes all known bugs
This is an updated preview. It address all of the known bugs in the previous
patch.
Created attachment 9340 [details]
updated preview patch, removed progressmanager junk
Sorry there was some progressmanager junk in preview patch.
Created attachment 9355 [details]
release quality patch, includes fix for multi view restore state bug
This is a release quality patch to support pattern matching placeholders. This
patch also addresses a bug with saving and restoring state of a view with a
secondary id.
There is one FIXME in this patch. I was hoping a Nick could provide some
insight into how it should be fixed. I'm not quite sure if and how the
Relationship info can be read from the place holder. See new method
addChildForPlaceholder.
Matt, I've released the patch but have the following questions/comments:
- hasPlaceholder calls findPart(secondaryId)
- changed to call findPart(primaryId, secondaryId)
- Can simplify:
private static final String WILD_CARD= new String(new char[] {'*'});
->
private static final String WILD_CARD = "*";
- Can we simplify it by just having findPart(primaryId, secondaryId), with
findPart(primaryId) simply calling findPart(primaryId, null). This would reduce
duplicate code.
- findPart should only do pattern matching for placeholders, not editors and views
- what is addChildForPlaceholder supposed to do? It seems to be splitting the
area -- why?
- would be good to have some tests for placeholders in folders (or in
placeholder folders)
> hasPlaceholder calls findPart(secondaryId)changed to call findPart(primaryId, secondaryId) Strange. Thanks for catching that. > Can we simplify it by just having findPart(primaryId, secondaryId), with findPart(primaryId) simply calling findPart(primaryId, null). Sure, I will do this. > findPart should only do pattern matching for placeholders, not editors and views Okay, I believe you are talking about findPart(String id, LayoutPart[] parts). I'll fix that too. > what is addChildForPlaceholder supposed to do? It seems to be splitting the area -- why? Couldn't figure out how to calculate the relationship info so I just hardcoded the ratio to .5 I just realized how to do this so I will provide another patch for this. > would be good to have some tests for placeholders in folders (or in placeholder folders) Yes, I will do this too. Created attachment 9446 [details]
Updated patch. Fixes bugs and addresses questions/comments
This patch addresses Nicks questions and concerns. It also fixes some other
unknown bugs as well as some "fixed view" related bugs.
I've committed the changes with a few minor cleanups: - defined ID_SEP (= ":") and getKey methods in ViewFactory, and changed all places where ':' was used to use these - defined LayoutPart.getPlaceHolderId(), with override in ViewPane, and changed the placeholder creation when a view is closed to use this - moved WILD_CARD to PartPlaceholder, added PartPlaceholder.hasWildcard(), and fixed up refs accordingly Looking at the code for findPart(String, LayoutPart[]) and findPart(String, String, LayoutPart[]), it looks like it's tracking currentMatchingPart and wildCard only for the current level, rather than maintaining this as it recurses. This would seem to imply that if you were looking for had "*" at a deeper level and "myId" at the top level, then it would match "*" before "myId". Or are we getting lucky because the nesting depth is effectively 1? Created attachment 9713 [details]
Fixes recursive issues Nick mentioned
Nick, this is a much cleaner solution. Please consider committing it.
Thanks
The patch looks good. I made a few minor changes to the MatchingPart nested class, to avoid creating garbage during the compare (which can be O(N^2)), and to not include null ids in the length. Closing. |