Hi Matthew,
The updated webrev is available at:
http://cr.openjdk.java.net/~anthony/8-10-GTKFileDialogMultiSelRecentFiles-7132194.1/
Please note that there's still problems with formatting at lines 210-211
(TABs instead of spaces), 239 (missing spaces), and also an extra empty
line at 248. No need to resend a new patch, I'll fix these myself before
pushing the fix.
In the future, please pay attention to formatting your code. At first
this issue may seem unimportant, however, it greatly simplifies
maintaining the code, especially as large as OpenJDK. That's why we
require all fixes to follow the common Java Code Conventions and be
properly formatted. You could use the webrev utility [1] to generate
webreves for your code changes. It is easier to spot mis-formatted code
using a HTML view of your changes.
Having said that, I'm approving the fix. Thank you for the contribution!
If anyone on the mailing list has additional comments, they are welcome.
[1] http://openjdk.java.net/guide/webrevHelp.html
--
best regards,
Anthony
On 1/30/2012 11:51 PM, Matthew Smith wrote:
Anthony
I made the changes suggested and tried it out. Everything seems ok.
Cheers.
mbs
On 01/30/2012 10:08 AM, Anthony Petrov wrote:
Hi Matthew,
I've published a webrev at:
http://cr.openjdk.java.net/~anthony/8-10-GTKFileDialogMultiSelRecentFiles-7132194.0/
I have a few stylistic comments:
1. Lines 187, 235: please avoid using reverse order for conditionals.
I.e. (list == NULL) is preferred to (NULL == list).
2. Line 208: always put the if/else (and while()) statements into
blocks {} (and add a new line after {.)
3. Lines 208, 235,236, 240, 246: please use proper spacing (e.g.. "if
(cond) {", "a = b;", etc.)
4. (this one is less stylistic): at line 208 what prevents us from
using "if (entry[0] == '/')"? This seems more simple and robust than
using strchr() and pointer arithmetics.
Other than that the fix looks fine to me, thank you! Once you fix the
issues above and send us an updated patch, I can push the fix to the
repository.
Comments from other AWT team members (and anyone interested) are welcome!
--
best regards,
Anthony
On 01/24/12 00:50, Matthew Smith wrote:
openjdk/jdk/src/solaris/native/sun/awt/sun_awt_X11_GtkFileDialogPeer.c
This patch is intended to address the issue of newer versions of gtk
where the file dialog lets you select files w/out selecting a directory
via Search or Recently Used. When this happens the 'current directory'
is returned as null, and the Files selected will be returned with the
current working directory appended to the path.
The .java file will show the incorrect behavior if you run it and use
Recently Used to select a file (gnome 3).
This patch does not change the behavior when a directory is selected, or
when the dialog is canceled. If you select a file from the Recently Used
option the directory will be set to the root directory, and the filename
will be the complete path and filename. When multiple files are selected
from a folder the original behavior occurs. If multiple files are
selected from Recently Used then the directory is again set to root, and
all of the fiIes have the complete name.
This work around can be verified with the .java file provided.
Finally this is an updated version of the patch, originally I used
'bool' instead of a 'gboolean' I did compile it and test that it works.
Thank you
mbs
