Hi Alexander,
Thanks for the update. The fix looks good to me.
Note that myself, I wouldn't change the existing error handling code
(e.g. old LN 224-225 and 231-232 in sun_awt_X11_GtkFileDialogPeer.c),
but I'm still OK with that. Leaving this up to you and other reviewers.
--
best regards,
Anthony
On 6/9/2014 7:07 PM, Alexander Zvegintsev wrote:
Hi Anthony,
thanks for the review, please see the updated webrev:
http://cr.openjdk.java.net/~azvegint/jdk/9/8040007/01/
I assume you've tested this case, and it still works fine, right?
Sure, it works fine. That is why gtk_file_chooser_get_current_folder()
is not used and
isFromSameDirectory() was introduced.
--
Thanks,
Alexander.
On 06/09/2014 06:07 PM, Anthony Petrov wrote:
Hi Alexander,
src/solaris/native/sun/awt/sun_awt_X11_GtkFileDialogPeer.c
176 free(prevDir);
177 prevDir = strdup(dir);
It's unnecessary to re-duplicate the prevDir on each loop iteration
here. I suggest to initialize it once instead. The less pointer
operations, the less room for bugs.
229 (*env)->ExceptionClear(env);
230 JNU_ThrowInternalError(env, "Could not instantiate
current folder");
This error message sounds misleading because the code above doesn't
instantiate a "folder", it tries to instantiate a string. Also, if
exceptions did occur, I believe it's fine to report them as they are.
Otherwise, if the pointer is NULL, then this looks more like an OOM
than an internal error to me.
278 //This is a hack for use with "Recent Folders" in gtk
where each
279 //file could have its own directory.
I assume you've tested this case, and it still works fine, right?
--
best regards,
Anthony
On 6/6/2014 8:00 PM, Alexander Zvegintsev wrote:
Hello,
please review the fix
http://cr.openjdk.java.net/~azvegint/jdk/9/8040007/00/
for the bug
https://bugs.openjdk.java.net/browse/JDK-8040007
With this fix we don't use current folder from native GtkFileChooser
anymore, now we build it by ourselves.
[1]
https://developer.gnome.org/gtk2/stable/GtkFileChooser.html#gtk-file-chooser-get-filenames
[2]
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-path-get-dirname
