Bug 462042 - New Servlet Wizard: Default template code for Servlet
Summary: New Servlet Wizard: Default template code for Servlet
Status: RESOLVED FIXED
Alias: None
Product: WTP Java EE Tools
Classification: WebTools
Component: jst.servlet (show other bugs)
Version: unspecified   Edit
Hardware: PC Linux
: P3 enhancement (vote)
Target Milestone: 3.7   Edit
Assignee: Roberto Sanchez Herrera CLA
QA Contact: Chuck Bridgham CLA
URL:
Whiteboard: greatfix
Keywords: greatfix, helpwanted
Depends on:
Blocks:
 
Reported: 2015-03-12 19:54 EDT by Daniel Azarov CLA
Modified: 2015-04-02 10:32 EDT (History)
3 users (show)

See Also:
cbridgha: review+


Attachments
Suggested patch (5.61 KB, patch)
2015-03-16 19:55 EDT, Daniel Azarov CLA
no flags Details | Diff
updated patch (5.62 KB, patch)
2015-03-16 20:01 EDT, Daniel Azarov CLA
no flags Details | Diff
Generated Servlet sreenshot (105.26 KB, image/png)
2015-03-16 20:02 EDT, Daniel Azarov CLA
no flags Details
Generated Servlet sreenshot (102.53 KB, image/png)
2015-03-17 11:47 EDT, Daniel Azarov CLA
no flags Details
updated patch (11.73 KB, patch)
2015-03-17 17:23 EDT, Daniel Azarov CLA
no flags Details | Diff
Generated Servlet sreenshot (102.12 KB, image/png)
2015-03-17 17:24 EDT, Daniel Azarov CLA
no flags Details
Servlet with no doGet method (71.83 KB, image/png)
2015-03-17 17:25 EDT, Daniel Azarov CLA
no flags Details
Servlet with no doPost method (77.92 KB, image/png)
2015-03-17 17:26 EDT, Daniel Azarov CLA
no flags Details
updated patch (9.85 KB, patch)
2015-03-19 13:40 EDT, Daniel Azarov CLA
shr31223: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Azarov CLA 2015-03-12 19:54:56 EDT
It would be nice to have a default template code generated for Servlet.

This will make the generated Servlet a little bit more useful, and much easier to showcase in demos.

For example, here is one template that could be generated in doGet() and doPost():

protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
response.getOutputStream().print("Served at: " + request.getContextPath());
}

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
doGet(request, response);
}
Comment 1 Roberto Sanchez Herrera CLA 2015-03-13 10:45:09 EDT
Good suggestion for enhancement. A patch is welcome.
Triaging initially for WTP 3.7
Comment 2 Daniel Azarov CLA 2015-03-16 19:55:25 EDT
Created attachment 251623 [details]
Suggested patch
Comment 3 Daniel Azarov CLA 2015-03-16 20:01:45 EDT
Created attachment 251624 [details]
updated patch
Comment 4 Daniel Azarov CLA 2015-03-16 20:02:29 EDT
Created attachment 251625 [details]
Generated Servlet sreenshot
Comment 5 Daniel Azarov CLA 2015-03-17 11:47:40 EDT
Created attachment 251651 [details]
Generated Servlet sreenshot
Comment 6 Roberto Sanchez Herrera CLA 2015-03-17 13:23:50 EDT
(In reply to Daniel Azarov from comment #3)
> Created attachment 251624 [details]
> updated patch

Hi Daniel, thank you for the patch.

I have a couple of comments:

1. In order to contribute patches, you need a CLA. More information here: http://wiki.eclipse.org/CLA

2. In your patch, the doPost method calls directly doGet. But in the new servlet wizard, the user can select to not create the doGet methods. So, if the user only selects to create doPost, doPost would be calling the HttpServlet implementation of doGet. Not sure if we want that.

3. Why did you decide to use response.getOutputStream(), which according to the javadoc: "Returns a ServletOutputStream suitable for writing binary data in the response", instead of response.getWriter(), which seems more appropriate (from javadoc: Returns a PrintWriter object that can send character text to the client.)

4. Did you update manually the file ServletTemplate.java? Or did you let JET to update it based on the changes in servlet.javajet?
Comment 7 Daniel Azarov CLA 2015-03-17 16:16:07 EDT
(In reply to Roberto Sanchez Herrera from comment #6)

Hi Roberto, thank you for your comments

> 1. In order to contribute patches, you need a CLA. More information here:
> http://wiki.eclipse.org/CLA
Done

> 2. In your patch, the doPost method calls directly doGet. But in the new
> servlet wizard, the user can select to not create the doGet methods. So, if
> the user only selects to create doPost, doPost would be calling the
> HttpServlet implementation of doGet. Not sure if we want that.

I am going to test something like:
<% if (model.shouldGenDoPost()) { %>
	protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
<% if (model.shouldGenDoGet()) { %>
		doGet(request, response);
<% } %>
	}
<% } %>
> 
> 3. Why did you decide to use response.getOutputStream(), which according to
> the javadoc: "Returns a ServletOutputStream suitable for writing binary data
> in the response", instead of response.getWriter(), which seems more
> appropriate (from javadoc: Returns a PrintWriter object that can send
> character text to the client.)
How about:
response.getWriter().append("Served at: ").append(request.getContextPath());

> 4. Did you update manually the file ServletTemplate.java? Or did you let JET
> to update it based on the changes in servlet.javajet?
I did it manually. Now trying to figure out how JET works...
Comment 8 Daniel Azarov CLA 2015-03-17 17:10:22 EDT
JET Builder was switched off for org.eclipse.jst.j2ee.web project

when I turned it on - it created new ServletTemplate.java

But copyright header is gone and all NON-NLS comments.
Are there any setting to fix it? or should I add them back manually?

Or should I just get two new lines for new ServletTemplate.java?

Thank you.
Comment 9 Daniel Azarov CLA 2015-03-17 17:23:40 EDT
Created attachment 251670 [details]
updated patch
Comment 10 Daniel Azarov CLA 2015-03-17 17:24:09 EDT
Created attachment 251671 [details]
Generated Servlet sreenshot
Comment 11 Daniel Azarov CLA 2015-03-17 17:25:51 EDT
Created attachment 251672 [details]
Servlet with no doGet method
Comment 12 Daniel Azarov CLA 2015-03-17 17:26:22 EDT
Created attachment 251673 [details]
Servlet with no doPost method
Comment 13 Roberto Sanchez Herrera CLA 2015-03-17 17:26:51 EDT
(In reply to Daniel Azarov from comment #8)
> JET Builder was switched off for org.eclipse.jst.j2ee.web project
> 
> when I turned it on - it created new ServletTemplate.java
> 
> But copyright header is gone and all NON-NLS comments.
> Are there any setting to fix it? or should I add them back manually?
> 
> Or should I just get two new lines for new ServletTemplate.java?
> 
> Thank you.

Usually when we update code generated by JET, we make the changes in the .javajet file, enable the JET builder, and once JET generates what we want, we disable the builder, and edit what JET generated, to fix the copyright headers and add the  NON-NLS comments manually.
Comment 14 Daniel Azarov CLA 2015-03-17 18:42:54 EDT
(In reply to Roberto Sanchez Herrera from comment #13)
> (In reply to Daniel Azarov from comment #8)
> > JET Builder was switched off for org.eclipse.jst.j2ee.web project
> > 
> > when I turned it on - it created new ServletTemplate.java
> > 
> > But copyright header is gone and all NON-NLS comments.
> > Are there any setting to fix it? or should I add them back manually?
> > 
> > Or should I just get two new lines for new ServletTemplate.java?
> > 
> > Thank you.
> 
> Usually when we update code generated by JET, we make the changes in the
> .javajet file, enable the JET builder, and once JET generates what we want,
> we disable the builder, and edit what JET generated, to fix the copyright
> headers and add the  NON-NLS comments manually.

Thank you.

That's exactly what I did for last updated patch. :)
Comment 15 Roberto Sanchez Herrera CLA 2015-03-19 12:47:53 EDT
The patch works fine, but I'd exclude the addition of the file org.eclipse.emf.codegen.JETBuilder.launch and the changes in org.eclipse.jdt.core.prefs.

Requesting review from Chuck.
Comment 16 Daniel Azarov CLA 2015-03-19 13:40:48 EDT
Created attachment 251759 [details]
updated patch

Patch updated,
org.eclipse.emf.codegen.JETBuilder.launch and the changes in org.eclipse.jdt.core.prefs excluded.
Comment 17 Roberto Sanchez Herrera CLA 2015-03-31 12:58:09 EDT
Committed to master for WTP 3.7 (http://git.eclipse.org/c/jeetools/webtools.javaee.git/commit/?id=0a7fa296ed81df94826f44e5cb57f0435cc23979)
I incremented the plugin version as part of the change. 

Resolving.