Bug 140752 - wizards: create html/jsp/css/js files outside the webcontent dir
Summary: wizards: create html/jsp/css/js files outside the webcontent dir
Status: CLOSED FIXED
Alias: None
Product: WTP Source Editing
Classification: WebTools
Component: wst.sse (show other bugs)
Version: 1.5   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 1.5 RC5   Edit
Assignee: Amy Wu CLA
QA Contact:
URL:
Whiteboard: PMC_approved
Keywords: greatbug
: 103404 144650 144651 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-05-09 04:43 EDT by Kaloyan Raev CLA
Modified: 2014-07-03 11:57 EDT (History)
9 users (show)

See Also:


Attachments
Patch (85.23 KB, patch)
2006-05-09 04:43 EDT, Kaloyan Raev CLA
no flags Details | Diff
revised patch (77.18 KB, patch)
2006-05-25 06:39 EDT, Kaloyan Raev CLA
no flags Details | Diff
jspcsshtmljsui.patch (77.71 KB, patch)
2006-05-26 19:40 EDT, Amy Wu CLA
no flags Details | Diff
jspcsshtmljsui.patch (77.65 KB, patch)
2006-05-31 15:35 EDT, Amy Wu CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kaloyan Raev CLA 2006-05-09 04:43:03 EDT
There are for file types that are specific to Web Projects: HTML, JSP, CSS and JavaScript. These files have to be placed inside the webContent folder of the project in order to be packed in the resulting WAR archive. Placing these files outside the webContent folder has no meaning. 

At the moment it is relatively easy for the user to create web files outside the webContent dir, and even not in a web project at all. This is because of two main reasons: 
  1. The 'New HTML' and 'New JSP' menu items appear in the context menu of each file/menu inside an arbitrary project type (even non-web projects). 
  2. In the New File wizard there is no check if the file is placed in the webContent dir or even if in a web project. 
  
This way the user is misled for the right location of the HTML/JSP/CSS/JS file types. 

A patch is attached that solves the described issue. The patch covers all the four file types. 
The effect of the patch is as follows: 
  1. Put a couple of checks in the New File wizard: 
    1.1. Checks if the destination project is Static Web Project or Dynamic Web Project. If not, an error message is shown and the user cannot finish the file creation. 
    1.2. Checks if the destination folder is inside the webContent folder path. If not, a warning message is shown, but the user still can finish the file creation. 
  2. Shows the 'New File' menu items in the context menu if: 
    2.1. The context menu is of a Static Web Project or Dynamic Web Project. 
    2.2. The context menu is of a folder or a file inside the webContents directory and the resource's project meets the rule in 2.1.

The above effect is valid for HTML, CSS and JS file types. There is a little exception for the JSP file type. There is no sense JSPs to be placed inside Static Web Project, so the checks for JSPs are strictly for Dynamic Web Projects.

Some details around the implementation of the patch: 
  - The check if a project is a Static Web or Dynamic Web is done by checking if the project has 'wst.web' or 'jst.web' project facet, respectively. 
  - The webContent folder source path is obtained by getting the source path of the component resource which runtime path is root. 
  - The filter of the 'New File' menu itmes in the context menu needed implementing new PropertyTester: to check if the current resource is inside the webContent dir of a web project. Since this PropertyTester is needed by the eclipse projects of all for file types, the best place, I could find for it, is the org.eclipse.wst.common.ui project: 

  org.eclipse.wst.common.ui.internal.propertytester.ResourcePropertyTester
Comment 1 Kaloyan Raev CLA 2006-05-09 04:43:32 EDT
Created attachment 40689 [details]
Patch
Comment 2 Amy Wu CLA 2006-05-09 09:53:24 EDT
I know we already have a bug or 2 open on this issue, so thanks so much for providing a patch for this.  I'll check it out.
Comment 3 Amy Wu CLA 2006-05-09 12:49:25 EDT
Just want to note maybe fix bug 103404 as well when looking into this bug.
Comment 4 Amy Wu CLA 2006-05-23 17:51:35 EDT
Okay, I've looked over the patch a bit.
Regarding visibility of New file wizard actions on folders/files in project explorer:
-I agree that the new file wizard actions should not appear on folders/files that are not web projects.  I have opened bug 143082 so that a property tester will be made available to test this.
-However, I think that limiting the visibility of the action to the web content folder may be too much.  Sometimes a user may just want to select the web project or select anywhere inside of a web project and create a new file.  And then expect that the wizard will put the new file in the appropriate place.  I think this approach is better.

