Community
Participate
Working Groups
Hello, I would like to propose a new kind of view we can find in other IDE like sharp develop for instance. It is the scout view (in #develop idiom) and consists in proposing in a palette vieww by category code snipets that we can drag and drop into the current editor. For some kind of works, it is better than code assist because we can't recall in mind every key shortcuts, and it is a good help to have that presented visually. http://www.icsharpcode.net/OpenSource/SD/Tour/094/ToolScout.aspx best regards Xavier
*** Bug 197243 has been marked as a duplicate of this bug. ***
> We envision a view showing a tree/table with the templates and a part which > allows to edit the selected template. We also need an import/export wizard and > maybe a new template wizard. The view should only show the templates which make > sense in the context of the active editor. > This is all possible but not trivial. > I am not looking at the contexts (that needs tracking of the cursor in the editor) - but surely we can update the view depending on the current editor. Eclipse text editor provides the TemplatePreferencePage which is used by JDT and Ant. If a link can be provided between the templates and the type of editor then the view can change its contents depending on the active editor and show a message when the templates are not available for the current editor. Just supporting a view for the Java editor itself is of great value. I did a small hack to show the preference page in a view and using it as a fast view (see eclipsesnippets.blogspot.com). I find that itself is pretty useful. As for the functionality of export, import, restore defaults etc. - IMO, it is best to leave it in the preferences. The often used functionality like add, delete, modify templates can be provided by the view. The view can be in-addition to the preference page, instead of being a replacement for it. May be I will cookup something and post it in next couple of days. How does this sound?
A simple view that only allows to configure the (Java editor) template preferences is not something we would release. You could offer it a separate plug-in though.
While I can't find the visuals referred to in comment 0, this sounds vaguely like the Snippets view provided by WTP. It has a requirement on GEF, though, which is not part of the default platform.
Yes, I think Benno already made a similar comment. Would it be possible to refactor that view so that it doesn't depend on GEF so that it can be hosted by Platform Text?
(In reply to comment #5) > Yes, I think Benno already made a similar comment. Would it be possible to > refactor that view so that it doesn't depend on GEF so that it can be hosted by > Platform Text? > The problem with that view is that it does not make use of content assist. I prefer the UI of the current templates to that of Snippets view. -- KD
(In reply to comment #3) > A simple view that only allows to configure the (Java editor) template > preferences is not something we would release. You could offer it a separate > plug-in though. > I am not looking for a simple view. Rather a view that supports the current editor and changes contents depending on it. I have ant/java editors that support templates in my eclipse installation. As a POC may be a view that supports these two editors.
Created attachment 74439 [details] palette view This is the palette view contributed by amateras html editor I was referring to.
Created attachment 74440 [details] snipped view This shows WTPs snipped view and its corresponding customize dialog.
(In reply to comment #5) > Yes, I think Benno already made a similar comment. Would it be possible to > refactor that view so that it doesn't depend on GEF so that it can be hosted by > Platform Text? It would take some work, but yes, it could. Nothing that's part of the API relies on GEF. The view can show and hide its categories based on the active part, most often according to the content type being edited by the active editor. The difference in behavior (and content) between Content Assist's Templates and the Snippets view have yet to be settled, though, and that's one of several reasons I've not actively sought its inclusion in the platform thus far. It might be possible to surface the Templates within the view, though, wrapping them in such a way that they trigger the same behavior as an ICompletionProposal when dropped into an editor. It would require some investigation to see what's publicly available through API. Benno, does the Amateras palette use a similar customizing dialog?
Created attachment 74444 [details] Palette configuration dialogs
Created attachment 74505 [details] TemplatesView plugin sources These are the sources for the templatesview plugin. See the followup post for details.
Like I mentioned earlier I created a plugin for TemplatesView and posted it as a zip file. Few points of usage (and few questions at the end): 1. I followed the structure of ContentOutline for creating the TemplateView. 2. Editor needs to provide ITemplatePage through getAdapter(). Since this is not possible without updating the editors themselves, I used Platform#AdapterManager to add template pages to AntEditor and CompilationUnitEditor. When you launch the eclipse workbench, click on the Sample command (or Sample command from Sample Menu). This command does the registration from then onwards the template view should work. 3. The template view is added to the Java perspective. 4. No images - just text buttons on the view toolbar. The toolbar supports add/delete/edit. 5. The View menu support import/export/revert/restore options. Since I tried to create the plugin as an external plugin quite a bit of access restriction warnings and duplicate code is used. Can someone tell me what is the best way to package this type of plugins? Is it as a patch against the Eclipse platform code? This is the first time I am contributing, so any help is appreciated.
One more point. This is the hacked code - all the Java doc etc. need to be updated.
Hello, After hunting around quite a bit, I still could not find how I can do the following. Can anyone throw some light? 1. I have a template and IEditorPart (We can assume that it is an AbstractTextEditor). How can I programmatically invoke content assist? 2. I am planning to add an option to filter the templates only to the currently active content. For this I need to get a callback whenever editor change the currently edited content type. How can I get this. Can we do these with the available public API on editors? TIA.
Hey, that's already pretty nice, I like the PageBookView stuff I would prefer a patch against the platform. I guess org.eclipse.ui.workbench.texteditor for the TemplatePage/TemplateView stuff and org.eclipse.jdt.ui for the Java related stuff. You can then also patch CompilationUnitEditor to adapt to ITemplatePage. Things to do: - It must be possible to drag text from an editor to the view - It must be possible to past text into the view - It must be possible to drag text from the view to an editor. This needs investigation, since the drop would not be a text drop but a special drop which evaluates the variables. Some kind of a TemplateProposal drop. Same for past into editor. - I don't like the table, neither on the preference page nor in the view. At least columns of the table must be sortable. But I would want a tree/table tree with context types as root nodes. But that's probably another bug. - Maybe we should allow to edit the templates in the source viewer in the template view and not (only) in the dialog. Few comments: - ITemplatePage can extend IPage instead of IPageBookViewPage - TemplateView and TemplatePreferencePage share a lot of code, maybe we can find a common abstraction.
(In reply to comment #16) > I would prefer a patch against the platform. from CVS or the 3.3? Both are OK with me. > Things to do: > - It must be possible to drag text from an editor to the view We can invoke the New... editor here. I think the JDT folk should add some magic to provide the content assist for template creation. > - It must be possible to past text into the view OK. > - It must be possible to drag text from the view to an editor. This needs > investigation, since the drop would not be a text drop but a special drop which > evaluates the variables. Some kind of a TemplateProposal drop. Same for past > into editor. I am thinking of double click. When a template item is double clicked we can invoke TemplateProposal#apply. For now, I might extend ITemplatePage to provide the needed glue from the editors. > - I don't like the table, neither on the preference page nor in the view. At > least columns of the table must be sortable. But I would want a tree/table tree > with context types as root nodes. But that's probably another bug. Somehow, I feel that the table view is the easiest to use. Assuming that this view is going to be used often, a tree-table view might not be a right choice. Still, let us wait on this. > - Maybe we should allow to edit the templates in the source viewer in the > template view and not (only) in the dialog. Should be possible. We will not be able to provide a visual clue that an edit is happening and this might not be such a good idea. > > Few comments: > - ITemplatePage can extend IPage instead of IPageBookViewPage IPage is supposed to have been deprecated. > - TemplateView and TemplatePreferencePage share a lot of code, maybe we can > find a common abstraction. Yes. Similarly the (Ant/Java)TemplatePreferencePages have lot of common code with (Ant/Java)TemplatePage. A refactoring is needed here.
(In reply to comment #17) > from CVS or the 3.3? Both are OK with me. CVS HEAD > We can invoke the New... editor here. I think the JDT folk should add some > magic to provide the content assist for template creation. I don't understand. I thought when dropping text a new template will be created with a default name and the Pattern will be the dropped text, no variables. > I am thinking of double click. When a template item is double clicked we can > invoke TemplateProposal#apply. For now, I might extend ITemplatePage to provide > the needed glue from the editors. Double click does edit the template which is what I would expect. I think all we need is a new Transfer like TemplateTransfer and the editor been a DropTarget for TemplateTransfer. When dropped the editor can create and apply the TemplateProposal add the drop position. > Somehow, I feel that the table view is the easiest to use. Assuming that this > view is going to be used often, a tree-table view might not be a right choice. > Still, let us wait on this. We plan to add more context types and templates. I feel that the table does not scale. But more important, it is not possible to drop/past into a context type with the table. > Should be possible. We will not be able to provide a visual clue that an edit > is happening and this might not be such a good idea. Yes, good point. And we don't know how to save/cancel. > IPage is supposed to have been deprecated. Indeed, you're right. I forgot: - View has to listen to preference changes and update accordingly. > 1. I have a template and IEditorPart (We can assume that it is an > AbstractTextEditor). How can I programmatically invoke content assist? I don't know, why? > 2. I am planning to add an option to filter the templates only to the > currently active content. For this I need to get a callback whenever > editor change the currently edited content type. How can I get this. I don't know. I don't think that's the right thing to do. If you drag/copy a template then you can drop/past it anywhere in the editor. Maybe we can have a 'link with editor' mode, but I think a table with context types as root would be a more elegant solution. However, we might want to filter the view based on other criteria, i.e. you don't want to show a foreach template if the active CU does not belong to a 1.5+ project.
> As for the functionality of export, import, restore defaults etc. - IMO, it is > best to leave it in the preferences. Yes, I agree with you now. It does not make much sens to copy the code/functionality of the preference page. It should be possible to create a template by drop/past, delete templates, and maybe rename templates. For everything else the preference page should be used. An action like 'Configure...' should open the appropriate preference page. The value-add it allows to create a new template with drag and drop and that is context sensitive.
(In reply to comment #18) > (In reply to comment #17) > > We can invoke the New... editor here. I think the JDT folk should add some > > magic to provide the content assist for template creation. > > I don't understand. I thought when dropping text a new template will be created > with a default name and the Pattern will be the dropped text, no variables. > You are looking at plain snippets. For making effective use of content assist, name is needed (and it seems to be mandatory, New... template doesn't allow an empty name. > > I am thinking of double click. When a template item is double clicked we can > > invoke TemplateProposal#apply. For now, I might extend ITemplatePage to provide > > the needed glue from the editors. > > Double click does edit the template which is what I would expect. I think all > we need is a new Transfer like TemplateTransfer and the editor been a > DropTarget for TemplateTransfer. When dropped the editor can create and apply > the TemplateProposal add the drop position. > Double click need to perform the default action. In case of templates it is inserting it into the active editor at the current cursor position. This is how, WST snippets also work. For the last few days, I am using the Snippets view. Drag&Drop snippets into an editor might not be such a good idea. For one, the drop position got to be accurate and being text it is tougher to drop at the right place. > > Somehow, I feel that the table view is the easiest to use. Assuming that this > > view is going to be used often, a tree-table view might not be a right choice. > > Still, let us wait on this. > > We plan to add more context types and templates. I feel that the table does not > scale. But more important, it is not possible to drop/past into a context type > with the table. > I am looking into the ExpandBar. It looks good - except that most of the defaults for background, gradient etc. are taken from the Display defaults. Extending it or enhancing it is an option that need to be looked at. > > 1. I have a template and IEditorPart (We can assume that it is an > > AbstractTextEditor). How can I programmatically invoke content assist? > > I don't know, why? > Forget it. Since, I started touching the JavaEditor and AntEditor this becomes moot. > > 2. I am planning to add an option to filter the templates only to the > > currently active content. For this I need to get a callback whenever > > editor change the currently edited content type. How can I get this. > > I don't know. I don't think that's the right thing to do. If you drag/copy a > template then you can drop/past it anywhere in the editor. Maybe we can have a > 'link with editor' mode, but I think a table with context types as root would > be a more elegant solution. However, we might want to filter the view based on > other criteria, i.e. you don't want to show a foreach template if the active CU > does not belong to a 1.5+ project. > Link with the editor is the mode I have in mind. In this mode only the context specific items will be shown. As far as the progress, I checked out the sources from CVS and made it work against them. Once the expandbar part of the work is done, I will post the patch.
Created attachment 74784 [details] View snapshot Hi, This is how currently template view looks. Any comments and suggestions? -- KD
(In reply to comment #21) > Created an attachment (id=74784) [details] > View snapshot > > Hi, > > This is how currently template view looks. Any comments and suggestions? > > -- KD > The Platform and JDT does not use ExpandBar and we will most likely not start using it. I would use a table/tree. But this is to the Platform/Text lead to decide.
Btw I've played a bit with the snippets view: I find it strange, that the customize dialog does show the views content again. This time using a tree instead of expand bars, why? The customize dialog has some resize problems, bug 198242 Some mnemonics are missing. We don't use table cell editors, and if we do we provide a second way to enter data, like an Edit... dialog. But the strangest thing is if you define a template using a variable and you drop this template into an editor a dialog pops up where the user has to enter the values for the variables. The linked mode should be used for this kind of interactions. See screen shot. Also if you cancel the drop a strange exception is drown, bug 198243
Created attachment 74917 [details] Insert variable dialog
(In reply to comment #22) > The Platform and JDT does not use ExpandBar and we will most likely not start > using it. I would use a table/tree. But this is to the Platform/Text lead to > decide. > It is bad - the expand bar view (on OSX atleast) looks really good. I finished view with all the features we discussed. Will post a patch later today (need to rush for a meeting). Anyhow, converting the expandbar into a tree/table should not be a major problem - if that is what makes us stuck. Later.
Created attachment 75026 [details] TemplatesView plugin sources - patch against CVS - using TreeControl Hi, This patch adds a TemplatesView page to the general category. Ant and Java editors are modified to support the view. Like Benno suggested in an earlier message, the control is changed to Tree. I am taking back my words on ExpandBar - this looks good and the code is much more cleaner. Thanks Benno. Features of the templates view: 1. Templates can be added, modified and deleted from the view. 2. Collapse all/Expand all option on the toolbar. The option will act as collapse-all if any one context is being viewed. 3. Drag&Drop from the editors will create a new snippet. You can turn on an option from the View menu to launch the New dialog when a snippet is dropped. You can drag into different contexts and the new template will be created in that context. 4. Copy&Paste - you can copy a template and paste it into the editor. Similarly text from the clipboard can be copied into the templates view. I will post a detailed list of modifications later. Note-1: this patch contains binary files (images). Most of these images are copied from the WST snippet view. Note-2: JavaTemplatesPage.java - the code for insertTemplate() is ugly. Can someone with a knowledge of contentassist with java editor have a look at it? Note-3: can someone knowing AntEditor have a look at the AntTemplatesPage.java Known problems: 1. When the DnD/Copy-Paste can't happen (pasting a template into a readonly editor or incompatible editor) fails silently. I think this is a reasonable solution. Please give me feedback.
Few more missed features: 1. Link to editor mode. When turned on, shows only the templates for the current context. 2. Show disabled items (from view menu): will include the disabled templates in the view. These items are greyed out for visual indication. Otherwise they work as regular templates. 3. Preferences: You can open the preferences page from the menu. 4. Context menu on the items. That should cover all the points. Awaiting your feedback.
>The Platform and JDT does not use ExpandBar and we will most likely not start >using it. I would use a table/tree. But this is to the Platform/Text lead to >decide. We won't start to use ExpandBar at this point. The patch is way too big as it contains many whitespace changes. Make sure to only change what's really needed and then please create and attach a new patch. If we want to put this into ui.views we need to get Platform UI into the boat - adding Tod and Boris to comment on this. Can one of you guys quickly look at this and comment? Thanks. Regarding double-click on a template: I would need to play around with this to make a final statement here. Both suggested actions make sense. Maybe we need to add a mode similar to the History view where the user can choose what happens.
Created attachment 75067 [details] TemplatesView patch: whitespace problems corrected Eclipse save options gave the problem with the earlier patch. Sorry for that. I recreated the patch after removing inadvertent changes. Please bear with first time contributor. The patch is still hefty at around 140K - some of it is because of factoring out EditTemplateDialog from TemplatePreferencePage.
(In reply to comment #28) > We won't start to use ExpandBar at this point. > Fair enough. After working with ExpandBar - I think it still has a way to go before it can be considered for inclusion in standard packages. > The patch is way too big as it contains many whitespace changes. Make sure to > only change what's really needed and then please create and attach a new patch. > Sorry for that. I just posted a new patch. Please have a look. > If we want to put this into ui.views we need to get Platform UI into the boat - > adding Tod and Boris to comment on this. Can one of you guys quickly look at > this and comment? Thanks. > Few reasons why I thought it should be part of ui.views: 1. Structurally both ContentOutline view and Templates view are the same. The only difference is the type of adapter looked for in the EditorPart. 2. Similar to ContentOutline, Template is tightly related to editors. 3. I am visualizing a case where a visual editor might provide a palette using ITemplatesPage interface. In case this goes to ui.views, a refactoring can be done to create an abstract class from which both can be subclassed. > Regarding double-click on a template: I would need to play around with this to > make a final statement here. Both suggested actions make sense. Maybe we need > to add a mode similar to the History view where the user can choose what > happens. I am looking at this view as more of a handy tool to use templates. PreferencesPane can still be used for high maintenance tasks.
>Please bear with first time contributor. Sure - don't worry ;-) >Few reasons why I thought it should be part of ui.views: Don't get me wrong: ui.views is a good host for this that's why I added Tod and Boris to consider. We can't put it into a Text plug-in. The only other host might be ltk.ui which is not yet part of any official build.
Comments to the patch: - you need to attach the resource (images) separate from the patch - org.eclipse.ui.workbench.texteditor cannot depend on org.eclipse.ui.views - the patch breaks clients as it remove API (EditTempalteDialog) - you need to add your name to each copyright and also add the copyright notice to the new files - add since tags to all new APIs (including private members) - PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage().activate(fEditor); should not be used. Simpler is fEditor.getSite().getPage().activate(fEditor). - I see that it uses the same approach as the Content Outline. Clients always had trouble finding out how to extend this. It might be simpler and more visible for clients if we take the extension point road. Comments to the UI/Usability - the view is too big and scrolling behavior should mainly be on the tree and not just tree+preview - I get very strange insertions when I drop a Java code template on Javadoc - drag over feedback should indicate when I can't drop a template, e.g. when I drag a Java code template over Javadoc (see also next comment) - I'm not sure we need to see all templates for the active editor. It might be better to only show those visible in the current context. But not yet sure on that one. - for performance reasons you shouldn't update the templates view on each keystroke. Rather do it on post selection change. This also allows you to decouple it from the editor a bit more. OK - so much for now. Benno please provide more detailed comments on the usability. I will add more comments when the next patch is ready.
(In reply to comment #32) Daniel: Thanks for the feedback. Will do the modifications and post a new patch. Few comments: > Comments to the patch: > - org.eclipse.ui.workbench.texteditor cannot depend on org.eclipse.ui.views Where can we put the TemplatesPage that provides an implementation of ITemplatesPage for text editor? It can't go into org.eclipse.ui.views because it depends on the Template classes. I will relook at the structure. Any suggestions are most welcome. > - the patch breaks clients as it remove API (EditTempalteDialog) I just made it a top level class - it is still the same code. Another option is to make it public in TemplatePreferencePage and make use of it. Does not seem good. However, I did not find any existing plugins making use of EditTemplateDialog. JDT/Ant overrides the editTemplate/createViewer. > - I see that it uses the same approach as the Content Outline. Clients always > had trouble finding out how to extend this. It might be simpler and more > visible for clients if we take the extension point road. > I think ContentOutline took that route because technically it might not be very easy to implement it other way. We need to change the page in the pagebook view whenever the current editor changes. For that we need to inquire the editor for the object to use. I think getAdapter is suitable for this purpose. I am not clear how to go about it using extension points. Any suggestions? > Comments to the UI/Usability > - the view is too big and scrolling behavior should mainly be on the tree and > not just tree+preview Which platform did you check it? The scrolling behavior on OSX is only on the tree. The preview is always fixed size at the bottom of the view. > - I get very strange insertions when I drop a Java code template on Javadoc I guess you got a single character inserted. The JavaTemplatesPage implementation of insertTemplate() is not very nice. I need help from the jdt folks and don't know how to ask for it. > - drag over feedback should indicate when I can't drop a template, > e.g. when I drag a Java code template over Javadoc (see also next comment) Can be done. But actually it is useful sometimes to drop a Javadoc template into a Java comment. Like I mentioned with some help from JDT folks, these can be made better. > - I'm not sure we need to see all templates for the active editor. It might > be better to only show those visible in the current context. But not yet > sure on that one. Yup. I guess you missed link to editor mode. Check it out. > - for performance reasons you shouldn't update the templates view on each > keystroke. Rather do it on post selection change. This also allows you to > decouple it from the editor a bit more. > The templates view is not updated at each keystroke. It gets updated only when the context changes and that too if the link-to-editor mode is on. Thanks for the comments and suggestions.
(In reply to comment #33) > I just made it a top level class - it is still the same code. Another option is > to make it public in TemplatePreferencePage and make use of it. Does not seem > good. However, I did not find any existing plugins making use of > EditTemplateDialog. JDT/Ant overrides the editTemplate/createViewer. Actually we subclass it in WTP (you could say that requirement was my fault). It's necessary to provide the correct syntax coloring while editing our languages' templates.
(In reply to comment #23) > Btw I've played a bit with the snippets view: > > I find it strange, that the customize dialog does show the views content again. > This time using a tree instead of expand bars, why? It was a GEF-lead design. The PaletteViewer UI used by the view and the left-half of the Customize dialog are provided by GEF. > But the strangest thing is if you define a template using a variable and you > drop this template into an editor a dialog pops up where the user has to enter > the values for the variables. The linked mode should be used for this kind of > interactions. See screen shot. This was largely chosen because the view doesn't exclusively target interactions with text editors. Both the dialog that's shown and the transfer type(s) used when dropping a snippet are fully controllable by extension point. And I'm not sure if Linked Edit mode existed when the view was first created.
(In reply to comment #34) > Actually we subclass it in WTP (you could say that requirement was my fault). > It's necessary to provide the correct syntax coloring while editing our > languages' templates. > I will put it back as a protected member extending the EditTemplateDialog with fully qualified name. In that case it does not break the API at the same time I do not need to replicate the code in TemplatesPage. How does it sound? Is snippets view orthogonal to templates? Can we make it to implement ITemplatesPage and provide the getAdapter() to return it?
>I will relook at the structure. Any suggestions are most welcome. As long as we don't even know whether ui.views would accept such a view we can't go discussing further. Maybe it needs to be put into >> - the patch breaks clients as it remove API (EditTempalteDialog) >I just made it a top level class - it is still the same code. The class is a protected class inside another one. If you move it out then this breaks binary compatibility (just look at the name of the class files before and after your change). Your newly proposed change might work but I'm not 100% sure without doing more investigation. I prefer no to do this. From you patch I can't see any other client of the edit template dialog, so why not just *use* the existing one? I don't pan/want to make this class API by itself anyway. >The templates view is not updated at each keystroke I didn't investigate in detail what exactly the view is doing. It's clear from the code that it listens to caret location changes and that's not OK. >Yup. I guess you missed link to editor mode. Check it out. OK, then I'd say the out of the box experience is wrong ;-) Regarding inserting templates from wrong context: simply disallow it, as the "JDT folks" doesn't want to support this. >Which platform did you check it? WindowsXP. There the view scrolls as one unit. I found the view hard to use as I found myself scrolling all the time (see upcoming picture). Some additional comment to the idea of using an extension point: this only makes sense if we think that other kinds of templates (not just the known editor text templates) could be contributed and which then insert some text/code. If that's not the case we can stick with the adapter for now.
Created attachment 75300 [details] Picture showing bad scrolling behavior on WindowsXP
My findings (all with java templates and java editor) -------------------------------------------------------- 1. DnD or copy/paste a template into editor 2. Undo Is: The first character of the template is not removed Should: Remove all characters ------------------------------------------------------- Given: package p; /** *| */ public class E {} 1. Replace '|' with caret 2. Select javadoc templat '<i>' 3. Insert Is: /** <i></i> */ Should: /** *<i></i> */ ------------------------------------------------------- 1. Make file shown in java editor read only 2. Drag and drop a template Is: Template is inserted and editor dirty Should: Dialog should ask to make file writable ------------------------------------------------------- On view resize the column width of the table changes, jumps around, flickers, this on WinXP ------------------------------------------------------- The scrollbars are on the view and not on the tree for me too. ------------------------------------------------------- 1. Select javadoc node 2. New... Is: Dialog shows java as context Should: Have taken javadoc as context ------------------------------------------------------ Should be able to drag and drop a template on its own context. Result would be a copy of the template. ------------------------------------------------------ Should be able to paste a template on itself. Result would be a copy of the template. ------------------------------------------------------ I'm still not convinced of the double click/insert mode. Once the focus is in the template view the caret is not shown anymore in the editor and I don't see where the template will be inserted. ------------------------------------------------------ When in linked with editor mode the context nodes collapse on context switch. The nodes should either be always expanded or remember there expand state. ------------------------------------------------------ What's the 'Show no dialog on drop' action for? I mean, of course we should not show a dialog, unless absolutely necessary. ------------------------------------------------------ 1. Got the following when creating 'New...' template: java.lang.NullPointerException at org.eclipse.ui.texteditor.templates.TemplatesPage.setDisableItemColors(TemplatesPage.java:602) at org.eclipse.ui.texteditor.templates.TemplatesPage.refresh(TemplatesPage.java:1226) at org.eclipse.ui.texteditor.templates.TemplatesPage.access$6(TemplatesPage.java:1224) at org.eclipse.ui.texteditor.templates.TemplatesPage$1.propertyChange(TemplatesPage.java:478) I can not reproduce but TreeItem#getData() is spec'd to possible return null. ------------------------------------------------------- > The JavaTemplatesPage implementation of insertTemplate() is not very nice. > I need help from the jdt folks and don't know how to ask for it. I guess bug 1, 2 and 3 are due to this. I can have a look at it after M1 if you want.
(In reply to comment #36) > Is snippets view orthogonal to templates? Can we make it to implement > ITemplatesPage and provide the getAdapter() to return it? Yes, it's orthogonal. The Snippets view is not a PageBook View, and it's usage wasn't designed for it to be one page in a page book. It shows and hides drawers as needed depending on the active editor's input and the user's preference, largely because we envisioned picking snippets from multiple languages with a single editor.
Created attachment 75416 [details] icons used in templatesview.patch This zip contains all the icons files used in the TemplatesView.
Created attachment 75417 [details] TemplatesView - fixed copyright, bugs and improved(hopefully) UI This patch contains TemplatesView. The images are in a separate zip file that is attached earlier. Sorry for the long post. I am trying to address all comments/suggestions from the earlier posts. Comments: 1. I used SashForm and ViewForm for displaying the view. IMO, it looks nicer. 2. Most probably, the bug with scrollpanes (jumping view, scrollbars, single scrollbar etc.) is fixed. I could not check it on windows - please check it out. 3. Selecting multiple templates is used only for deleting. All other options require a single select. 4. DnD is only copy mode. You can DnD a template into another context and the template is created in that context. 5. Bug with JavaEditor is fixed - You can drop a template into a JavaEditor if the editor itself supports the template at that drop position (hitting ctl-space returns this template in the list) or at the beginning of line, or at the end of a line or after any whitespace character. 6. If the template is dropped within a java identifier - the part before the drop position goes into the template (this is how JavaEditor also works). 7. Can't drop templates into a read-only editor. I did not go with the suggestion with prompting a dialog because the templates view is shown for java class files also. 8. Link to editor mode is the default. 9. Preferences (options, sash size, tree column widths) are saved. 10. I am using bold-italic for the contexts in the treeview. I didn't find any other standard view decorating the nodes. We can remove it if needed. 11. TemplatePreferencePage - I reverted back. Changed the visibility of EditTemplateDialog in TemplatePreferencePage to public so that I can access it from TemplatesPage. I am not happy with this. IMO, the EditTemplateDialog should be a toplevel class (still need not be published) and should be referenced from both TemplatePreferencePage and TemplatesPage. 12. Insertion on Double click seems to be a natural operation on a template. I am using this view for the last few days, and once in a while from the editor I type in a new line and double click a template. Since the focus is back to the editor, I go ahead with typing. It saves quite a few keystrokes. 13. About the show new dialog option: since the effectiveness of the templates is in their ability to insert into an editor by typing the first few characters and invoking content assist, entering a name for the template seems good to me. The alternative is to drop the code, click on edit and modify the name. I suggest we keep it. (BTW, the new dialog is shown even when pasting code or copying a template). 14. Removed dependence of editor on the ui.views. TemplatesPage implements IPageBookPage and since ITemplatesPage is anyhow a marker interface - this does not matter. Though, I prefer explicitly implementing ITemplatesPage. 15. Now, we start a compoundchange while applying a template in JavaTemplatesPage. Undo removes the whole insertion of the template. 16. handleCursorPositionChanged: It is necessary. SelectionProvider#addSelectionListener() not providing the updates properly for JavaEditor(it works for AntEditor and the number of invocations are still the same). Since a single caret position change can change the current context, this is the right place to hook into the editor. 17. When we drop/paste code from an editor the $ characters are being escaped for saving into the pattern. 18. DragOver caching: JavaTemplatesPage caches the result for an offset and returns it if the current offset is same as the earlier one. This is done to improve performance, as dragOver() is called repeatedly even when the drop position does not change. 19. All copyright messages are updated/added. 20. Added @since 3.4 tags to all new API. 21. Sorry for the NPE in the earlier version. bug - fixed. I am wondering whether it is a good idea to provide a default template support at the AbstractTextEditor level. In that case all text editors that do not use the templates still get rudimentary template support and those that provide get their own versions. What do you think? That is all for now. Please give me feedback.
Created attachment 75435 [details] Replace the file in the patch with this file. Fixed few problems with inserting a template.
Please attach a full functional patch that someone can grap and test.
Created attachment 75441 [details] TemplatesView - fixed a problem while inserting a template into JavaEditor This is the full patch with the fix (attachment: 75435) included. You need the icons from attachment 75416 [details] along with this patch. Changes: 1. Does not allow a template to be applied if the context differs. 2. Does not allow a template to be applied if the prefix at the drop position does not match the template. 3. Does validation checks while pasting a template into the editor.
-------------------------------------------------- View should open where the outline view is shown. -------------------------------------------------- Don't use italic for context nodes. Maybe bold. Maybe something like: 'Java (24 Templates)'. No description. ------------------------------------------------- Use the ColumnLabelProvider to set the font and color of columns ------------------------------------------------- Don't resize the column width when resizing the view ------------------------------------------------- Should be able to move a template to another context ------------------------------------------------- I changed my opinion regarding double click. DnD does not allow to wrap a template around a word/line selection, whereas double click insertion does allow that. ------------------------------------------------- I think 'disabled' templates will lead to confusion. The preference page seams not to use the term disabled, which is good for us. We can use the term 'hide' in the context of the view. The action would be renamed to 'Show hidden templates' (or so) and a context menu action should be added to 'hide' respectively 'show' a template. I like that the hidden templates are drawn grayed when shown, but this is not accessible and the meaning of the grayed color is not obvious. Maybe something like: 'catch block (marked as hidden)' ------------------------------------------------- All in all we're close now, ui wise at least, maybe we should use some images in the tree to add a bit of color. I'll attach a screen shot showing my proposed changes.
Created attachment 75769 [details] proposed chages
As I said, I think we are almost there with the UI, but I did not review the code at all and as long as I don't get a go from Platform I will not review the code, as this would just be a waste of time. Tod? Boris? What do you think?
Created attachment 75791 [details] Icons to be used with templates view
Created attachment 75794 [details] TemplatesView - implemented suggested improvements (In reply to comment #46) > -------------------------------------------------- > View should open where the outline view is shown. This is handled by JDT perspective factory. Fixed. I am also opening the view along with the outline view. > -------------------------------------------------- > Don't use italic for context nodes. Maybe bold. Maybe something like: > 'Java (24 Templates)'. No description. I was uncomfortable with the decoration. I removed the decoration. The context nodes show 'Java (24 templates, 3 hidden) > ------------------------------------------------- > Use the ColumnLabelProvider to set the font and color of columns Thanks for the heads-up. TemplateLabelProvider now implements ITableColorProvider. > ------------------------------------------------- > Don't resize the column width when resizing the view Done. > ------------------------------------------------- > Should be able to move a template to another context It is already provided. You can either use edit/DnD or cut&paste. Is it not working that way? > ------------------------------------------------- > I changed my opinion regarding double click. DnD does not allow to wrap a > template around a word/line selection, whereas double click insertion does > allow that. Thanks. Another nice feature is that if a line/word selection does not exist in the template - the template replaces the selected buffer. This is also quite handy. > ------------------------------------------------- > I think 'disabled' templates will lead to confusion. The preference page seams > not to use the term disabled, which is good for us. We can use the term 'hide' > in the context of the view. The action would be renamed to 'Show hidden > templates' (or so) and a context menu action should be added to 'hide' > respectively 'show' a template. I like that the hidden templates are drawn > grayed when shown, but this is not accessible and the meaning of the grayed > color is not obvious. Maybe something like: 'catch block (marked as hidden)' I changed all the labels to hidden. I am still uncomfortable with the terminology as everywhere in the code it is referred to as disabled/enabled. > ------------------------------------------------- > All in all we're close now, ui wise at least, maybe we should use some images > in the tree to add a bit of color. I'll attach a screen shot showing my > proposed changes. > Thanks. It really helped. I am happy with the icons used now - but open to any changes others suggest. Besides the above changes, I cleaned up the JavaTemplatesPage code. Now it uses JavaContext and JavaDocContext extensions for drop position checking - and seems to be much more reliable than the older version.
(In reply to comment #48) > As I said, I think we are almost there with the UI, but I did not review the > code at all and as long as I don't get a go from Platform I will not review the > code, as this would just be a waste of time. > > Tod? Boris? What do you think? > I really hope this becomes part of the eclipse standard. This is quite useful. Please do review the code even otherwise when you get some time. I am sure the feedback will be quite useful to me. As it is, thanks to you and Daniel, I learned quite a bit from this work. -- KD
(In reply to comment #48) > As I said, I think we are almost there with the UI, but I did not review the > code at all and as long as I don't get a go from Platform I will not review the > code, as this would just be a waste of time. > > Tod? Boris? What do you think? Sorry for not responding earlier, I was swamped with other work. It seems that the best way forward would be to push GEF's PaletteView down into the platform (or IDE?). I would be fine with that given that it's another rather general PageBookView that may be used by a number of other projects, not all of which can/should depend on GEF. However, I am sure that this has been discussed before - does someone else have pointers to a previous discussion about this?
I personally think moving GEF into the platform (distribution) would be a good idea. It's runtime is relatively small, and is relatively stable (i.e. should not need a lot of support or "new function"). I'm not sure where it might have been discussed in writing, but in informal discussions, the reasons for not doing it earlier was that there was no reason to make platform larger, if the platform didn't use it. So ... maybe that's changing? I'm adding Anthony (I think its current lead) for awareness/comment.
(In reply to comment #52) > It seems that the best way forward would be to push GEF's PaletteView down into > the platform (or IDE?). I would be fine with that given that it's another > rather general PageBookView that may be used by a number of other projects, not > all of which can/should depend on GEF. However, I am sure that this has been > discussed before - does someone else have pointers to a previous discussion > about this? > IMO, palette view and templates view are two different entities. Taking UML class diagram editor as an example a palette view provides objects like classes, attributes, associations etc. These are the basic elements that can be placed on the canvas. This list should be owned by the editor and is not editable by the user. The templates view for the editor may provide diagram templates for GoF patterns. This should be customizable by the user. I can't think of a need for a palette view for a text based editor - I might be wrong at that. Both PaletteView and TemplatesView share the same structure and code with ContentOutline view. All of them are tightly coupled with editor and change their content depending on the current editor. May be a higher level abstraction needs to be factored out of these.
(In reply to comment #52) > It seems that the best way forward would be to push GEF's PaletteView down into > the platform (or IDE?). I would be fine with that given that it's another > rather general PageBookView that may be used by a number of other projects, not > all of which can/should depend on GEF. However, I am sure that this has been > discussed before - does someone else have pointers to a previous discussion > about this? GEF's PaletteView is simply a PageBookView. All pages in PaletteView implement PalettePage, which is simply a IPageBookViewPage. However, the next step is the definition of a PaletteViewerPage, which is the base implementation of a PalettePage. It is a full GEF object requiring a GEF PaletteViewerProvider and a GEF EditDomain (command stack). So PaletteView is a GEF Object, so you need to be a GEF based editor to make use of it.
(In reply to comment #55) > GEF's PaletteView is simply a PageBookView. All pages in PaletteView implement > PalettePage, which is simply a IPageBookViewPage. > > However, the next step is the definition of a PaletteViewerPage, which is the > base implementation of a PalettePage. It is a full GEF object requiring a GEF > PaletteViewerProvider and a GEF EditDomain (command stack). > > So PaletteView is a GEF Object, so you need to be a GEF based editor to make > use of it. > So we could push down PaletteView and IPalettePage to ui.views. GEF can then keep its PaletteViewerPage base implementation in GEF and we could provide a base implementation in ui/workbench/texteditor? Our implementation would be based on text templates (the patch provided by Kara). How does that sound?
(In reply to comment #56) > So we could push down PaletteView and IPalettePage to ui.views. GEF can then > keep its PaletteViewerPage base implementation in GEF and we could provide a > base implementation in ui/workbench/texteditor? Our implementation would be > based on text templates (the patch provided by Kara). > > How does that sound? > Benno: I have few reservations with this suggestion. 1. A palette view does not make sense for text editors. Palette is strongly associated with graphics. 2. This will foreclose having templates/templatesview for graphical editors. 3. Templates terminology is already in widespread use - changing it might not be a good idea. My suggestion will be to add TemplatesView/ITemplatesPage to the ui.views. PaletteView/IPalettePage should stay with GEF.
(In reply to comment #56) > So we could push down PaletteView and IPalettePage to ui.views. GEF can then > keep its PaletteViewerPage base implementation in GEF and we could provide a > base implementation in ui/workbench/texteditor? Our implementation would be > based on text templates (the patch provided by Kara). > > How does that sound? This is what I was thinking too. We need to be very conservative about which views end up in the platform as universally applicable views. A palette view sounds universally applicable to me. I am not convinced that a template view makes sense for everybody.
Created attachment 76655 [details] Palette and Templates view with UML2 class editor I could not figure out how to get the thumbnail for stored pattern. Otherwise, this is a screenshot from a working prototype that uses TemplatesPage and implements it for DiagramDocumentEditor from GMF. In effect it provides the templates for all the editors based on this. After having a look at it, I feel the TemplatesPage can be used for both text based as well as graphical editors. There is very little that is dependent on a text editor there.
(In reply to comment #58) > This is what I was thinking too. We need to be very conservative about which > views end up in the platform as universally applicable views. A palette view > sounds universally applicable to me. I am not convinced that a template view > makes sense for everybody. > I have given some reasons in comment #54 and comment #57. Please also look at the screen shot I uploaded that shows both palette view and templates view. I hope that will make it clear as to what I have in mind. Now that M1 is out, can someone look at it and see whether this can go into M2?
!!! This answer only targets where to put the Palette/Template view. !!! For WTP I see another problem with the Palette view: its Snippet view is not just tied to WTP editors but can be used along with other editors (e.g. the Java editor) as well. WTP can only migrate to the Palette view if Platform UI provides an extension point so that more than one client can provide Palette items for the active editor. This however sounds like more work for Platform UI than just put in some placeholder and probably also means a major rework for WTP. Boris, Nitin please comment. An alternative approach would be to implement the view in org.eclipse.ui.editors but not register it via plugin.xml. As far as I know if then two clients register that view under the same ID (which Platform Text would make public) then only one view would appear in the UI and that view would then be able to show the templates for the active editor i.e. exactly like the Outline view does.
(In reply to comment #61) I could see us adding another PageBookView, but not a view with internal structure so that multiple plug-ins can contribute items. The latter would not only mean more work, but also that we at the Platform UI would have to decide how palettes look like for any editor. Given the differences between textual and graphical editors, this does not seem like a good idea to me.
!!! This answer only targets the code itself i.e. code review. !!! This is another round of code review. Templates(view) and TemplatePage(s) as a whole are not yet reviewed. Please remove the Ant stuff from the patch. Once the Templates view is committed to HEAD you can file a bug report against Ant and attach the code there. >16. handleCursorPositionChanged: It is necessary. No way ;-). You must not update the view each time the caret or cursor position changes. Use a post selection listener (the Java editor does the same to update the Outline view). - never ever make something API if not really needed, e.g. ITemplatesViewImageConstants - TemplatesViewImages: this is overkill. Simply use the plug-ins image registry. - TemplatePages should not offer API to insert a template - this looks like a layer breaker and needs another solution. - updating of the page needs to be generalized so that not every single editor needs to do it. - same for the ClipboardOperationAction: we need to provide a generic extensible action so that we don't copy the code all over the place. - JavaContextExtension looks strange and pollutes the context class hierarchy - creating your own compilation unit in JavaContextExtension is not needed - you can get it directly from the Java editor - move the Templates view classes into *.templates.view package(s). - field naming conventions sometimes not correctly used. Use - f* for non-static non-final - fg* for static non-final - big letters for constants i.e. static final - rename the view to TemplatesView (almost all out views end with "View") - copyright for files added in 2007 must look like: * Copyright (c) 2007 not: * Copyright (c) 2000, 2007 - add your credential the same in all files and use this format. Name (company if applicable) - text - <full bug url> e.g.: Dakshinamurthy Karra - Templates view - https://bugs.eclipse.org/bugs/show_bug.cgi?id=69581
!!! This now about the UI / usability. !!! - end sentences with a '.' and use similar wording as in the Outline view i.e. "Templates are not available." - we don't use 'Preference Page...' - just 'Preferences...' - don't use generic strings: if you remove one template use singular and not plural - DnD causes exceptions when I drag a context (i.e. a parent node) - >4. DnD is only copy mode. You can DnD a template into another context and the >template is created in that context. It should be possible to move the template to another context as well - 'Copy' is missing from the context menu - if you want to support copying and moving templates from one context to the other then you also need to enable this via Copy/Paste and hence add it to the context menu too. - 'Collapse or Expand...' is strange and we have this nowhere in the UI ==> either remove it completely or replace it with a 'Collapse All' - casing of tool tips is wrong, e.g. 'Link with editor' must be changed to 'Link with Editor' - 'Link with Editor' state must be adopted by next editor which gets opened. This is not the case. - Remove the 'hidden' stuff completely: this also makes the labels shorter (also remove the template count). Otherwise you need to provide support to hide and show templates too and that's overkill. This isn't used that often. - use 'Java', 'Javadoc' and SWT (uppercase initial letter - except for SWT) - I would remove the option to paste a snippet without dialog: without the dialog the drop happens almost unnoticed. We can add it later should someone complain about the dialog. - pasting snippets should generate a new name i.e. "Pasted Snippet" "Pasted Snippet 2", "Pasted Snippet 3" etc. - SWT templates don't use the SWT template icon. - for read-only and Class files don't show the templates view contents i.e. say "Templates are not available.".
>11. TemplatePreferencePage - I reverted back. Changed the visibility of >EditTemplateDialog in TemplatePreferencePage to public so that I can access it >from TemplatesPage. You can access a protected class from the same package - no need to change that visibility.
This got missed by the big amount of feedback: it looks nice and goes into the right direction ;-)
(In reply to comment #63) > Please remove the Ant stuff from the patch. Once the Templates view is > committed to HEAD you can file a bug report against Ant and attach the code > there. > Will do. The reason I did this for AntEditor also is to ensure that the API for TemplatesPage is generic and useful for other editors. Otherwise, unknowingly I might put some dependencies on JavaEditor itself. > >16. handleCursorPositionChanged: It is necessary. > No way ;-). You must not update the view each time the caret or cursor position > changes. Use a post selection listener (the Java editor does the same to update > the Outline view). > Code does not lie ;-). I tried doing the update in the post selection listener and that does not work for Java editor (it works for AntEditor). I did not look at the code, but my guess is that JavaEditor overrides the selection listener to behave differently than AbstractTextEditor. For example, the JavaDoc and the associated element are considered as a single unit and a selection event is fired only when another element is selected. Since we want different templates to be displayed for JavaDoc and Java code, this does not work properly. > - never ever make something API if not really needed, e.g. > ITemplatesViewImageConstants > > - TemplatesViewImages: this is overkill. Simply use the plug-ins image > registry. > The plugin does not have an image registry. That is why I created separate classes for these images (and I followed what seemed to be the conventions for creating images in eclipse). How shall we go about it in this case? Create an image registry at the plugin level? > - JavaContextExtension looks strange and pollutes the context class hierarchy > > - creating your own compilation unit in JavaContextExtension is not needed - > you can get it directly from the Java editor > Thanks for the info. I will look into this. I do not consider the code in JavaTemplatesPage as final. I tried to reduce the amount of modifications done in the JDT per-se. Thanks for the rest of the feedback. (Note: We are in different timezones - so bear with me as I do the modifications and post them tomorrow.)
(In reply to comment #64) > - 'Collapse or Expand...' is strange and we have this nowhere in the UI > ==> either remove it completely or replace it with a 'Collapse All' > I will replace it with 'collapse all'. > - Remove the 'hidden' stuff completely: this also makes > the labels shorter (also remove the template count). Otherwise you need to > provide support to hide and > show templates too and that's overkill. This isn't used that often. > Do you want the hidden templates to be shown? We can remove all other options but still show the hidden templates also with greyed fonts. Since inserting an hidden template still works the same way - it should be OK. > - use 'Java', 'Javadoc' and SWT (uppercase initial letter - except for SWT) > These are the names given by JDT as TemplateContext names. I don't think they should be touched. > - I would remove the option to paste a snippet without dialog: without the > dialog the drop happens almost unnoticed. We can add it later should > someone complain about the dialog. > I agree with you on this. But Benno feels that we should not have a dialog and I feel that we should have a dialog. Seeing as it is we have two view points among developers, may be we should keep it? > - SWT templates don't use the SWT template icon. > I did not understand. Can you clarify? > - for read-only and Class files don't show the templates view contents i.e. > say "Templates are not available.". > Here I differ. It is pretty useful to cut some code from a class/readonly file and paste it into templates for using in our own code. Since the drop is not supported (in case of readonly files, we ask the used whether to make it writeable) - this should be fine.
(In reply to comment #66) > This got missed by the big amount of feedback: it looks nice and goes into the > right direction ;-) > Thanks. -- KD
>The plugin does not have an image registry. Every UI plug-in has one. You can use: TextEditorPlugin.getDefault().getImageRegistry(); >> >16. handleCursorPositionChanged: OK, we/I can take a look at this later. As noted this sync code needs to be generalized (pushed down) anyway. >Will do. The reason I did this for AntEditor also is to ensure that That's good. Keep on doing that but post it a separate patch. >Do you want the hidden templates to be shown? I would completely remove them. This functionality is rarely used and if someone disable the template on the preference it is pretty safe to not show it in the Templates view either. >These are the names given by JDT as TemplateContext names. I don't think they >should be touched. Indeed. I filed bug. >I did not understand. Can you clarify? Use code assist to add an SWT template and you'll see that those templates have a different icon. Ah - just saw you also don't need the template icon used for normal text templates in content assist. We should use the same icon. - read-only files: Point taken ;-) But if linking is enabled the view should show nothing as no templates are available. Ah, and two other things I noticed just a few minutes ago while using it: - DnD inside the view needs to auto-scroll if at the border. - the local tool bar should also get an 'Insert Template' button.
(In reply to comment #68) > > - I would remove the option to paste a snippet without dialog: without the > > dialog the drop happens almost unnoticed. We can add it later should > > someone complain about the dialog. > > > I agree with you on this. But Benno feels that we should not have a dialog and > I feel that we should have a dialog. Seeing as it is we have two view points > among developers, may be we should keep it? Ahh, I thought a dialog is shown when dropping a template into an editor. But a dialog is shown when dropping a snipped into the template view. Showing a dialog when creating a new template is fine.
>>These are the names given by JDT as TemplateContext names. I don't think they >>should be touched. >Indeed. I filed bug. It's bug 202239.
Created attachment 77830 [details] Icons to be used with templatesview
Created attachment 77831 [details] TemplatesView - implemented suggested improvements I implemented all of the comments/suggestions made by Dani with an exception. Few comments: 1. I factored TemplatesPage into TemplatesPage and TextEditorTemplatesPage. The TemplatesPage can be used by any editor that uses jface text template infrastructure. TextEditorTemplatesPage can be used by editors that use AbstractTextEditor. 2. handleCursorPositionChanged: Post selection listener works - sorry, Dani. I somehow missed the *post* part of it and always using SelectionListener which did not work. Now it works. 3. ClipBoardOperation: Again by using AbstractTextEditor#setAction/getAction this can be avoided. However, can someone please look into the way I implemented it in TextEditorTemplatesPage#setupPasteOperation()? 4. Readonly editor and Templates: Implementing not showing the templates is painful (the default message is shown by TemplatesView, not TemplatesPage). Besides this, when we drop a snippet into the TemplatesView we need the tree to be visible. My suggestion is to leave it as it is. Please give your comments/suggestions.
(In reply to comment #74) > Created an attachment (id=77831) [details] > TemplatesView - implemented suggested improvements My review of TemplatesPage: ------------------------------------------------------------------------ The tree flickers on context switch. In TemplatesPage.refresh write: fTreeViewer.getTree().setRedraw(false); try { fTreeViewer.refresh(); fTreeViewer.expandAll(); } finally { fTreeViewer.getTree().setRedraw(true); } ------------------------------------------------------------------------- The icons and +/- signs are too small. Compare i.e. with problems view. The reason is that context.gif is not 16x16 pixel in size. ------------------------------------------------------------------------- Use org.eclipse.ui.internal.texteditor.PixelConverter instead of something like gc.stringExtent("XXXXXXXXXXXXXXX").x; ------------------------------------------------------------------------- Use com.ibm.icu.text.Collator.compare(String, String) to compare strings, always! Write something like Collator.getInstance().compare(a, b); See javadoc of java.lang.String.compareToIgnoreCase(String): 'Note that this method does _not_ take locale into account, and will result in an unsatisfactory ordering for certain locales.' ------------------------------------------------------------------------- Why do you override isSorterProperty in TemplateViewerComparator? ------------------------------------------------------------------------- In TemplatesContentProvider.getTemplates(String) check that the template has not been deleted. I would add a method to TemplateStore: getTemplatesData(String contextTypeId) with a similar implementation as the existing getTemplates(String contextTypeId). ------------------------------------------------------------------------- Rewrite something like if (isActiveContext(contextType) || !fLinkWithEditor) to if (!fLinkWithEditor || isActiveContext(contextType)) for performance and correctness reasons: Code which is not executed can not fail and needs no cpu time. Infix expressions are evaluated from left to right in java. -------------------------------------------------------------------------- TemplatesPage.updateContextTypes(String[]) should be final -------------------------------------------------------------------------- you do not pass fControl to createTemplateTree but to setupPatternViewer, be consistent (pass it to both). Although you pass it to setupPatternViewer you do also reference the field fControl in there. Once you call it createXXXX then you call it setupXXXX, but both method do create something. -------------------------------------------------------------------------- Get rid of fields: fSashSize, fLinkWithEditorAction, fNameWidth, fDescriptionWidth Each field increases the possible states an object can have and makes it therefore harder to understand OO code. -------------------------------------------------------------------------- I _highly_ recommend to you to get rid of fields fSelectedTemplateList and fSelectedType. Always ask the viewer directly for the selection instead, otherwise you have to keep to models in sync which is painfully and very though to maintain. -------------------------------------------------------------------------- I think you can safely get rid of fContextMenu: The menu is disposed when the tree it is attached to is disposed. -------------------------------------------------------------------------- Why can I only drag a single template and not multiple (to i.e. drop it into another context)? -> Get rid of fDNDTemplate. Maybe you need two transfer types: Template transfer and TemplatePersistencetData transfer? -------------------------------------------------------------------------- Enhancement requests: ------------------------------------------------------------------------- It would be very nice if the tree could remember the collapsed/expanded contexts. ------------------------------------------------------------------------- Allow to sort by column ------------------------------------------------------------------------- That's it for now, more to come... Ahh and yes agreed: > This got missed by the big amount of feedback: it looks nice and goes into the > right direction ;-)
(In reply to comment #75) Thanks for the feedback. Few comments below: > ------------------------------------------------------------------------- > Use org.eclipse.ui.internal.texteditor.PixelConverter instead of something like > gc.stringExtent("XXXXXXXXXXXXXXX").x; I want to keep TemplatesPage free of texteditor dependency. Depending where ITemplatesPage and TemplatesView landup finally, TemplatesPage should also go there. > ------------------------------------------------------------------------- > Use com.ibm.icu.text.Collator.compare(String, String) to compare strings, > always! Write something like Collator.getInstance().compare(a, b); > Problem with Cut&Paste ;-). The same code exists in TemplatePreferencePage. How do we go about fixing that? Do we raise a bug against it and provide a patch or fix it with this patch itself? > ------------------------------------------------------------------------- > Why do you override isSorterProperty in TemplateViewerComparator? Again another Cut&Paste problem. Having a look at the Java doc, I think it is required to be overridden and return true. I will check it again. > ------------------------------------------------------------------------- > In TemplatesContentProvider.getTemplates(String) check that the template has > not been deleted. I would add a method to TemplateStore: > getTemplatesData(String contextTypeId) with a similar implementation as the > existing getTemplates(String contextTypeId). Passing false to TemplateStore#getTemplateData() will not return deleted templates. This is OK. > Get rid of fields: > fSashSize, fLinkWithEditorAction, fNameWidth, fDescriptionWidth > Each field increases the possible states an object can have and makes it > therefore harder to understand OO code. The fields are a left-over from the earlier version that updates preferences on dispose(). Fixed. > -------------------------------------------------------------------------- > I _highly_ recommend to you to get rid of fields fSelectedTemplateList and > fSelectedType. Always ask the viewer directly for the selection instead, > otherwise you have to keep to models in sync which is painfully and very though > to maintain. These are used quite often and calling a function to get the current selection and create these each and every time does not look good. > -------------------------------------------------------------------------- > I think you can safely get rid of fContextMenu: The menu is disposed when the > tree it is attached to is disposed. This piece is picked from JavaOutlinePage. I am not sure whether this is needed or not - will do a little experimentation and remove it if not needed. > -------------------------------------------------------------------------- > Why can I only drag a single template and not multiple (to i.e. drop it into > another context)? -> Get rid of fDNDTemplate. Maybe you need two transfer > types: Template transfer and TemplatePersistencetData transfer? Too much work for too little benefit ;-). We do not need another transfer for implementing this. Let us consider it as an enhancement request. > ------------------------------------------------------------------------- > It would be very nice if the tree could remember the collapsed/expanded > contexts. Might not be very useful. As I understand this, you are suggesting that the tree should remember the state for each context - am I right? > ------------------------------------------------------------------------- > Allow to sort by column > ------------------------------------------------------------------------- > Yup. Will attempt once this finds a home ;-). > That's it for now, more to come... > Waiting....
(In reply to comment #76) > > ------------------------------------------------------------------------- > > Use org.eclipse.ui.internal.texteditor.PixelConverter instead of something like > > gc.stringExtent("XXXXXXXXXXXXXXX").x; > > I want to keep TemplatesPage free of texteditor dependency. Depending where > ITemplatesPage and TemplatesView landup finally, TemplatesPage should also go > there. > The reason I did not move TemplatesPage currently is that we need a dependency on jface.text that is not available with ui.views. Pending the final decision on where it will be hosted, I am deferring this refactoring.
(In reply to comment #76) > Problem with Cut&Paste ;-). The same code exists in TemplatePreferencePage. > How do we go about fixing that? Do we raise a bug against it and provide a > patch or fix it with this patch itself? Uuuh, ok, good point;-) Probably got missed when fixing bug 157093, have to ask Dani why Collator is not used. I can't see any reason why to do a comparToIgnoreCase. > Too much work for too little benefit ;-). We do not need another transfer for > implementing this. Let us consider it as an enhancement request. Ok, agreed, but we must at least allow to drag multiple selected templates: 1. it's feels strange that I can't drag if multiple selection. 2. There may be drop targets (in the future) which can handle drop of multiple templates. > Might not be very useful. As I understand this, you are suggesting that the > tree should remember the state for each context - am I right? Yes, remember for each context. Useful because i.e. if I do Swing programming then I'm not interested in SWT templates. Either there must be a way to hide a context or if the state is remembered I can collapse the SWT context node and don't have to look at its children ever again. Hint: This is only an example, we won't show SWT context if SWT is not on the classpath. But even if it is I still may not be interested in SWT templates. > > -------------------------------------------------------------------------- > > I _highly_ recommend to you to get rid of fields fSelectedTemplateList and > > fSelectedType. Always ask the viewer directly for the selection instead, > > otherwise you have to keep to models in sync which is painfully and very though > > to maintain. > These are used quite often and calling a function to get the current selection > and create these each and every time does not look good. Trust me, get rid of it! This will make the code much cleaner, i.e. look at addTemplate: ... fSelectedTemplateList.clear(); fSelectedTemplateList.add(data); changeSelection(); changeSelection: if (isSingleTemplateSelected()) //yes I know that fTreeViewer.setSelection( new StructuredSelection(fSelectedTemplateList.get(0)), true); //I know the first element, it's data! else fTreeViewer.setSelection(new StructuredSelection());//will never hit updateSelectedItems(); updateSelectedItems: fSelectedTemplateList.clear();//oh no! all for nothing! ...(lot of code: retrieving selection from tree, filling into fSelectedTemplateList) //I know the result! updatePatternViewer(getSelectedTemplate()); getSelectedTemplate: //Mmmm what might be the result? it's data.getTemplate! if (isSingleTemplateSelected()) return ((TemplatePersistenceData)fSelectedTemplateList.get(0)).getTemplate(); This are like 50 lines of very hard to understand code which do basically nothing. You can do the same in about 1 line of code (fTreeViewer.setSelection(new StructuredSelection(data), true);). This does not only not look good it is also bad. ---------------------------------------------------------------------------- Just saw this: String suffix= fSelectedTemplateList.size() == 1 ? "" : "s"; String title= NLSUtility.format("Removing Template{0}", new Object[] {suffix}); This only works for English. Other languages may have other/no ways to express plural.
(In reply to comment #78) > ---------------------------------------------------------------------------- > Just saw this: > String suffix= fSelectedTemplateList.size() == 1 ? "" : "s"; > String title= NLSUtility.format("Removing Template{0}", new Object[] {suffix}); > > This only works for English. Other languages may have other/no ways to express > plural. > How to do this properly? Do we keep two NLS strings - one for singular and one for plural?
>How to do this properly? Do we keep two NLS strings - one for singular and one >for plural? Yep ;-)
Created attachment 78536 [details] Icons to be used with TemplatesView
Created attachment 78538 [details] TemplatesView - implemented suggested improvements One more update. Adds the ability to DnD multiple templates within the Templates view. Remembers the collapse/expand state of the contexts. Note-1: The fSelectedTemplateList can't be removed (though I cleaned up the code quite a bit in this area and changed it into an array). The reason is that once a drag operation starts the selection is overwritten. Note-2: The TemplateTransfer uses TemplatePersistanceData. Since these objects are *referred* to by the TemplateStore these are transferred as java objects. The transfer can work only within the same instance of eclipse. IMO, this is OK.
(In reply to comment #82) > Created an attachment (id=78538) [details] > TemplatesView - implemented suggested improvements > > One more update. Adds the ability to DnD multiple templates within the > Templates view. Remembers the collapse/expand state of the contexts. Cool! Thanks! > Note-1: The fSelectedTemplateList can't be removed (though I cleaned up the > code quite a bit in this area and changed it into an array). Never use exceptions for control flow! As you do in setSelectedTemplates. See i.e. here: http://www.onjava.com/pub/a/onjava/2003/11/19/exceptions.html?page=2 In short, throwing exceptions is expensive. The runtime must switch context and generate a stack trace. And it is hard to read/understand, see, you need a comment to explain what you do... Also, what if Iterator#hasNext has a bug which leads to a ClassCastException? > The reason is that once a drag operation starts the selection is overwritten. Sorry, I don't understand. > Note-2: The TemplateTransfer uses TemplatePersistanceData. Since these objects > are *referred* to by the TemplateStore these are transferred as java objects. > The transfer can work only within the same instance of eclipse. IMO, this is > OK. Why don't you use Template in TemplateTransfer? Why do you call getSelectedTemplates() in EditorDropTarget? Use event.data? Why do you set the selection in the view when dropping into editor? Why is editor drop target part of TemplatePage? Shouldn't this code better be in AbstractTextEditor along with setupEditorDropTarget? What is getTransfers and getTemplateDataFromDrop? Wouldn't it be enough if the view is a drop target for TextTransfers? Same for getTemplateDataFromClipboard? The undo first character bug is back. You don't need to insert the first character into the document, use CompilationUnitContext#forceEvaluation(true) instead.
(In reply to comment #83) > > Note-1: The fSelectedTemplateList can't be removed (though I cleaned up the > > code quite a bit in this area and changed it into an array). > > Never use exceptions for control flow! As you do in setSelectedTemplates. See > i.e. here: http://www.onjava.com/pub/a/onjava/2003/11/19/exceptions.html?page=2 > In short, throwing exceptions is expensive. The runtime must switch context and > generate a stack trace. And it is hard to read/understand, see, you need a > comment to explain what you do... Also, what if Iterator#hasNext has a bug > which leads to a ClassCastException? > Thanks. Fixed. > > The reason is that once a drag operation starts the selection is overwritten. > > Sorry, I don't understand. > Once the drag starts the tree selection is reset to the item below the current position of the cursor. The original selection when the drag started is lost. > > Note-2: The TemplateTransfer uses TemplatePersistanceData. Since these objects > > are *referred* to by the TemplateStore these are transferred as java objects. > > The transfer can work only within the same instance of eclipse. IMO, this is > > OK. > > Why don't you use Template in TemplateTransfer? Because they can't be uniquely identified. Two templates can exist in the store with the same settings. Ofcourse we can use Template object references, but since the tree maintains TemplatePersistanceData using them is simpler. > Why do you call getSelectedTemplates() in EditorDropTarget? Use event.data? Nope. setDragData() is called just before the drop(). event.data is available only in drop(). > Why do you set the selection in the view when dropping into editor? Because the selection highlight is removed after the drop. Atleast in OSX. May be a bug in DnD? > Why is editor drop target part of TemplatePage? Shouldn't this code better be > in AbstractTextEditor along with setupEditorDropTarget? I did this factoring for using it with the GMF editor. Basically, an editor should provide only the type of transfers it supports to be dropped into the templates view and ways to convert the selection into a template. > What is getTransfers and getTemplateDataFromDrop? Wouldn't it be enough if the > view is a drop target for TextTransfers? Same for getTemplateDataFromClipboard? > See the above answer. GMF editors use CustomDataTransfer not text transfer. I think this is a better way. > The undo first character bug is back. You don't need to insert the first > character into the document, use CompilationUnitContext#forceEvaluation(true) > instead. > It was never fixed ;-). Some where in the JDT template evaluation (when import statements are being added) a begin/end compound change is being called. Try inserting a template that requires import statements and this will happen in regular insert also. ie. the template will be removed in one undo and import statements require another undo. Anyhow, once forceEvaluation() is called - this should not be a problem. I will check this out. -- KD
Created attachment 79074 [details] Picture of Contribution Questionnaire In order to go further with this we need your personal input to the contribution questionnaire. Please answer all the questions from the attached image.
Let's put the Templates view into Platform Text to support the Platform Text templates - and just those i.e. don't try to be generic and be able to host the Palette view or similar stuff ==> move all the o.e.ui.views changes and the view ID constant to o.e.ui.workbench.texteditor. This also fixes the issue that the TemplatesPage doesn't implement ITemplatesPage. Also, TemplatesPage and TextEditorTemplatesPage can probably be folded together. I still see various new APIs that aren't needed. Please change that. E.g. the whole TemplatesPageImages class.
(In reply to comment #86) > Let's put the Templates view into Platform Text to support the Platform Text > templates - and just those i.e. don't try to be generic and be able to host the > Palette view or similar stuff ==> move all the o.e.ui.views changes and the > view ID constant to o.e.ui.workbench.texteditor. This also fixes the issue that > the TemplatesPage doesn't implement ITemplatesPage. Also, TemplatesPage and > TextEditorTemplatesPage can probably be folded together. > OK. > I still see various new APIs that aren't needed. Please change that. E.g. the > whole TemplatesPageImages class. > I am confused here. Since texteditor plugin does not have any class that defines the image ids, where do I put them? Putting them into TemplatesPage does not seem right. -- KD
(In reply to comment #85) > In order to go further with this we need your personal input to the > contribution questionnaire. Please answer all the questions from the attached > image. > Any online version? Or you just want me to post the answers to this thread? -- KD
Having the class is OK but it should be moved into an *.internal.* package.
You don't have access to the online version. Please post the data here.
(In reply to comment #83) > The undo first character bug is back. You don't need to insert the first > character into the document, use CompilationUnitContext#forceEvaluation(true) > instead. > I tried it with CompilationUnitContext#forceEvaluation(true) - unfortunately, it is not giving the right results ;-). For example, if we select some text and doubleclick a template: 1. If the template does not use selection using a variable, then the template should replace the text. 2. If the template uses selection, then the template is inserted as it is. The (1) is not possible with this. There are few other problems. I am leaving it as it is for now. -- KD
Created attachment 79118 [details] Icons to be used with TemplatesView
Created attachment 79120 [details] TemplatesView - implemented suggestions 1. Removed o.e.views dependency. 2. Merged TemplatesPage and TextEditorTemplatesPage into TemplatesPage. 3. Still keeping ITemplatesPage - useful if some editor needs to implement their own version of TemplatesPage. 4. Created an *.internal.* package and removed the unneeded API 5. TemplatesPage is cleaned up. API looks cleaner ;-). 6. Uses PixelConvertor to calculate the initial widths. -- KD
(In reply to comment #90) > You don't have access to the online version. Please post the data here. Here we go: Description: TemplatesView is a view provides easy access to the text editor templates using TemplatesPage. This view provides options for creating, modifying and deleting templates. Snippets from an editor can be dropped onto the view to create new templates. Templates can be copied/moved between contexts. A TemplatesPage implementation for Java editor is included. Cryptography: No Authors(s): Self -- KD
Created attachment 79124 [details] TemplatesView - fixed a bug with Cut&Paste Implemented multiple selection handling for Cut&Paste. Somehow I missed this out when implementing multiple selection for DnD. Sorry for that.
>Authors(s): Self I have to fill out the form and hence I am "self" ;-) I need your - first and last name (I guess it's Dakshinamurthy, Karra but need to be sure) - organization if written for that company and not at "home" - phone number if possible
(In reply to comment #96) > >Authors(s): Self > I have to fill out the form and hence I am "self" ;-) > OOPS ;-) > I need your > - first and last name (I guess it's Dakshinamurthy, Karra but need to be sure) First Name: Dakshinamurthy Last Name: Karra > - organization if written for that company and not at "home" Organization: Jalian Systems Private Ltd. (Home == work) > - phone number if possible Phone: +918023432215 > Thanks. -- KD
IP review is now under way: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=1793 Note: this is only accessible for committers.
Where did you get the icons? Did you draw them yourself? While the IP process is ongoing there are some improvements I'd like to see: - move the view constant into the view ('ID') - move the help constant from the view into IAbstractTextEditorHelpContextIds - move the stuff from templates.view to templates as we only have three types ==> this also removes the change to the TemplatesPreferencePage - we don't need a message class per class but per package and since we move the types into the templates package we can even get rid of both of them ==> simply move the string defs into TextEditorTemplateMessages and its properties file - move the view contribution (plugin.xml stuff) to JDT UI
(In reply to comment #99) > Where did you get the icons? Did you draw them yourself? > All the icons are from eclipse.org projects. Though I don't know where any individual one has come from :(. I just made a icons directory and un-jarred all icons from the plugins. There should not be any IP/Copyright issues there. > > While the IP process is ongoing there are some improvements I'd like to see: > - move the view constant into the view ('ID') > - move the help constant from the view into IAbstractTextEditorHelpContextIds > - move the stuff from templates.view to templates as we only have three types > ==> this also removes the change to the TemplatesPreferencePage > - we don't need a message class per class but per package and since we move the > types into the templates package we can even get rid of both of them ==> > simply > move the string defs into TextEditorTemplateMessages and its properties file > - move the view contribution (plugin.xml stuff) to JDT UI > I will get some time next week and will work on these. Thanks. -- KD
Created attachment 80754 [details] TemplatesView - implemented suggestions Implemented the suggestions. > - move the view constant into the view ('ID') Done. And this ID is being used in JavaPerspectiveFactory. > - move the help constant from the view into IAbstractTextEditorHelpContextIds Done. > - move the stuff from templates.view to templates as we only have three types > ==> this also removes the change to the TemplatesPreferencePage Done. Fixed the format also. > - we don't need a message class per class but per package and since we move the > types into the templates package we can even get rid of both of them ==> > simply > move the string defs into TextEditorTemplateMessages and its properties file Done. > - move the view contribution (plugin.xml stuff) to JDT UI > Not done. Does not seem right as the view itself is part of texteditor package - why should we add the view contribution of JDT UI. I am on vacation from tomorrow and might not be able to access the net for the next one week. -- KD
>Not done. Does not seem right as the view itself is part of texteditor package >- why should we add the view contribution of JDT UI. Because we only want the view to be there if it is really needed. We don't want it to appear in an RCP that has no templates. If another plug-in, e.g. CDT also wants this view it can simply make the same contribution (with same ID) and there will only be one view even if JDT and CDT is installed. >All the icons are from eclipse.org projects. That's not good enough as different projects can have different legal blurbs and hence the icons might be under different licenses. Anyway, don't bother further I will let all the new artwork redesign by our UI design team.
David, do you see a problem (for WTP) with Platform Text adding this Templates view?
I've logged bug #207242 as a separate dependent bug with a number of comments on the patch contribution 2007-10-19.
Dakshinamurthy, the IP work is progressing. They like you to answer a question in this bug. Here's the original post: --- Comment #10 from Sharon Corbett <sharon.corbett@eclipse.org> 2007-10-25 10:30:39 --- Hi Dani: Can you please arrange to have the contributor make a statement on the referenced bug confirming they authored the code and have the rights to donate the code to Eclipse under the EPL for inclusiion in future Eclipse releases. Please let me know when that has been confirmed. Thanks, Sharon --------------------------------
(In reply to comment #105) > Dakshinamurthy, the IP work is progressing. They like you to answer a question > in this bug. Here's the original post: > > --- Comment #10 from Sharon Corbett <sharon.corbett@eclipse.org> 2007-10-25 > 10:30:39 --- > Hi Dani: Can you please arrange to have the contributor make a statement on > the referenced bug confirming they authored the code and have the rights to > donate the code to Eclipse under the EPL for inclusiion in future Eclipse > releases. Please let me know when that has been confirmed. Thanks, Sharon > -------------------------------- > This is to confirm that the code is developed by myself and I have the rights to donate the code to Eclipse under EPL. -- KD
(In reply to comment #102) > >Not done. Does not seem right as the view itself is part of texteditor package > >- why should we add the view contribution of JDT UI. > Because we only want the view to be there if it is really needed. We don't want > it to appear in an RCP that has no templates. If another plug-in, e.g. CDT also > wants this view it can simply make the same contribution (with same ID) and > there will only be one view even if JDT and CDT is installed. > Dani: I am confused here :(. Is it not the perspectives that define what views are displayed in the workbench and shown in the menus? > >All the icons are from eclipse.org projects. > That's not good enough as different projects can have different legal blurbs > and hence the icons might be under different licenses. Anyway, don't bother > further I will let all the new artwork redesign by our UI design team. > Sorry for the trouble. I will keep track next time. Thanks and Regards KD
>I am confused here :(. Is it not the perspectives that define what views are >displayed in the workbench and shown in the menus? Yep, but there are other places where the view would show up (e.g. Window > Show View > Other...).
(In reply to comment #108) > >I am confused here :(. Is it not the perspectives that define what views are > >displayed in the workbench and shown in the menus? > Yep, but there are other places where the view would show up (e.g. Window > > Show View > Other...). > In most cases this is similar to the content outline view. Templates view depends on the editor to provide the TemplatesPage adapter. Even in JDT perspective, except for the Java editor all other editors shows the templates not available message. Hopefully having the view visible in the workbench might induce other editor developers to provide TemplatesPage using the template infrastructure that is available :). That said, the final call is ofcourse yours. This is just a cut & paste of the view definition from one plugin.xml to another :) -- KD
We won't have time to do this for M4. Will be done early M5.
Committed with some modifications: - view must not be contributed by workbench.texteditor's plugin.xml - changed code and Javadoc to adhere existing style (e.g. use tabs not spaces) - moved JavaTemplatesPage to org.eclipse.jdt.internal.ui.javaeditor - moved Java template page contribution to CompilationUnitEditor - modified code which returns the template page from getAdapter - rewrote code in TemplatesView that gets the adapter - TemplatesPage -> AbstractTemplatesPage - change AbstractTemplatesPage work on ITextEditor interface and not on AbstractTextEditor - moved classes in internal package into public package and made it package visible - for M6 I only added a placeholder for the view as it needs much more testing before going public widely. Depending on the outcome of our next weeks test pass I might also disable the view entirely for M6 There are still some issues regarding the API which I have to cleanup before M6, e.g. passing document as argument sometimes and accessing it directly in other methods. Dakshinamurthy, do you have time to work on the following issues: - retarget the new CollapseAll command (could maybe also use their handler) - write the help doc - fixt the TemplateTransfer implementation which is incomplete and only works inside the same workspace
>- retarget the new CollapseAll command (could maybe also use their handler) See bug 223549. >- write the help doc See bug 223550. >- fixt the TemplateTransfer implementation which is incomplete and only works inside the same workspace See bug 223548.
(In reply to comment #111) > Dakshinamurthy, do you have time to work on the following issues: Good to see that this is moving forward. Unfortunately, I am tied up till mid of April, after which I can have a look at the pending stuff. Thanks and Regards KD
