nsIFilePicker.files returns returnCancel in mode modeOpenMultiple if result is > 4k see http://bugzilla.mozilla.org/show_bug.cgi?id=172001#c10 for how to fix it. thanks to firstname.lastname@example.org for the info.
14 years ago
Copied the URL from bug 172001 comment 10. Note that I would prefer G/SetWindowLong(... DWL_USER ...) to G/SetProp().
Neil: yup, seconded. Just a note: We'll want to pass in OFN_ENABLESIZING as well as OFN_ENABLEHOOK, since the latter disables the former by default.
Here's a first attempt at a patch. I've tested it locally (on W2K), and it works fine, as far as I can tell. Every time the selection changes in the filepicker, it recalculates what the length of the buffer would be, and reallocates the caller's buffer if necessary. I've tested the Unicode path, but I'm not able to test the ANSI path, since I don't have W95/98/etc. Note that you *can't* test the ANSI path on a Unicode-based system (WNT/2k/etc), as GetOpenFilenameA is implemented as an ANSI->Unicode 'thunk' function, and won't reallocate the caller's buffer (like what we now do in nsToolkit). The MSKB article alludes to this in its last paragraphs, though it's far from clear. This patch doesn't handle failure from nsMemory::Realloc, though I don't know what the correct behaviour would be in that case. Neil: I tried setting DWL_USER, but that caused a crash deep in Windows' DLLs, so I guess the common dialog uses it. I plumped for GWL_USERDATA instead, which should also be free for our use.
Neil: Before sspitzer reviews this patch properly, could you take a look and check there's nothing significantly wrong with it?
I think PR_Malloc and PR_FREEIF are more "null-friendly" functions to use. Possibly at line 288 of the old nsToolkit.cpp the "Ansification" routine could substitute the hook procedure. Also, I think that at line 314 of the old nsToolkit.cpp you should always be able to reallocate the unicode buffer (rather than anywhere else). Check for consistent style e.g. space between if ( A -p on your diff would have been nice. Also, you mistyped ;; on one line.
Comment on attachment 130982 [details] [diff] [review] Patch v1 Cancelling review request. Neil, thanks for the review - I might get back to you in more detail when I'm nearer a compiler. I was originally trying to avoid increasing the coupling between nsFilePicker and nsToolkit (hence the reason that nsFilePicker chooses the hook procedure), though I later realised that nsToolkit's open/save dialog procedures aren't called by anything other than nsFilePicker. Perhaps I should just move the relevant routines out of nsToolkit and into nsFilePicker - the GetOpenFileName and GetSaveFileName functions in nsToolkit really have nothing to do with the rest of nsToolkit, which is all about message pump management. Otherwise, the Unicode hook will be in nsFilePicker and the ANSI hook will be in nsToolkit, which seems crazy. Updated patch to follow (with -p this time - I wasn't aware of that option).
Created attachment 131212 [details] [diff] [review] Patrch v2 Same as the last patch, with the filepicker routines from nsToolkit moved into nsFilePicker, the nsToolkit ConvertAtoW / WtoA functions exposed as static public members, and Neil's comments (comment 6) addressed.
Comment on attachment 131212 [details] [diff] [review] Patrch v2 Neil, care to review this version?
Comment on attachment 131212 [details] [diff] [review] Patrch v2 OK, so I see that this version creates more issues than it resolves. Perhaps you could put both hook procedures in nsToolkit? Your moved reallocation of the unicode buffer uses the wrong size, the new aFileNameW->nMaxFile should now be lenW+2, also I just noticed, shouldn't there be more alloc failure checking? [I don't like that while loop to calculate lenA but I can't improve it except by replacing lenA+1 with ++lenA]
Neil: I could move the hook procedures to nsToolkit, but I thought that it was better to move the filepicker routines to nsFilePicker, as (a) nsToolkit doesn't otherwise have anything to do with the filepicker, and (b) nobody calls the filepicker routines in nsToolkit apart from nsFilePicker. Placing the hook procedures in nsToolkit would work, but would imply that it was possible for other callers to use the filepicker routines, which isn't strictly true. The reallocation of the Unicode buffer doesn't actually use the wrong size in practice, but there's actually no reason for any of that length computation to be there - I can just remove it, and reallocate the Unicode buffer to be the same size as the resized ANSI buffer. Regarding allocation checking: yes, I agree that there needs to be some, though it's not entirely clear what the correct behaviour should be. I assume that PR_Realloc won't destroy the source buffer if it can't reallocate it? If that's true, then in the hook procedures, I could assign to a temporary and only store the result back to the file buffer if the reallocation succeeded. If there's not enough space available in the buffer, the open/save dialog will exit with an error (like it does now), but it won't cause a crash. In the ANSI 'thunk' procedure (CallOpenSaveFileNameA), we could return a failure if we couldn't reallocate the source Unicode buffer to fit (and again, we should go through a temporary to ensure that a failure doesn't leak the original buffer). Was there anything else that you specifically didn't like? I'm not sure what alternative arrangement would make more sense, but suggestions are very welcome!
> I could move the hook procedures to nsToolkit No, leave that bit how it was in your first patch, I've warmed to it :-) > I can ... allocate the Unicode buffer to be the same size as the ANSI buffer. Can you be sure of that working?
>> I can ... allocate the Unicode buffer to be the same size as the ANSI buffer. > Can you be sure of that working? No. I was assuming that all ANSI characters were in the BMP, and would therefore take only a single UTF-16 character. I'm not so sure that's true, so I'll just calculate the required size and resize the Unicode buffer according to that.
pushing out. I'll try to get some time to clean up the first patch (attachment 130982 [details] [diff] [review]) for 1.6b. This isn't that critical, anyway.
Fixed on trunk by the patch in bug 234946, and probably wontfix for branch.
This bug is not fixed by bug 234946. It is still present in trunk.
Agreed. Sorry about that.
Really fixed this time!