Regarding selecting location for new file in wizard:
-I like that if the user does not initially select a location inside the WebContent folder, we will select the WebContent folder for them.  This would take care of bug 103404.
-I'm not sure, however, if we should completely prohibit the user from selecting a location outside of the WebContent folder.  Or not even in a web project.  I think a warning would suffice.  But if the user wants to place their file elsewhere, we could allow it.  It is possible, for example, for user to create a Java project, instead of a web project, and then add the necessary jars to the build path in order to create a dynamic web project.  We want to make sure we allow users to add their web files in their java project then.

Kaloyan, all that being said, would you like to take another shot with this patch?  You'd basically need to remove the common.ui property tester code and use the property tester from bug 143082 instead.  And then instead of throwing any errors in the wizards, put up a warning.

There is also the matter of how you retrieved the WebContent folder.  It should actually be something like this:
public IPath getWebContentPath(IProject project){
  IVirtualComponent component = ComponentCore.createComponent(project);
  IPath modulePath = component.getRootFolder().getWorkspaceRelativePath();
  return modulePath;
}
That will get you the first web content folder.  There are actually potentially several web content folders possible, so that should also be taken into consideration when checking to see if current location selection is in the WebContent folder.
Comment 5 Kaloyan Raev CLA 2006-05-25 06:39:05 EDT
Created attachment 42554 [details]
revised patch
Comment 6 Kaloyan Raev CLA 2006-05-25 06:47:05 EDT
Amy, thank you for your comment and hints. 
I prepared new patch (attached as "revised patch") that conforms to your suggestions. 

When I decided what to be error or warning and where to appear the "New <Web> File" menu, I tried to follow the situation with Java files, e.g. you can't (error) create Java class outside java project, but you may (warning) create it outside the source directory. 

Are there UI guidelines that cover the topic?
Comment 7 Amy Wu CLA 2006-05-26 19:35:18 EDT
The patch looks pretty good.  Here are some minor modifications I made to the patch I attached:
-decreased visibility of wizard pages (made class package protected and methods private when possible)
-when an exception is thrown, catch and log instead of catch and throw runtime exception
-the template name was being passed in for the newXXXfilewizardpage's name.  I assume this was a copy & paste error, and changed it to the original page names
-instead of "wst.web" & "jst.web" use IModuleConstants.WST_WEB_MODULE & IModuleConstants.JST_WEB_MODULE
-made warning messages say:
The *file should* be inside Dynamic Web Project. 
The *file should* be inside the Web Content folder.
instead of *folder must*
-removed css & js wizard from the project navigator so as to not clutter the menu.  if we get more requests to add it, then we can at a later time

I'm not sure if there are any UI guidelines on when to prohibit a user from creating a file outside of an expected project and when to just give them a warning.  I've added Janet & Lawrence for an feedback on this.  But my feeling is we should allow users to create web files in non-web projects because our ediors will work just fine.  But we should give users a warning that it's not recommended.

Janet, Lawrence, any feelings on this?

Thanks again for the patch, Kaloyan!
Comment 8 Amy Wu CLA 2006-05-26 19:40:32 EDT
Created attachment 42795 [details]
jspcsshtmljsui.patch
Comment 9 Lawrence Mandel CLA 2006-05-26 19:50:08 EDT
Amy, I think we need to be careful with these warnings. There are other valid project types that can contain these files types. As one example, documentation plug-ins typically contain HTML and CSS files. I've also seen them with JavaScript and they may at some point support dynamic file types like JSPs. It's also possible at a WTP adopter will create their own "Web" project type that will support these file types.

Instead of simply warning any time they're not in the webcontent folder I think it will be better to constrain these message to only appearing if a user adds the files in a Web project but outside of the webcontent folder. In this case I think only the message
"The *file should* be inside the Web Content folder."
is applicable and it should be converted into a choice dialog (OK/Cancel) and provide more information about the problem such as
"You have selected to create a Web file outside of the webcontent folder. Files outside of the webcontent folder will not be included in your deployed Web application. Do you want to create this file in the specified location?"
Comment 10 Amy Wu CLA 2006-05-30 15:02:08 EDT
Okay, so let's throw away the warning message about web files should be in a web project.  And only display some sort of warning about web files should be in the webcontent folder only when user is creating the file somewhere inside a web project.

