From: SourceForge.net <no...@so...> - 2011-01-22 12:12:59
|
Merge Requests item #3159588, was opened at 2011-01-17 02:53 Message generated for change (Comment added) made by vampire0 You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1235750&aid=3159588&group_id=588 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: for 4.4.x Status: Open Resolution: None Priority: 5 Private: No Submitted By: Björn Kautler (vampire0) Assigned to: Kazutoshi Satoda (k_satoda) Summary: fix the settings for building the Windows launcher Initial Comment: Please merge my requests in order of submission, then the first three have according backporting branches and the rest should apply cleanly. Trunk revision: r19209 Message: fix the settings for building the Windows launcher ---------------------------------------------------------------------- >Comment By: Björn Kautler (vampire0) Date: 2011-01-22 13:12 Message: > There seems no reason to drop this chance by widening (r19209) or dropping (your patch) the encoding declareration. The reasons for dropping are: - Launch4j expects an XML-like file without XML declaration, not a real XML file - All our files are UTF-8 and I'd prefer consistency > If you accept showing javaw.exe on Vista and 7, there is no reason to > mimic the name only on XP or wine. I don't like it that on Vista and 7 javaw.exe is shown. I see this as bug in Launch4j and actually there is a bug report about it. I expect this to be fixed in the future by either invoking the JVM through JNI like javaw.exe itself does or by copying the javaw.exe alias jedit.exe to some place that is writable on all Windows systems. The reason to mimic the name only on XP or wine is that it works there fine and that it currently doesn't work on other systems is a bug. Counterquestion, what is the reason to not doing it where it works, just because it doesn't work everywhere due to a bug? o_O That's ridiculous. > Also, it is even confusing since our > launcher has the same name. I think it's better and no problem to show > users the truth. No, it is more confusing and really annoying having it named javaw.exe. I always have mulitple javaw.exe processes running and I never know which one is which one. I always then have to start some 3rd party process watcher that tells me the whole command line so that I see which program the javaw.exe process started. THAT is really confusing and annoying. I as a user would prefer the process to have the name of the programm so that I can see what this process is about. Having two processes named java.exe is no problem in my eyes. And more, if people use the automatic start of the jEdit server, there will still be only one jedit.exe process, because in this cases the --l4j-no-wait option is used and the launcher quits after invoking javaw.exe. Also in my eyes it is more confusing having a javaw.exe and a jedit.exe, because then someone kills jedit.exe and wonders why it is still running. If he sees two jedit.exe, he can just kill both and is not confused. (Please don't get the idea now to make stayAlive false, because then the -wait option of jEdit would be of no use) ---------------------------------------------------------------------- Comment By: Kazutoshi Satoda (k_satoda) Date: 2011-01-22 09:29 Message: > You can also overdo the logical splitting, I didn't want to make single > line commits. ;-) I disagree. Let's go to jedit-devel to discuss this later. > About the encoding thing. Hm, I see, sorry. launch4j uses a plain (snip) > as ensure_only_ASCII_chars.patch Your additional check doesn't crash with my proposal. It seems good to have if you can really do such mistake. With my proposal, the check can be done earlier if you use jEdit (or any of other encoding-safe XML editors). There seems no reason to drop this chance by widening (r19209) or dropping (your patch) the encoding declareration. You can just add the check into trunk and possibly post another merge request for it. > About the custom proc name thingy. I know the issue you describe, but this > is no problem, (snip) I don't aware of the behavior on Vista and 7. Thanks. However, still rejected. If you accept showing javaw.exe on Vista and 7, there is no reason to mimic the name only on XP or wine. Also, it is even confusing since our launcher has the same name. I think it's better and no problem to show users the truth. > About the description, I don't think it is necessary for the installer. I see. Let's drop the discussion from this merge request. ---------------------------------------------------------------------- Comment By: Björn Kautler (vampire0) Date: 2011-01-22 02:25 Message: You can also overdo the logical splitting, I didn't want to make single line commits. ;-) About the encoding thing. Hm, I see, sorry. launch4j uses a plain FileReader to read in the whole file, then replaces some stuff to port 2.x config to 3.x config and AFTER that, when the characters are broken already, they send the whole thing through an xml parser. But I'd like to propose a different solution for this. I'd really like to have all our files uniformly in UTF-8 (except the properties files of course) as this makes it clearer for people that are new to the code. You can state all files are in UTF-8, except of the properties files as per specification. So I suggest to keep the file in UTF-8 but only allow ASCII characters (which makes the file ASCII encoded actually in the end) and remove the xml declaration line like in the launch4j examples, then using a second copy, similar to how I did for the win32installer.iss with outputencoding US-ASCII and check that the two files do not differ. This way the file can be UTF-8 and we ensure programmatically that only ASCII chars are in there. Then also noone can, e. g. by not reading the comment make it another encoding again and actually break it again. My proposed change is attached as ensure_only_ASCII_chars.patch About the custom proc name thingy. I know the issue you describe, but this is no problem, at least not with the launch4j version I used to build the EXE launcher. If the jedit.exe cannot be written as the user has no write permission like in Windows Vista and Windows 7, it will still work, just that the process is named javaw.exe like if the setting would be false. But e. g. on Windows XP or wine, it works fine and the process is called jedit.exe. Also I expect launch4j to fix this issue by either writing the copy to some temp directory or by using JNI to launch the JVM in-process. So I still think this change should be accepted. About the description, I don't think it is necessary for the installer. For the launcher I had a concrete unnicety I wanted to come over. If you choose "Open with ..." from the context menu of a file and choose the launcher, after that in the list of choices only the description is shown and thus it should contain the title of the application. I don't think this is an issue for the installer. ---------------------------------------------------------------------- Comment By: Kazutoshi Satoda (k_satoda) Date: 2011-01-21 19:04 Message: r19209 is still mixing 4 changes. Now I reviewed and accepted 2/4. I want more care about *logical* separation of changes in the future to avoid this kind of difficulty. That said. I propose the attached patch (NOTE: against trunk) as a whole result of my review. Please review it and give the result back here. If you accept the patch, I'll commit it into trunk and merge it in conjunction with r19209 into 4.4.x. Followings are the detail of my review. > -<?xml version="1.0" encoding="US-ASCII"?> > +<?xml version="1.0" encoding="UTF-8"?> Rejected. US-ASCII is the best encoding to avoid problem of Launch4j which uses system default encoding no matter what encoding the config file declare in it. Thus I made r17738. Here, "best" means a common subset of system default encodings on many of known OS and locales. > - <customProcName>false</customProcName> > + <customProcName>true</customProcName> Rejected. The implementation of customProcName=true is, copying javaw.exe into the running JRE directory with the custom name, and run it. This will be banned under valid user permission (for example, not having write permission within %ProgramFiles%). > - <jdkPreference>preferJre</jdkPreference> > + <jdkPreference>preferJdk</jdkPreference> Accepted. I has been convinced with your comment in a previous mail. http://old.nabble.com/Re%3A---jEdit-commits---SF.net-SVN%3A-jedit%3A-19130--jEdit-branches-4.4.x-tp30455645p30459639.html > The main reason for this is the availability of > tools.jar which is needed by some plugins. Don't ask me which, I don't > remember currently. http://www.google.co.jp/search?q=jEdit+tools.jar > - <fileDescription>Programmer's Text Editor</fileDescription> > + <fileDescription>jEdit - Programmer's Text Editor</fileDescription> Accepted. But, shouldn't we do the same thing for VersionInfoDescription in win32installer.iss ? ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=1235750&aid=3159588&group_id=588 |