Community
Participate
Working Groups
Build Identifier: 20100218-1602 This is a feature request for the implementation of a Minimap feature, as can be found in the "Sublime" text editor and in a few (non-Eclipse) IDE plugins. A good explanation, as well as a screenshot can be found here: http://blog.quplo.com/2010/05/5-innovations-in-text-editing-and-ides/ Copy-paste of the description, should the website go down: "The Sublime text editor puts a column to the left of your text editing environment which displays a Google Maps-like minimap. For editing huge code files, this is a great way to get an overview of the buildup of your code, especially if you need to jump quickly to your class properties, definitions, or just want to get a feel for the structure of your file. We found that the minimap works best when you’re trying to discipline yourself to keep your files small: large classes in C#, tons of style rules in CSS, and deeply nested HTML become really obvious when the minimap is right there staring at you the whole time." I think this would be a very powerful addition to Eclipse. Reproducible: Always
Created attachment 175633 [details] Screenshot displaying the feature (as implemented in Sublime)
Platform provides the Outline view for this purpose. What displayed there, however depends on the editor you are using. Moving to platform-text for comments
The question is whether this should be limited to text files or whether it might also be useful in general, e.g. for graphics.
(In reply to comment #3) > The question is whether this should be limited to text files or whether it > might also be useful in general, e.g. for graphics. If I remember correctly, GEF does supports this kind of functionality in the outline view already.
The problem with the Outline view is that there's only one per editor, unless the specific editor decides to add tabs inside its own outline page. The question is whether every editor (type) should invent something similar or whether the mini-map (or bird's eye) view comes with the Platform, like the Outline does.
Moving to Platform UI for discussion whether they want to provide this in a generic way. If this is not desired then the bug can be moved back to Text.
This could be viewed as a layout issue I think. If we were to put the Outline view *directly* into the same composite as the one hosting the editor it would then appear the same as the 'mini-map' no ?
Created attachment 176835 [details] Mockup of the Outline view 'embedded' in an editor
>This could be viewed as a layout issue I think Yep, but maybe I'd then like to use that space to put my Java Outline. So, the first questions to answer is: does the mini-map relate to the outline?
To me it appears that they're the same concept, must arranged differently. Perhaps just dragging the Outline view to the left of the editor area would help?
>Outline view to the left of the editor area would help? Mmh, no. It would also need the concept of having multiple pages in the Outline for the same editor part (assuming a mini-map would be general concept in the Platform).
Eric, Remy, I am moving this bug under [EditorMgmt], but if it belongs more to another area such as [DetachedView], please update accordingly. Thanks.
Any chance to get this implemented?
(In reply to Diogo Santana from comment #13) > Any chance to get this implemented? Only if someone contributes a high quality patch.
There is a good plugin providing that on marketplace: https://marketplace.eclipse.org/content/overview-plugin-eclipse @Sandip: would you be interested in contributing it to Eclipse Platform? Feel free to ask for any guidance!
Hi Mickael Istria, I would love to contribute the plugin. In fact I had a discussion about it with Lars Vogella about this at EclipseCon 2014 in California. I would love to get guidance on all the things needed for a successful contribution.
The source code is available here: https://github.com/sandipchitale/sandipchitaleseclipseplugins/tree/master/text.overview
Please see the screen shot in the marketplace entry. My implementation uses a static, fixed width Iviewpart that is by default laid out to the right of editor area. It is shared by and works with all abstracttexteditor sub classes in a generic way.
(In reply to Sandip Chitale from comment #16) > Hi Mickael Istria, I would love to contribute the plugin. Great! > I would love to get guidance on all the things needed for a successful > contribution. As a 1st iteration, your contribution could take the form of a new bundle inside eclipse.platform.ui: 1. Clone eclipse.platform.ui repository from Gerrit https://git.eclipse.org/r/#/admin/projects/platform/eclipse.platform.ui 2. Add there your plugin, rename the bundle and its packages and other ids (extensions) to something like org.eclipse.ui.text.minimap 3. Add the necessary copyright headers (see other .java files in this repository for examples) 4. Add a line in the bundles/pom.xml to get your bundle built together with platform-ui 5. Git add everything 6. Push to Gerrit: https://wiki.eclipse.org/Gerrit Once your code is pushed to Gerrit, regular Platform UI committers will be able to have a look at it and provide more specific suggestions, leading to new iterations, until the submission is ready to be merged. You can find some contributors to help you with that more directly on the #eclipse-dev channel on IRC.
(In reply to Mickael Istria from comment #19) > (In reply to Sandip Chitale from comment #16) > > Hi Mickael Istria, I would love to contribute the plugin. > > Great! > > > I would love to get guidance on all the things needed for a successful > > contribution. > > As a 1st iteration, your contribution could take the form of a new bundle > inside eclipse.platform.ui: Yes, if the mini map is not for textual editors only. Otherwise the correct place is platform.text. Also, it depends whether this plug-in would work (i.e. provide the mini map) for for all textual editors or whether it has extensions that are expected to be implemented by clients. I did not look at the code, but a better solution might be to incorporate it into one of the existing text plug-ins.
It is for text editors only and it does not expect any extension. I will start with platform.text and see how it goes.
(In reply to Sandip Chitale from comment #21) > It is for text editors only and it does not expect any extension. > > I will start with platform.text and see how it goes. If it's for AbstractTextEditor then the 'org.eclipse.ui.workbench.texteditor' would be where I'd start.
Actually it works at the org.eclipse.swt.custom.StyledText level. If interested see the implementation of the view here: https://github.com/sandipchitale/sandipchitaleseclipseplugins/blob/master/text.overview/src/text/overview/views/OverviewView.java and has dependencies on: Require-Bundle: org.eclipse.ui, org.eclipse.core.runtime, org.eclipse.jface.text based on that I guess it belongs in platform.text? (Pardon my ingnorance of this).
(In reply to Sandip Chitale from comment #23) > Actually it works at the org.eclipse.swt.custom.StyledText level. If > interested see the implementation of the view here: > > https://github.com/sandipchitale/sandipchitaleseclipseplugins/blob/master/text.overview/src/text/overview/views/OverviewView.java > > > and has dependencies on: > > Require-Bundle: org.eclipse.ui, > org.eclipse.core.runtime, > org.eclipse.jface.text > > based on that I guess it belongs in platform.text? Yes, and there most likely into the 'org.eclipse.ui.workbench.texteditor' plug-in. > (Pardon my ingnorance of this). np. Before starting to contribute you will have to sign the CLA. See https://wiki.eclipse.org/CLA for details.
Thanks. I will sign the CLA. Are there instructions in one place somewhere on how to set up the development environment. I did clone the git repo for platform.text and imported the projects. However, then started getting parent pom errors, so had to clone some other repos etc. Also getting a lot of errors - m2e complaining about lifecycle mapping and so on. Any help this is welcome. I use to develop with Eclipse code base before but have not done so lately.
(In reply to Sandip Chitale from comment #25) > Thanks. > > I will sign the CLA. > > Are there instructions in one place somewhere on how to set up the > development environment. See https://wiki.eclipse.org/JDT_UI/How_to_Contribute which explains it for JDT UI, but it is similar for Platform Text. Cloning Platform Text into a new workspace and importing the plug-in mentioned before should be the only thing you need fetch. > Also getting a lot of errors - m2e complaining about lifecycle mapping and > so on. We don't use m2e, so, can't help you with this.
(In reply to Sandip Chitale from comment #25) > Also getting a lot of errors - m2e complaining about lifecycle mapping and > so on. You may be missing the m2e/tycho connector: Got to Preferences > Maven > Discovery, click Open Catalog and install the Tycho connector. However, as Dani said, you can also ignore m2e if you plan to add your code to an existing plugin.
For some reason I am not finding the Tycho connector via discovery. Especially for additional goals that Eclipse build seem to be using. Yes...if I add my view into an existing Eclipse plugin(s) then I just need to be able to build those locally. I will try to follow the link Dani suggested with an example of JDT UI. Presumably it maps similarly for platform.text as well.
Hi Sandip, any progress on that topic? Is there anything we can do to help you?
Hi Mickael, I have made most of the changes. If interested you can see them in my temp Github repo here: https://github.com/sandipchitale/eclipse.platform.text I had a family emergency and hence the delay in setting up the Gerit review. I will get to it by this weekend.
(In reply to Sandip Chitale from comment #30) > Hi Mickael, I have made most of the changes. If interested you can see them > in my temp Github repo here: > > https://github.com/sandipchitale/eclipse.platform.text Nice. Could you turn those into a Gerrit patch? This is the usual process to contribute to Eclipse Platform: https://wiki.eclipse.org/Platform_UI/How_to_Contribute
Hi Sandip, What's the status of your contribution? Are you planning to submit the Gerrit patch soon? Do you need assistance with that?
Hi Sandip, Do you think, there is any chance to have some contribution? Minmap feature is a feature which misses inside Eclipse IDE although other IDE provides already that. Hope you will find time to contribute to Eclipse IDE. Regard's Angelo
Hi guys, After studying the code of https://github.com/sandipchitale/sandipchitaleseclipseplugins/blob/master/text.overview/src/text/overview/views/OverviewView.java I have noticed that a new StyledText is used to render the MiniMap. IMHO I think it's a bad idea to do that because for large file I think we can have trouble. An another MiniMap plugin have this problem https://github.com/apauzies/eclipse-minimap-view/issues/4 The reason about this performance problem is because of TextLayout using. StyledText maintains a pool of TextLayout for several lines and dispose it if it needs to render other part of lines. In a editor, there is no problem because we don't render the full lines, but in MiniMap context, we need to render the all lines. I think to have a very good performance, we need to use one TextLayout which draw the all lines. To do that we need to create a FastStyledText which extends Canvas and use the content, styles of Styledtext of the editor. The idea is to render FastStyledText with the content/styles of StyledText by looping each lines like StyledText#handlePaint does by using StyledTextRenderer. I have started a POC with that, if you are interested (and once it will work), I could create a gerrit patch if you wish. @Mickael what do you think about that?
(In reply to Angelo ZERR from comment #34) > I have started a POC with that, if you are interested (and once it will > work), I could create a gerrit patch if you wish. > @Mickael what do you think about that? We'd welcome a good patch for it.
I tell me if this MinimapView should be hosted more in Platform UI (org.eclipse.ui.views) like PropertySheet, no?
(In reply to Angelo ZERR from comment #36) > I tell me if this MinimapView should be hosted more in Platform UI > (org.eclipse.ui.views) like PropertySheet, no? If the view is specific to text, it'd be better to try to put it in eclipse.platform.text.
> If the view is specific to text, it'd be better to try to put it in eclipse.platform.text. I agree with you, but imagine that you wish in the future extends the Minimap (to render graphics editor for instance) view like PropertySheet does? In this case Platform UI is better, no?
(In reply to Angelo ZERR from comment #38) > > If the view is specific to text, it'd be better to try to put it in eclipse.platform.text. > > I agree with you, but imagine that you wish in the future extends the > Minimap (to render graphics editor for instance) view like PropertySheet > does? In this case Platform UI is better, no? One step after the other. We'll discuss that later, and IIRC, GEF or GMF alrady provides a similar feature.
(In reply to Mickael Istria from comment #39) > (In reply to Angelo ZERR from comment #38) > > > If the view is specific to text, it'd be better to try to put it in eclipse.platform.text. > > > > I agree with you, but imagine that you wish in the future extends the > > Minimap (to render graphics editor for instance) view like PropertySheet > > does? In this case Platform UI is better, no? > > One step after the other. We'll discuss that later, and IIRC, GEF or GMF > alrady provides a similar feature. For me it depends what your Gerrit change provides. If it provides a generic framework that does not depend on Text, then that part should be in Platform UI and maybe an extension for textual editors in Platform Text. But if your change only targets textual editors then I agree with Mickael.
> But if your change only targets textual editors then I agree with Mickael. Ok let's go for Platform Text. My idea is to host minimap code in the org.eclipse.ui.internal.views.minimap package. Are you OK? After studying several existing Eclipse Minmap project and experimenting several solution to manage Minimap. The all Minimap that I have seen display the editor content when editor is opened, but doesn't manage the case when editor content changed. Here my technical conclusion: 1) uses Styledtext instead of Canvas I though that using Canvas and redraw lines with one TextLayout was fast, but it's not really true. This idea comes from https://github.com/jeeeyul/pde-tools/blob/master/net.jeeeyul.pdetools/src/net/jeeeyul/pdetools/crazyoutline/CrazyCanvas.java I have tested this plugin but with long file, the performance are bad because CrazyCanvas redraw the full content of StyledText and doesn't take care of paint event (y and height) like StyledText does. My test was done by editing big file StyledText.java. The performance with this big file is very good with https://github.com/sandipchitale/sandipchitaleseclipseplugins/blob/master/text.overview/src/text/overview/views/OverviewView.java 2) Use PageBookView instead of ViewPart The MinimapView must use PageBookView like Outline does. the OverviewView plugin uses ViewPart and there is one StyledText instance used for the whole editor. With PageBookView, we can have an instenace of StyledText per editor. So with that, the StyledText from PageBookView (for Minimap) can be synchronized easily with StyledText from the editor (I mean for text and styles). OverviewView plugin is not able to refresh the Minimap when text changed (The Minimap becomes blank as soon as you type text content, but perhaps I miss something?) Having a Styledtext instance per editor is easy to synchronize (just with a simple document changed listener for instance) 3) Styles synchronization At this step we have text in the Minimap with scale. But we need to colorize it and takes care of styles changed. It seems that it's not possible to track the styles changed (when StyledText#setStyles for instance is called). IMHO I think StyledText should provide a new event StyledTextStylesEvent when styles changed. @Dani, @Mickael whet do you think about that? For the moment I have implemented this feature by using presentation listener and it works good with Java Editor, XML Editor which uses presentation listener. Perhaps in the first step we can support only those kinds of editor? Now I would like to speak about my contribution. My idea is to create several gerrit patchs to facilitate the review: * initialize MinimapView which is filled with only text. The minimap will able to be refreshed when text changed. * colorize the minimap with styles by using presentation listener. * draw the selection backround area in the minimap of the visibles lines of text viewer + manage scroll of this area (to synchronize it with editor scroll). Are you OK with that? If yes, must I create a bugzilla per issue, or must I use just this bugzilla issue?
New Gerrit change created: https://git.eclipse.org/r/123936
Created attachment 274323 [details] Minimap (gerrit patch) demo @Mickael suggested me to create a big gerrit patch with my whole work with Minimap. Please review my gerrit patch https://git.eclipse.org/r/123936 This gerrit patch display scaled Styledtext with styles and draw view port in the minimap. It support multi part editor and even styles wich defines font (like Markdown). Please see attached demo. Ant feedback are welcome, thanks!
I think the default font in the preview is too small. I cannot identify anything. IMHO Minimap should react to the font setting of the user for the text font size. If I increase/ decrease the font size with Ctrl+ or Ctrl- I expect minimap to react to this setting. I suggest to add a preference for the font size in minimap, I think it should be defined in relation to the text font. Background is gray for the unfocused view on Linux which makes it even harder for me to see something. Screenshot to follow.
Created attachment 274331 [details] Screenshot - Minimap on Linux
(In reply to Lars Vogel from comment #44) > I think the default font in the preview is too small. I cannot identify > anything. That's how most minimaps work ;) It's not meant to read code. > IMHO Minimap should react to the font setting of the user for the text font > size. If I increase/ decrease the font size with Ctrl+ or Ctrl- I expect > minimap to react to this setting. I disagree with that as I believe the goal of the minimap is not to be readable but more to give a visual hint of the position in the file and allow some recognition using the overall shape of the code blocks. It's not intended to read code. IMO the minimap should take the biggest size that prevents it from requiring a scroll. Minimap requiring scroll would be unusable. Minimap allowing user configuration would be useless disturbance and doesn't fit in the goal of the minimap.
@Angelo: thanks for the patch. How does it integrate with Word-wrap and code minings?
(In reply to Mickael Istria from comment #46) > (In reply to Lars Vogel from comment #44) > > I think the default font in the preview is too small. I cannot identify > > anything. > > That's how most minimaps work ;) It's not meant to read code. I'm using minimaps in other editors like Sublime and Code and at least I'm able to identify some structure. The current minimap implementation does not give me any information, as it is too small and uses a wrong background color so that it does not provde any information to me. And I'm a big fan of minimaps in other editors.
(In reply to Lars Vogel from comment #48) > I'm using minimaps in other editors like Sublime and Code and at least I'm > able to identify some structure. Just tested: Code adjusts the minimap font size based on the text font, at least it seems to jump to a larger / small font at certain text size points.
(In reply to Lars Vogel from comment #49) > Just tested: Code adjusts the minimap font size based on the text font, at > least it seems to jump to a larger / small font at certain text size points. Do you see your whole document all the time?
> @Angelo: thanks for the patch. You are welcome! > How does it integrate with Word-wrap and code minings? Word wrap should work but code minings is bugged (it takes some place but without draw of mining, I think we should ignore GlyphMetrics created by minings, but perhaps we could do that in an another gerrit patch?) > @Mickael, @Lars I'm planned to do a new gerrit patch to improve the draw of highlighted viewport. My main goal is to provide a first patch for a minimap which works well. After that we could improve it like with @Lars suggestion (size of font, etc). So please wait my gnew changed of gerrit patch and review it to merge it to master if you like it. Thanks!
Created attachment 274364 [details] Deom with minimap and dark theme @Lars, please retry my new gerrit patch https://git.eclipse.org/r/123936 This gerrit patch improves a lot performance and draw of client area in the minimap. Now I don't use gray color but selection background color with antialias (like block selection). I have fixed too bug when you scroll the client area in teh minimap to avoid drawing selection.
As we're likely to see a big popularity with code minings in a near future, I think it wouldn't be wise to ship a minimap until it works fine with code minings.
(In reply to Mickael Istria from comment #53) > As we're likely to see a big popularity with code minings in a near future, > I think it wouldn't be wise to ship a minimap until it works fine with code > minings. For the first minimal viable solution, I think that code mininings should get ignored in the minimap.
> As we're likely to see a big popularity with code minings in a near future, I think it wouldn't be wise to ship a minimap until it works fine with code minings. Yes sure. IMHO I think code mining draw should be ignored in minimap (for performance reason and more I think it's very usable as it is scaled). For that, we need to ignore GlyphMetrics created by code mining. One idea to do that is to mark StyleRange by using StyleRange.data = "codemining" and minimap could ignore the GlyphMetrics of the the style. It's just an idea... > For the first minimal viable solution, I think that code mininings should get ignored in the minimap. Yes please! Please create a new issue for that.
Guys please review my new gerrit patch https://git.eclipse.org/r/123936. Except for codemining case, this gerrit patch provide a "mature" minimap in a view. This gerrit patch should fixes some bugs and this https://bugs.eclipse.org/bugs/show_bug.cgi?id=535735 too
I think Wim questioned the "Minimap" name. If I search the Internet "Minimap" seems to be the correct term for this feature. Alternatively "Code preview" comes to mind but if minimap is established we should use that. @Angelo WDYT?
All Editor text like VSCode, Atom, Sublime etc uses this "minimap" name, so IMHO I think we should keep "Minimap."
(In reply to Angelo ZERR from comment #58) > All Editor text like VSCode, Atom, Sublime etc uses this "minimap" name, so > IMHO I think we should keep "Minimap." I agree. Sorry for the noise.
Please review my last gerrit patch https://git.eclipse.org/r/123936 which ignores GlyphMetrics which id generally created by code mining.
@Lars, @Karstem, @Mickael, I would like just to know if you think you will have time to review my minimap gerrit patch. My fear is that you are very busy and review will take long time. I would like to have more people feedbacks and I tell me more and more if I must create a github project with my minimap work. I don't want to have disturb with you and sorry with my impatience.
I will review tomorrow, sorry for my delay
> I will review tomorrow Very cool! > sorry for my delay No pb Lars, I wanted just to know the review status.
@Lars I have try to fix all comments that you have told me. I have just one question about version @since. I'm using: ---------------------- @since 3.12 ---------------------- But not sure for that, because MANIFEST.MF uses "3.11.100.qualifier". Please tell me which version I must use. Thanks!
(In reply to Angelo ZERR from comment #64) > @Lars I have try to fix all comments that you have told me. I have just one > question about version @since. I'm using: > > ---------------------- > @since 3.12 > ---------------------- > > But not sure for that, because MANIFEST.MF uses "3.11.100.qualifier". Please > tell me which version I must use. Thanks! IIRC all is internal API? In this case no since tags are necessary
> IIRC all is internal API? In this case no since tags are necessary Thanks @Lars for your clarification. I have removed this since version in my last gerrit patch.
Gerrit change https://git.eclipse.org/r/123936 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=883e4bcb4c17378f820408e38311de62afbdf4a9
Thanks a lot Angelo. Can you please add an entry to our N&N for 4.9? We also need a better icon for the view, I will open a bug for that.
*** Bug 535450 has been marked as a duplicate of this bug. ***
It's cool, but at least on the Mac, scrolling the editor is very slow if the minimap view is visible. Is there already a bug for performance improvements?
(In reply to Till Brychcy from comment #70) > It's cool, but at least on the Mac, scrolling the editor is very slow if the > minimap view is visible. Is there already a bug for performance improvements? I think this is the same as Bug 366471.
(In reply to Lars Vogel from comment #71) > (In reply to Till Brychcy from comment #70) > > It's cool, but at least on the Mac, scrolling the editor is very slow if the > > minimap view is visible. Is there already a bug for performance improvements? > > I think this is the same as Bug 366471. Sounds related, but I doubt that that will be improved to the point where the current minimap implementation will be fast enough. I think of things like: - Use an offscreen bitmap for caching. - Update the Minimap asynchronously
(In reply to Till Brychcy from comment #72) > Sounds related, but I doubt that that will be improved to the point where > the current minimap implementation will be fast enough. > I think of things like: > - Use an offscreen bitmap for caching. > - Update the Minimap asynchronously Great ideas, could you please open a new bug for these suggestions?
> I think of things like: > - Use an offscreen bitmap for caching. > - Update the Minimap asynchronously Your strategy changes the whole code of minimap. Today minimap uses a StyledText which is synchronized with StyledText from the editor and update the styles by scaling the font. It seems that for Windows OS and Linux OS, performance are very good without using some job or creating a bitmap but not for MacOS. I find it's shame to change this code just for one OS without fixing Bug 366471 before. IMHO this bug should be fixed at first to see if we must rewrite the Minimap code with your idea.
I've created bug 536206 for the issue of mini-map performance on Mac OS.
Angelo, please provide Gerrit for N&N.
> Angelo, please provide Gerrit for N&N. I will try to do that this week.
when I open the View the first time it is opened below editor are. Wouldn't it make more sense to open in e.g. in the same location where the outline is? Another question: How do we tell users that we have this new view?
New Gerrit change created: https://git.eclipse.org/r/125143
> when I open the View the first time it is opened below editor are. Wouldn't it make more sense to open in e.g. in the same location where the outline is? Another question: How do we tell users that we have this new view? Please create a new bug for your new requirement. Goal of this issue was to provide a basic Minimap. If Minimap must be improved please create another bug. Thanks!
> Angelo, please provide Gerrit for N&N. @Lars, please review my N&N at https://git.eclipse.org/r/125143
(In reply to Angelo ZERR from comment #80) > Please create a new bug for your new requirement. Goal of this issue was to > provide a basic Minimap. If Minimap must be improved please create another > bug. Thanks! Bug 536393
Gerrit change https://git.eclipse.org/r/125143 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=9b2c9c28e583a38487b98f8a386fb079060bd7dd
This change can cause a ClassCastException https://git.eclipse.org/r/plugins/gitiles/platform/eclipse.platform.text/+/refs/changes/36/123936/16/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/internal/views/minimap/MinimapPage.java#33 It is not guaranteed that the ITextOperationTarget is always also the ITextViewer and the hard typecast is unsafe. Shouldn't the handling instead check if the given editor inherits from AbstractTextEditor and then get the ISourceViewer from that via AbstractTextEditor.getSourceViewer()?
As there are issues (see https://bugs.eclipse.org/bugs/show_bug.cgi?id=321410#c84) I reopen this bug.
see above
> It is not guaranteed that the ITextOperationTarget is always also the ITextViewer and the hard typecast is unsafe. In theory, I agree with you, but have you a usecase? > Shouldn't the handling instead check if the given editor inherits from >AbstractTextEditor and then get the ISourceViewer from that via >AbstractTextEditor.getSourceViewer()? AbstractTextEditor.getSourceViewer() is protected, and using reflection is not very clean. So we cannot use it I think. in other words, using ITextOperationTarget is the only solution that I know. Perhaps we should keep this mean to retrieve ISourceViewer by checking the instanceof ISourceViewer and not enable minimap in the case of ITextOperationTarget is not an instance of ISourceViewer .
(In reply to Angelo ZERR from comment #87) > in other words, using ITextOperationTarget is the only solution that I know. > Perhaps we should keep this mean to retrieve ISourceViewer by checking the > instanceof ISourceViewer and not enable minimap in the case of > ITextOperationTarget is not an instance of ISourceViewer . Actually it looks like you only need ITextViewer, not ISourceViewer. What about implementing ITextViewer as target for org.eclipse.ui.texteditor.AbstractTextEditor.getAdapter(Class<T>), so you could write: fEditorViewer = (ITextViewer) textEditor.getAdapter(ITextViewer.class) Of course you'll have to handle the case where this returns null.
(In reply to Angelo ZERR from comment #87) > > It is not guaranteed that the ITextOperationTarget is always also the ITextViewer and the hard typecast is unsafe. > > In theory, I agree with you, but have you a usecase? In our ABAP development tools (ADT), editor.getAdapter(ITextOperationTarget.class) does not return the ISourceViewer implcitly. In general, we cannot assume that every implementation of ITextEditor subclasses AbstractTextEditor and even has an ISourceViewer under the hood. > > Shouldn't the handling instead check if the given editor inherits from >AbstractTextEditor and then get the ISourceViewer from that via > >AbstractTextEditor.getSourceViewer()? > > AbstractTextEditor.getSourceViewer() is protected, and using reflection is > not very clean. So we cannot use it I think. Good point. A solution would be to add an internal interface IAbstractTextViewer so that it can be accessed for this use case. > in other words, using ITextOperationTarget is the only solution that I know. It is indeed a nice solution that will likely be able to work with many existing implementations. But as it feels like a hack which works just by chance. > Perhaps we should keep this mean to retrieve ISourceViewer by checking the > instanceof ISourceViewer and not enable minimap in the case of > ITextOperationTarget is not an instance of ISourceViewer . Yes, I do like like the solution, but IMHO it should be a fallback to an more official (and semantically more logial) API (see below). (In reply to Till Brychcy from comment #88) > Actually it looks like you only need ITextViewer, not ISourceViewer. > > What about implementing ITextViewer as target for > org.eclipse.ui.texteditor.AbstractTextEditor.getAdapter(Class<T>), so you > could write: > fEditorViewer = (ITextViewer) textEditor.getAdapter(ITextViewer.class) > > Of course you'll have to handle the case where this returns null. This sounds like the proper way to do it. With the fallback solution it could look like this: fEditorViewer = textEditor.getAdapter(ITextViewer.class); if (fEditorViewer == null) { // try to get the ITextViewer by alternative means: // Try with getting the ITextOperationTarget. It is likely that the instance implementing // ITextOperationTarget is also the same instance implementing ITextViewer. ITextOperationTarget textOperationTarget = textEditor.getAdapter(ITextOperationTarget.class); if (textOperationTarget instanceof ITextViewer) { fEditorViewer = (ITextViewer) textOperationTarget; } }
(In reply to Sebastian Ratz from comment #89) > This sounds like the proper way to do it. With the fallback solution it > could look like this: > > fEditorViewer = textEditor.getAdapter(ITextViewer.class); > if (fEditorViewer == null) { > // try to get the ITextViewer by alternative means: > // Try with getting the ITextOperationTarget. It is likely that the > instance implementing > // ITextOperationTarget is also the same instance implementing ITextViewer. > ITextOperationTarget textOperationTarget = > textEditor.getAdapter(ITextOperationTarget.class); > if (textOperationTarget instanceof ITextViewer) { > fEditorViewer = (ITextViewer) textOperationTarget; > } > } That sounds reasonable for me.
> What about implementing ITextViewer as target for org.eclipse.ui.texteditor.AbstractTextEditor.getAdapter(Class<T>), so you could write: fEditorViewer = (ITextViewer) textEditor.getAdapter(ITextViewer.class) @Till is there any change to provide a gerrit patch?
(In reply to Angelo ZERR from comment #91) > > What about implementing ITextViewer as target for org.eclipse.ui.texteditor.AbstractTextEditor.getAdapter(Class<T>), so you could write: > fEditorViewer = (ITextViewer) textEditor.getAdapter(ITextViewer.class) > > @Till is there any change to provide a gerrit patch? Extending AbstractTextEditor.getAdapter should be trivial. I can provide gerrit this evening if you want.
New Gerrit change created: https://git.eclipse.org/r/126575
(In reply to Eclipse Genie from comment #93) > New Gerrit change created: https://git.eclipse.org/r/126575 Note: I've tested the "null"-case by temporarliy altering createMinimap to return null based on the editor title for a plain Example.java-file and a pom.xml (for testing MultiPageMinimapPage)
(In reply to Till Brychcy from comment #94) > (In reply to Eclipse Genie from comment #93) > > New Gerrit change created: https://git.eclipse.org/r/126575 > > Note: I've tested the "null"-case by temporarliy altering createMinimap to > return null based on the editor title for a plain Example.java-file and a > pom.xml (for testing MultiPageMinimapPage) Angelo has also tested the change. Can a platform committer please commit this.
(In reply to Till Brychcy from comment #95) > (In reply to Till Brychcy from comment #94) > > (In reply to Eclipse Genie from comment #93) > > > New Gerrit change created: https://git.eclipse.org/r/126575 > > > > Note: I've tested the "null"-case by temporarliy altering createMinimap to > > return null based on the editor title for a plain Example.java-file and a > > pom.xml (for testing MultiPageMinimapPage) > > Angelo has also tested the change. > Can a platform committer please commit this. I can do. Would be good to have automated tests for that code. Can you provide some?
(In reply to Matthias Becker from comment #96) > I can do. > Would be good to have automated tests for that code. Can you provide some? I agree tests would be nice but I haven't seen an existing test for the existing code that just needs modification, so that would currently be too much work for me. So sorry, no.
Gerrit change https://git.eclipse.org/r/126575 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=c57ecc40192949a008423009363558df39cf5452
(In reply to Till Brychcy from comment #97) > (In reply to Matthias Becker from comment #96) > > I can do. > > Would be good to have automated tests for that code. Can you provide some? > > I agree tests would be nice but I haven't seen an existing test for the > existing code that just needs modification, so that would currently be too > much work for me. > So sorry, no. Okay I understand this. So this would then be a task for Angelo who has contributes this feature. @Angelo: Can you please provide automated tests for the minimap feature in general.
(In reply to Till Brychcy from comment #95) > Can a platform committer please commit this. done. Thank you Till for fixing this on short notice
> @Angelo: Can you please provide automated tests for the minimap feature in general. To be honnest with you, I don't know which test kind I could write it and how I could write it. Minimap looks like Outline, and I have not found Outline tests too.
(In reply to Angelo ZERR from comment #101) > To be honnest with you, I don't know which test kind I could write it and > how I could write it. Minimap looks like Outline, and I have not found > Outline tests too. As we now deliver more frequent to our customers we don't have a lot of time in which we as commits can find bugs during our daily use of new eclipse versions. So for me automated tests now are even more important. If the outline is a bad example (for having no automated tests) then this should not prevent us to write new tests for new features. If you like an example: Have a look at https://git.eclipse.org/r/#/c/125791/. There we currently develop a new feature directly along with unit tests. Maybe the tests give you some inspiration.
> So for me automated tests now are even more important. I totally agree with you! > If you like an example: Have a look at https://git.eclipse.org/r/#/c/125791/. There we currently develop a new feature directly along with unit tests. Maybe the tests give you some inspiration. This test is not for UI. So to be honnest with you, it doesn't help me. It's very hard to test UI features. I think to have a good test for minimap we should test the MinimapWidget bind with a StyledText (a text editor): * test MinimapWidget content : when StyledText content of editor changed, MinimapWidget content changed. * test MinimapWidget styles : when StyledText styles of editor changed, MinimapWidget styles changed. * test draw of square. I have no idea for the moment how to write that. If someone have some idea, any help are welcome!
(In reply to Angelo ZERR from comment #103) > This test is not for UI. So to be honnest with you, it doesn't help me. It's > very hard to test UI features. I think to have a good test for minimap we > should test the MinimapWidget bind with a StyledText (a text editor): But isn't there stuff that can be tested without UI? > > I have no idea for the moment how to write that. If someone have some idea, > any help are welcome! Can somebody help Angelo with input how to write a good UI testP
New Gerrit change created: https://git.eclipse.org/r/126948
> Can somebody help Angelo with input how to write a good UI testP Forget my comments, tests was easy to write for testing "MinimapWidget content/styles. Please review my gerrit patch https://git.eclipse.org/r/#/c/126948/ > I agree tests would be nice but I haven't seen an existing test for the existing code that just needs modification, so that would currently be too much work for me. So sorry, no. Done with MinimapPageTest in my gerrit patch https://git.eclipse.org/r/#/c/126948/ On the other side, testing draw of square (which seems bugged https://bugs.eclipse.org/bugs/show_bug.cgi?id=536207) is more difficult. If you like my tests, please merge it and I think this issue can be closed, because it exists https://bugs.eclipse.org/bugs/show_bug.cgi?id=536207 to fix the bug with square)
(In reply to Angelo ZERR from comment #106) > > Can somebody help Angelo with input how to write a good UI testP > > Forget my comments, tests was easy to write for testing "MinimapWidget > content/styles. Please review my gerrit patch > https://git.eclipse.org/r/#/c/126948/ > > > I agree tests would be nice but I haven't seen an existing test for the existing code that just needs modification, so that would currently be too much work for me. > So sorry, no. > > Done with MinimapPageTest in my gerrit patch > https://git.eclipse.org/r/#/c/126948/ > > On the other side, testing draw of square (which seems bugged > https://bugs.eclipse.org/bugs/show_bug.cgi?id=536207) is more difficult. If > you like my tests, please merge it and I think this issue can be closed, > because it exists https://bugs.eclipse.org/bugs/show_bug.cgi?id=536207 to > fix the bug with square) Just merged your tests. Well done. Thank you.
Gerrit change https://git.eclipse.org/r/126948 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=191fdca089cad6440d11dbdf726431711d9024c3
New Gerrit change created: https://git.eclipse.org/r/126976
Gerrit change https://git.eclipse.org/r/126976 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=839c84221733f9fc9c6adc7a0a23d4132d79b92a