Popping up a messagebox in the wizard dialog doesn't seem right to me.  It seems like typically, any messages to the user are displayed in the wizard dialog itself, in that status text at the top of the wizard.  How about displaying a warning of:
"Files created outside of the Web Content folder will not be included in your deployed Web application."

Also, I would like to note that this defect may have to be deferred till 2.0 because we don't have much time to submit PII changes.  I may end up opening up a second defect to at least add in the change where the New web files wizard menu items only show up in the navigator's context menu when user selects a resource inside a web project.  I feel this should at least be fixed to reduce clutter in the context menu.
Comment 11 Lawrence Mandel CLA 2006-05-30 17:04:38 EDT
Amy, +1 to your suggested change of putting the warning in the wizard. I think the message is good as well.

Jeff, what's WTP's timeline for PII changes? Can this change still get included? If not, is our policy to hold off on aditional changes until 1.5.1?
Comment 12 Jeffrey Liu CLA 2006-05-30 17:20:34 EDT
I did a quick scan, the new PII volume is about 80 words. Is this about right? If so, please have them committed into a build before the end of this week (06/02), ideally, into RC4 if it's not too late...
Comment 13 Amy Wu CLA 2006-05-31 13:15:33 EDT
*** Bug 144651 has been marked as a duplicate of this bug. ***
Comment 14 Amy Wu CLA 2006-05-31 15:21:21 EDT
While we should not warn users about creating web files outside of web
projects, I think Bug 144651 raised a good point that we should specifically
warn users when they attempt to create a jsp in a non-java project (like static
web projects).  The jsp editor will not be fully functional if the jsp is in a
static web project (no java content assist proposals/validation).  So I'd like
to warn users in this specific case (and I say warn instead of error/forbid
because who knows, maybe they want to put jsps there on purpose)

As for PII, I thought it was based on unique word count.  The new volume with
the most recent changes would be 1 string in jsp.ui:
"JavaServer Pages created in projects that do not support Java might not work
as expected."
~15 words
And one string:
"Files created outside of the Web Content folder will not be included in your
deployed Web application."
~17 words
repeated in 4 plugins.  

I'll try to create a new patch based on the new issues we discussed and see if
I can get it in for this week.
Comment 15 Amy Wu CLA 2006-05-31 15:35:49 EDT
Created attachment 43157 [details]
jspcsshtmljsui.patch

Made the changes mentioned in previous comment(s).  When creating JSP's I check if user's selected project has the java nature, if not, I put up that warning about creating jsps in non-java projects (dynamic web projects support java while static web projects do not)
Comment 16 Jeffrey Liu CLA 2006-05-31 15:58:55 EDT
Yes, it's based on unique word count. So the volume is small enough. Please request PMC approval for this fix asap and let me know which build contains it. Thanks.
Comment 17 David Williams CLA 2006-05-31 16:39:57 EDT
+1 Thanks Amy, and Kaloyan. Good "team" work:) 

Comment 18 David Williams CLA 2006-05-31 16:44:35 EDT
I'd like to nominate this bug, Kaloyan Raev, in particular for a "great bug". 
Its a complex issue, made more complicated, and more important, in the context of having many components and project types like Callisto does. While we did use the exact patches supplied, they definitely helped move this along, and make feasible for WTP 1.5 (Callisto ... technicall will be in RC5, if they are counted that way). 

Comment 19 Amy Wu CLA 2006-06-01 09:38:57 EDT
*** Bug 144650 has been marked as a duplicate of this bug. ***
Comment 20 Amy Wu CLA 2006-06-01 09:40:56 EDT
*** Bug 103404 has been marked as a duplicate of this bug. ***
Comment 21 Arthur Ryman CLA 2006-06-01 14:33:20 EDT
+1 for WTP 1.5 RC5.
Comment 22 Amy Wu CLA 2006-06-01 18:08:15 EDT
fix committed and released for 1.5 RC5
Comment 23 Jeffrey Liu CLA 2006-06-02 10:52:41 EDT
Use the new PMC approval process.
Comment 24 Tim Wagner CLA 2006-06-05 15:05:16 EDT
+1 for 1.5.
Comment 25 Kaloyan Raev CLA 2006-06-20 08:39:39 EDT
verified with RC5
Comment 26 Amy Wu CLA 2006-06-20 15:12:47 EDT
closing