Bugzilla – Full Text Bug Listing
| Summary: | E4 should not store reference to view and editor icon contribution value | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Markus Keller <markus.kell.r> | ||||
| Component: | UI | Assignee: | Platform-UI-Inbox <Platform-UI-Inbox> | ||||
| Status: | CLOSED WONTFIX | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | daniel_megert, dirk.fauth, harawata, Lars.Vogel, psuzzi, thatnitind, tom.schindl | ||||
| Version: | 4.5 | ||||||
| Target Milestone: | --- | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| See Also: |
https://git.eclipse.org/r/46935 https://bugs.eclipse.org/bugs/show_bug.cgi?id=517169 |
||||||
| Whiteboard: | stalebug | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 426025, 427950, 433714, 465778, 466840, 493931, 495847 | ||||||
| Attachments: |
|
||||||
|
Description
Markus Keller
It's not only editor icons. View icons are also affected (bug 426025 comment 78). Set a breakpoint in org.eclipse.e4.ui.model.application.ui.basic.impl.PartImpl#setIconURI(String) to see where things go wrong. Basically, it's E4Application#loadApplicationModel(..) The only correct source of information that has been contributed via an extension point is the (live) extension point registry. The whole idea of a dynamic, plug-in based system that tries to persist a complete runtime datastructure built from the plug-ins' contributions just can't work. Ever. And there are even more hideous implementation flaws in org.eclipse.ui.internal.menus.MenuHelper#getIconURI(..) / #rewriteDurableURL(String), where E4 model information is extracted and stored in ways that baffle all description. We could solve the icon problem by instead of putting a complete URI into the setIconURI a specialized one like extension://my.special.bundle/viewId/icon and then have a icon resolves understanding this information but I think this opens the next question - how do we if e.g. the class implementing the editor/view is changing i guess this is recorded in the XMI as well right? ... the other option is that when ever the extension registry is modified we sync all stored information in the model with the persisted information updateing: * contributionURI * iconURI Yes, you can always add another cache. And when users tell you that it doesn't work, then add another cache refresher. Etc. But the ultimate conclusion is that all these things are completely unnecessary, and just cause bloat and a maintenance nightmare. 3.8 still starts considerably faster than anything E4-based. There is a reason why some people create layered systems without layer-breakers. I don't understand the offensive style of this reply but i tried to be helpful - so the 1st solution is the preferred one? (In reply to Thomas Schindl from comment #5) > the 1st solution is the preferred one? Hi Tom. The preferred solution is to read the correct/current icons from the editors and views extension points. (In reply to Dani Megert from comment #6) > (In reply to Thomas Schindl from comment #5) > > the 1st solution is the preferred one? > > Hi Tom. The preferred solution is to read the correct/current icons from the > editors and views extension points. Correct me if I'm wrong, but we are talking about the compatibility layer, don't we? Because AFAIK extension points shouldn't be used in plain E4. So a solution has to be done in the compatibility layer that doesn't set the iconURI values to the E4 model, but sets them on load. At least that is my understanding. I'm currently looking into this but I'm not sure this is only compat problem but also if you create MParts from MPartDescriptor - if I take the point MPart always has to delegate to MPartDescriptor for iconURI, contributionURI, ... (In reply to Markus Keller from comment #1) > It's not only editor icons. View icons are also affected (bug 426025 comment > 78). > My tests show that this is not true because the compat-layer pushes the correct icon from the extension registry into the transientData-Map of the MPart with the key IconUriForPart which makes the renderer pick a different icon! > Set a breakpoint in > org.eclipse.e4.ui.model.application.ui.basic.impl. > PartImpl#setIconURI(String) to see where things go wrong. Basically, it's > E4Application#loadApplicationModel(..) > Correct but the icon stored there is not used because the compat-layer pushes an entry to transient data map and so the renderer uses this URI (see SWTPartRenderer#getIconURI) - i have no freaking idea why the compat layer does it this way and not simply replaces the iconURI Created attachment 252954 [details]
Sample projects
Attached a sample project with editor and view contribution. Markus I'm unable to reproduce any rendering problem based on this sample.
My test was the following:
- launch with default icons
- Open Sample View
- Open Sample XML Editor
- Shutdown
- Replace icons in editor & view contribution
- Launch again
=> All icons are correct
As already pointed out in my other comment I have no freaking idea why the compat-layer is not simply setting the iconURI to the appropriate value when it processes the extension registry but as to my understanding the iconURI is NEVER consulted when running on the compat-layer but you always hit the value stored in transientData-Map found in the MPart
(In reply to Thomas Schindl from comment #10) > Created attachment 252954 [details] > Sample projects > > Attached a sample project with editor and view contribution. Markus I'm > unable to reproduce any rendering problem based on this sample. > > My test was the following: > - launch with default icons > - Open Sample View > - Open Sample XML Editor > - Shutdown > - Replace icons in editor & view contribution > - Launch again > > => All icons are correct Tom, the test case is wrong. When the part is created it uses the correct icon. The problem only appears for parts that aren't created yet, e.g. minimize parts or hidden editors. (In reply to Thomas Schindl from comment #10) The icons are correctly set when a view/editor part is created or when a view is in a visible view stack (even if not on top). The problematic scenarios are: - view in minimized view stack - editor that is not on top of the editor stack To reproduce, you have to: - minimize the Sample View - open another editor after opening the Sample XML Editor ok thanks - I can now reproduce! My proposal would be that the compat-layer is not pushing the concrete icon URL into the MPart/MPartDescriptor but something like "compatreg:/view/my.view.id" and "compatreg:/editor/my.editor.id" New Gerrit change created: https://git.eclipse.org/r/46935 I pushed an implementation of my proposal to gerrit which does the following: a) it allows ResourceUtilities to understand other url-schemes b) the compat-layer does not directly the final icon uri but for views: compat-icon-source:/view/my.view.Id for editors: compat-icon-source:/editor/my.editor.Id c) the workbench publishes an OSGi-Service who is able to handle the compat- icon-source translating it back to a resolveable URI using exactly the same code path that has been used in the current code The biggest draw back of this solution is that once you opened a workspace with the new urls you can NOT open that with an older version! I know we generally defined that this is not supported but it's important to understand that the NEW URI-Scheme would be a NEW public API contract we need to support in future! (In reply to Thomas Schindl from comment #16) > I pushed an implementation of my proposal to gerrit which does the following: > a) it allows ResourceUtilities to understand other url-schemes > b) the compat-layer does not directly the final icon uri but > for views: compat-icon-source:/view/my.view.Id > for editors: compat-icon-source:/editor/my.editor.Id > c) the workbench publishes an OSGi-Service who is able to handle the compat- > icon-source translating it back to a resolveable URI using exactly the > same > code path that has been used in the current code > > The biggest draw back of this solution is that once you opened a workspace > with the new urls you can NOT open that with an older version! I know we > generally defined that this is not supported but it's important to > understand that the NEW URI-Scheme would be a NEW public API contract we > need to support in future! I'm not a fan of introducing such a limitation since that scenario is more likely than icon changes in the extension points. Definitely not something I would touch for 4.5. What if we push the uri support in 4.5 so that we could go back and forth between 4.5 and 4.6 but don't do the url rewrite? (In reply to Thomas Schindl from comment #18) > What if we push the uri support in 4.5 so that we could go back and forth > between 4.5 and 4.6 but don't do the url rewrite? -1. The new URI just continues the problem of redundantly stored information. The problem is not the format of the iconURI, but that the iconURI is stored/restored at all. The solution should be not to store the iconURI separately. The editor/view id already has to be stored somewhere, and that single stored id should be used to compute all information that is provided by the extension point contribution. Another option is that the compat layer leaves the uri to null and we have a custom renderer in the compat layer who uses the view/editor registry in case iconuri is NULL Not sure if it would be possible, but what about changing the model so it is possible to specify a callback to retrieve the value instead the value itself? -1 for me on that a custom uri or a custom renderer - a custom renderer could even fix the backward problem we eg see with jdt - i never understood why so much compat stuff eg in den menu, toolbar has been pushed into the e4 layer already instead of the compat layer simply shipping itsown custom version of the renderer (In reply to Thomas Schindl from comment #22) > -1 for me on that a custom uri or a custom renderer - a custom renderer > could even fix the backward problem we eg see with jdt - i never understood > why so much compat stuff eg in den menu, toolbar has been pushed into the e4 > layer already instead of the compat layer simply shipping itsown custom > version of the renderer -1 on the callback idea? Since the menu renderer seems to have some issues in compat mode and it is quite painful to get e3 and e4 working correctly, I think separating both to have special renderers for both sounds like an interesting idea. Mass move to M6 This is a timebomb that shouldn't be left ticking without a target milestone. Note for adopters of new png icons: Bundles need to keep the *.gif icons of values contributed in these extension points: - org.eclipse.ui.editors > editor > icon (typically in /icons/full/obj16/) - org.eclipse.ui.views > view > icon (complete /icons/full/eview16/ folder) Projects that switch to .png graphics need to keep the .gif versions for such icon contributions. Please mention bug 465456 in the comment of the commit that removes most .gif files but leaves a few around. Mass move. Please move to a concrete milestone if you plan to work on this item. Moving target milestone to 4.9 for all bugs that are major or above. This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie. |