Community
Participate
Working Groups
Created attachment 94829 [details] Support for setting file attributes and times This patch is adding support for setting file attributes and file times in the tm.rapi library. This is needed for implementing setLastModified() and setReadOnly() in the WinCE file subsystem. The patch is also including unit tests for the new functionality.
Created attachment 94832 [details] jrapi.dll including the changes Attaching the updated native library. Legal Message: I, Radoslav Gerganov, declare that I developed attached code from scratch, without referencing any 3rd party materials except material licensed under the EPL. I am authorized by my employer to make this contribution under the EPL.
Thanks for the patch, it looks good especially thanks to the test cases. 132 lines of code for now. What I'm missing is Javadoc about the meaning of the "int" arg for fileAttributes; also, about the accuracy of setting file times. You might want to take a look at the EFS Javadocs in org.eclipse.core.filesystem as well as java.io.File and RSE IHostFile: IFileInfo#setLastModified() File#setLastModified() IHostFile#getLastModifiedDate() Note that when looking at java.io.File, Copyright rules will not allow you copy & paste any test from the Sun Javadocs but you can certainly learn about the concepts (it's an API so it's whole purpose is to be understood). Looking at java.io.File and org.eclipse.core.filesystemIFileInfo, I'm also wondering whether "Date" is a good type to represent the last modified time in your APIs. Have you read the class Javadoc of Date? The Date class itself is relatively lightweight, but it does have a dependency on Calendar which is not that lightweight and might not be needed in many occasions. Awaiting your comments before I commit this.
(In reply to comment #2) > What I'm missing is Javadoc about the meaning of the "int" arg for > fileAttributes; also, about the accuracy of setting file times. You might want > to take a look at the EFS Javadocs in org.eclipse.core.filesystem as well as > java.io.File and RSE IHostFile: > IFileInfo#setLastModified() > File#setLastModified() > IHostFile#getLastModifiedDate() > the int parameter for setFileAttributes is combination of OS.FILE_ATTRIBUTE_XXXX OR-ed constants. I will add clarification for this in the javadoc. the accuracy of setFileTime is about 1-2 sec and getFileXXXXTime is returning truncated value. For example if I call: setFileTime(handle, null, null, new Date(1207314503484)) getFileLastWriteTime(handle) returns: 1207314502000 that's why I have added 5sec. treshold in the unit test. I agree that this behaviour should be documented in the javadoc, so I will add it. > Note that when looking at java.io.File, Copyright rules will not allow you copy > & paste any test from the Sun Javadocs but you can certainly learn about the > concepts (it's an API so it's whole purpose is to be understood). > > Looking at java.io.File and org.eclipse.core.filesystemIFileInfo, I'm also > wondering whether "Date" is a good type to represent the last modified time in > your APIs. Have you read the class Javadoc of Date? The Date class itself is > relatively lightweight, but it does have a dependency on Calendar which is not > that lightweight and might not be needed in many occasions. > > Awaiting your comments before I commit this. > I thought that using Date will be more convinient but it seems that all of the APIs for setting and getting file times that you pointed are using plain long values. I agree to change my APIs (including these for getting file times) in order to be aligned with the others. I will explicitly state in the javadoc that (January 1, 1970) epoch timestamps should be specified. Do you agree with these changes?
Sounds all good to me. Note that Date can still be used on a higher level by clients if they want, but using long on the lowest level is the least overhead. Given that I didn't find the FILE_ATTRIBUTE_xxx constants instantly, I'm wondering if "OS" is a good name for the class they reside in? What about renaming it to "Rapi"?, also what about renaming the constants to RAPI_FILE_ATTRIBUTE_xxx in order to disambiguate and avoid name clashes in situations where a client wants to use both windows local native constants as well as RAPI constants? You might also want to use Eclipse 3.4M6 API Tooling tags in your Javadoc: For instance, OS which is holding constants, should be @noextend @noinstantiate just to clarify (it's never a good idea to extend a class or implement an interface, just to bring constants into visibility scope). See http://wiki.eclipse.org/API_Tooling_User_Guide http://wiki.eclipse.org/API_Javadoc_tags
I have one more API question: Currently your RAPI is all modeled as public abstract class IRapi*... and the like. But looking at the contents of the abstract classes, there is no implementation at all - except passing an address into the constructor, which finally ends up in IUnknown. I know that you're modeling the JRapi after original COM Objects, which have C++ heritage and thus only know abstract classes but no interfaces. Therefore I'm wondering whether it would make sense to modle the IRapi* stuff as Java interface rather than abstract class? In Java, advantage of interface over abstract class is that you can "mix in" interfaces into different class hierarchies; so you could eventually have a totally different implementation of, say, IRapiSession, from a different class hierarchy. Mock Objects use such different hierarchies for testing, for instance. If you stick with abstract classes, your advantage is that an abstract class is more easily evolved adding new methods since you can always provide a default implementation. The disadvantage of abstract classes is that you're always confined to a single line of inheritance hierarchy in Java (whereas C++ supports multiple inheritance so it's not an issue there). I'm wondering if there is any "standard approach" for modeling COM objects in Java that should be adopted? What do you think?
(In reply to comment #4) > Given that I didn't find the FILE_ATTRIBUTE_xxx constants instantly, I'm > wondering if "OS" is a good name for the class they reside in? What about > renaming it to "Rapi"?, also what about renaming the constants to > RAPI_FILE_ATTRIBUTE_xxx in order to disambiguate and avoid name clashes in > situations where a client wants to use both windows local native constants as > well as RAPI constants? > I don't think renaming the constants is a good idea since most of them are not RAPI specific. Actually only FILE_ATTRIBUTE_INROM, FILE_ATTRIBUTE_ROMMODULE and FAF_XXX are RAPI specific but I don't see any reason to rename them. The other constants come from the Win32 Platform SDK so there are no name clashes. The OS class is providing both win32 and rapi facilities and renaming it to "Rapi" will be confusing. If you have another suggestion I'll be happy to hear it. > You might also want to use Eclipse 3.4M6 API Tooling tags in your Javadoc: For > instance, OS which is holding constants, should be @noextend @noinstantiate > just to clarify (it's never a good idea to extend a class or implement an > interface, just to bring constants into visibility scope). See > http://wiki.eclipse.org/API_Tooling_User_Guide > http://wiki.eclipse.org/API_Javadoc_tags > Sure, I am aware of this tooling and I will take advantage of it in my next patches.
(In reply to comment #5) > I have one more API question: > > Currently your RAPI is all modeled as > public abstract class IRapi*... > and the like. > > But looking at the contents of the abstract classes, there is no implementation > at all - except passing an address into the constructor, which finally ends up > in IUnknown. > > I know that you're modeling the JRapi after original COM Objects, which have > C++ heritage and thus only know abstract classes but no interfaces. Therefore > I'm wondering whether it would make sense to modle the IRapi* stuff as Java > interface rather than abstract class? > > In Java, advantage of interface over abstract class is that you can "mix in" > interfaces into different class hierarchies; so you could eventually have a > totally different implementation of, say, IRapiSession, from a different class > hierarchy. Mock Objects use such different hierarchies for testing, for > instance. > > If you stick with abstract classes, your advantage is that an abstract class is > more easily evolved adding new methods since you can always provide a default > implementation. The disadvantage of abstract classes is that you're always > confined to a single line of inheritance hierarchy in Java (whereas C++ > supports multiple inheritance so it's not an issue there). > > I'm wondering if there is any "standard approach" for modeling COM objects in > Java that should be adopted? What do you think? > IRapi* are not intended to be implemented by clients (which should be added in the javadoc as well). I can't think of any usecase where one will want to mix them in another class hierarchy. With abstract classes I can easily provide common implementation for release(), hashCode(), equals() and maybe some other stuff as well. I have seen COM modeling in SWT, they are using concrete classes and inheritance, see org.eclipse.swt.internal.ole.win32.IUnknown and its extenders. In my case I don't want to provide full implementation of, let's say, IUnknown so I am using abstract base classes.
(In reply to comment #6) > I don't think renaming the constants is a good idea since most of them are not > RAPI specific. Actually only FILE_ATTRIBUTE_INROM, FILE_ATTRIBUTE_ROMMODULE > and FAF_XXX are RAPI specific but I don't see any reason to rename them. The > other constants come from the Win32 Platform SDK so there are no name > clashes. I think you're misunderstanding. You're right that FILE_ATTRIBUTE_SOMETHING are Windows API constants, but JRapi is in the Java space and its clients are Java programs. They do not know Windows API, but they might want to link against couple of other Java Libs which also define some constants with some names. The constants with the same names are in different packages for sure, so even if you happen to us e.g. you OS.FILE_ATTRIBUTE_READ_ONLY and EFS.FILE_ATTRIBUTE_READ_ONLY and TCF.FILE_ATTRIBUTE_READ_ONLY in the same file you can always qualify them by package name. But this doesn't help making the code more readable. And, especially, having your FILE_ATTRIBUTE_SOMEHTING win32 api constants defined for Rapi doesn't help you anything at all for re-using those constants in another Java lib that also happens to use Win32 API under the hood. Get the point? Your constants are always local to RAPI and will always be local to RAPI. No chance for re-using outside RAPI, so why not prefix them with RAPI_FILE_ATTRIBUTE_SOMETHING in order to avoid name clashes with other libs that don't have anything in common with RAPI. That being said, renaming OS to something more akin to Rapi would already help, because in client code you always have to qualify constants by the defining class or interface name anyways. If I don't know anything about a client code that I'm reading, reading Rapi.FILE_ATTRIBUTE_READ_ONLY would help me more getting the meaning than OS.FILE_ATTRIBUTE_READ_ONLY since OS is too generic. Renaming it to OSWindows or WindowsOS or WindowsPlatform.FILE_ATTRIBUTE_READ_ONLY would help here, but I'd be more in favor of Rapi because you cannot mix constants from different libs anyways and Rapi shows where they belong. Thanks for your explanation of the abstract classes, this absolutely makes sense.
Thanks for the comprehensive explanation, I understand your concerns now and I created bug 225874 to track this API change.
Thanks. As for the concrete enhancement covered on this bug, I will wait for new patches changing "Date" into "long" as well as added Javadocs and Javadoc API Tooling tags, as per comment #3 before I commit the contribution.
Note that this will be an API change since it changes "Date" into "long". Would be cool getting it in for 3.0M6, which has been postponed till this friday.
I am working on it and I hope to get it done today or tomorrow. Martin, would you like to include your name in the contribution list? Your comments are quite helpful for making the API better.
No thanks - I'll get myself added in case I contribute any actual code, which I haven't so far.
Created attachment 95112 [details] Support for setting file attributes and times(using longs) The API is updated to use long instead of Date for specifying the time.
Created attachment 95113 [details] jrapi.dll (using longs) jrapi.dll updated against the latest patch
Patch looks good, just in the .h file the copyright header is duplicated. No prob, I can get rid of this when committing. I'm surprised that the new jrapi.dll (using longs) is 15.5 KB whereas the old version (using Date) was 15.0 KB -- do you have any clue why the variant that should be simpler is actually slightly larger?
The old version was using longs in the native library (Date was used on the java side), the new version is using long[] in the native in order to have one native function for setting times and pass "null" values for times I don't want to set (like creation time and last access time).
Ok... note that you could also a long[] in order to get all types of file times with one native call, if you wanted to do that -- you can pass in the empty long[] array and have its elements filled by the native code
Correct, it is not a big deal but I can change it if you want.
Looking at the code again, I really cannot understand why you moved from "long" arguments to "long[]". Is it just such that you can express the timestamp with value "0"? It seems to be a waste of size and cpu cycles to me. Due to your timestamp computing formula, the actual date that evaluates to "0" is -OS.TIME_DIFF which computes to -11644473600000 : 1 Jan 1601 00:00:00 GMT are you really afraid of passing this value in? You could also pass in -1, which computes to -11644473600001 : 31 Dec 1600 23:59:59 GMT which is an invalid date in the WinCE filesystem since the long2FILETIME appears to assume unsigned long.
PS you should probably indicated the allowed minimum and maximum values for Windows CE file times is IRapiSession#setFile*Time() methods: WinCE Long.MAX_VALUE == 910692730085477 : 14 Sep 30828 02:48:05 GMT WinCE 0 == -11644473600000 : 1 Jan 1601 00:00:00 GMT I'm assuming that any value outside this bounds should throw a RapiException, although it might be worth testing what happens (Java long does overflow from Long.MAX_VALUE + 1 to Long.MIN_VALUE, so you could express dates after 14-Sep-30828 if you want... an interesting Unit test to check).
I don't like using special long values for implementing special cases. Using long[] and null values is much clear. How much is the difference between using long and long[1] on a desktop PC? I guess not enough that I should care about. The maximum value of setFileLastWriteTime should be specified, it is less than Long.MAX_VALUE because the corresponding getFileLastWriteTime/FILETIME2jlong will overflow and return negative value.
Well that's what I said and why I'm in favor of documenting it: long maxLong = (Long.MAX_VALUE / 10000L) - OS.TIME_DIFF; System.out.println(maxLong + " : " + new Date(maxLong).toGMTString()); long minLong = -OS.TIME_DIFF; System.out.println(minLong + " : " + new Date(minLong).toGMTString()); gives 910692730085477 : 14 Sep 30828 02:48:05 GMT -11644473600000 : 1 Jan 1601 00:00:00 GMT actually, I guess the min and max values are probably WinCE system dependent so I'm not sure if it's necessary documenting them; but you should document in the API Docs what happens in case of passing an invalid value, such as Long.MAX_VALUE for instance. Maybe the overflow doesn't hurt, I don't know. I personally think that -1 is a good value on the back-end for the special "don't set" case ... but anyways, it's your code and if you prefer long[] even though I don't think it's necessary, feel free to have it.
PS note that negative values are OK for the "long timestamp" parameter -- it's milliseconds since the Jan.1,1970 which may also be negative as my little code sample proves.
Ok, lets say we spec setFileLastWriteTime to accept values from -OS.TIME to (Long.MAX_VALUE/10000L)-OS.TIME_DIFF which represents the time from 1601 to 30828. What about getFile*Time? There are certain values which cannot be represented with signed long (even after converted to epoch 1970). I suggest to throw RapiException if the value returned from the native function (CeGetFileTime) is negative, i.e. overflowed.
Wouldn't do that. Throwing an exception somehow implies that user did something wrong. It's not the user's fault if some odd timestamps are on the CE device. Note that overflowing the long will result in negative long values, thus dates before year 1600 -- which is not really an error. You'd really have to try it out what the native device does for years after 30828 . It's even possible that you could "fix" it by doing if (timestamp < MIN_VAL) { //overflowed when reading FILEDATE from CE timestamp = 0-timestamp+1; //wrap to positive }
When setting the timestamp, you can of course throw an IllegalArgumentException or RapiException for timestamp values outside valid range.
I made some tests on a real device with WM6 and it turns out that it supports file times between 1985 and 2108. Passing larger/smaller values overflows the range. My conclusion is that we don't have to describe such things in the javadoc or try to do anything for avoiding overflows or whatever. This is what java.io.File, Win32 APIs and RAPI do. All we need to say in the javadoc is what epoch is expected and truncation is possible for setFileTime. Anything else is platform dependent.
(In reply to comment #28) > Passing larger/smaller values overflows the range. So, what happens when the range overflows? Is an exception thrown or does it just silently truncate? That's the kind of question I'd like to see answered in the Javadocs, I'm not so much interested about the concrete value range.
For me the patch is good, so I committed it: [225727][wince][api] Add support for setting file attributes and file times 180 lines of code, thanks for the patch and the good discussions around it. Renaming OS.java to Rapi.java or similar will be covered in bug 225874.
> So, what happens when the range overflows? Is an exception thrown or does it > just silently truncate? That's the kind of question I'd like to see answered in > the Javadocs, I'm not so much interested about the concrete value range. > It is silently truncated but I don't think we should state this in the javadoc. On Windows java.io.File#setLastModified() also truncates but it is not mention in the javadoc.
Hm... in general, I'm in favor of documenting stuff that I found out. You invested some time now to find out what happens for arguments that are too large to fit; why not document it in order to save somebody else some time. Also note that java.io.File is slightly different in that it needs to support any kind of underlying OS; whereas your IRapiSession#setLastModified() needs to work on RAPI only. You'll probably argue that CeSetFileTime() also doesn't document this; but IMHO this is just a flaw in the documentation and if it's intentional in order to be more flexible in the future it's a bad intention. The better an API is specified what it does, the less time one needs to spend debugging it and figuring out issues - that's my experience.
