Community
Participate
Working Groups
In order to better mimic deployment in a servlet container, we typically use the "org.eclipse.equinox.http.jetty.context.path" system property to set the context path (more or less equivalent to the WAR name at deployment). It would be nice to have this be in the "Runtime Settings" section of the run configuration (just like Manual Port configuration) instead of having to set a -D argument. I plan on submitting a patch for this.
Created attachment 192764 [details] Patch to add context ptah to tooling Added a patch to add context path to tooling. I added some appropriate test cases, and noticed a couple of issues with the URL validation and test case. I added a call to url.toURI() in validateUrl() which will throw a URISyntaxException if there are invalid characters in the url. This seems to be what was expected by catching the MalformedURLException, but that only throws if the protocol is invalid. I didn't change the test case for RAPLaunchConfigValidator_Test.testUrl(), but it doesn't make any sense to me. It is checking for an error with the port. I did add a test case for validation of the URL if the context path contains invalid characters and that works properly.
Created attachment 192779 [details] Alternative patch with previous context path changes and preview of the complete url As discussed earlier, I added a preview of the complete url as it would be generated using all of the options on the Main tab. I added it to the browser section since it seems to logically fit there. Austin and I felt it made more sense to have the browser section be the last one, since launching the browser is the final action. When constructing the url, I ran into an issue of what to do when the user does not specify a manual port. As it stands now, I passed -1 to the URLBuilder which effectively generates a url without an explicit port. This could be misleading however since the port would have to be specified in order to actual connect. Clearly we wouldn't want to display an incorrect port, and we can't guarantee that we could display the same one which would be chosen by the launcher. Perhaps something like "http://127.0.0.1:[TBA]/rap" would be better? I also discovered another interesting case, if the user doesn't specify a manual port and disables automatic launching of the browser how would they know what port was chosen? This configuration probably needs to be caught by the validation and flagged to the user at least as a warning.
Thank you for the patch Cole. I will provide feedback ASAP. Currently, we are adding an additional section to the main tab (see bug 254718) therefore we discussed yesterday different layout possibilities. >Perhaps something like "http://127.0.0.1:[TBA]/rap" would be better? +1 >This configuration probably needs to be caught by the >validation and flagged to the user at least as a warning. +1
Created attachment 192949 [details] Updated patch to add [TBD] in url preview for automatic port
(In reply to comment #3) > >This configuration probably needs to be caught by the > >validation and flagged to the user at least as a warning. > +1 Filed a new bug with patch for this case (see bug 342474)
Hi Cole, today I had time to review your patch. Thank you for the patch. During the review I noticed following: * There are some compile errors because you are using java 1.6. RAP tooling uses currently java 1.4. * The source code violates our coding conventions. Please take a look at: http://wiki.eclipse.org/RAP/CodingConventions * Handle context's starting and ending slashes. This should be at most a warning because we can handle it internally (remove or add a slash if needed). * I think we can use a Link or Label for the URL preview because it looks better then the Text widget. * Please don't change the order of "Runtime Settings" and "Browser". I opened a new bug for this (see 342854). * Bug 254718 has been resolved today. Therefore, there are some conflicts with your patch. Could you please resolve these conflicts.
(In reply to comment #6) > Hi Cole, > > today I had time to review your patch. Thank you for the patch. During the > review I noticed following: > * There are some compile errors because you are using java 1.6. RAP tooling > uses currently java 1.4. I will try to get 1.4 setup, but specifically what is broken? > * The source code violates our coding conventions. Please take a look at: > http://wiki.eclipse.org/RAP/CodingConventions I understand this and I tried to follow the conventions, specifically what is wrong? > * Handle context's starting and ending slashes. This should be at most a > warning because we can handle it internally (remove or add a slash if needed). That makes sense, I guess we can just handle it internally and there is no need for a warning. > * I think we can use a Link or Label for the URL preview because it looks > better then the Text widget. > * Please don't change the order of "Runtime Settings" and "Browser". I opened a > new bug for this (see 342854). > * Bug 254718 has been resolved today. Therefore, there are some conflicts with > your patch. Could you please resolve these conflicts. I'll update the patch.
(In reply to comment #6) > Hi Cole, > > * I think we can use a Link or Label for the URL preview because it looks > better then the Text widget. I used a Text widget to allow the user to select and copy the url. I will look at changing to a link widget.
(In reply to comment #7) > (In reply to comment #6) > > Hi Cole, > > * The source code violates our coding conventions. Please take a look at: > > http://wiki.eclipse.org/RAP/CodingConventions > I understand this and I tried to follow the conventions, specifically what is > wrong? * Some missing spaces after/before closing brackets * Lines longer than 100 characters. >That makes sense, I guess we can just handle it internally and there is no need >for a warning. +1 .We have enough warnings. If we can avoid some is better.
(In reply to comment #8) > (In reply to comment #6) > > Hi Cole, > > > > * I think we can use a Link or Label for the URL preview because it looks > > better then the Text widget. > I used a Text widget to allow the user to select and copy the url. I will look > at changing to a link widget. I did some experiments with link instead text field but it didn't look so good because clicking on it doesn't make any sense and you can't copy it. I think we will left it as text widget.
Created attachment 193889 [details] updated patch Updated patch as discussed, against CVS head.
Created attachment 194259 [details] reviewed and updated patch During the review I changed following: * updated some license titles * did smal efectoring * fixed a bug in RAPLaunchDelegate. Starting and ending slashes were not handled here when the VM argument is added.
Applied modified version of patch #5 to CVS HEAD. Added dedicated validation of context path, minor UI tweaks, refactorings, code cleanup.
