<AWT Dev> A patch so multiple selected files can have different directories.

[Artem Ananiev]

On 1/31/2012 8:57 PM, Anthony Petrov wrote:
> 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.

It looks fine. I would explicitly delete the stringCls local ref in the 
toPathAndFilenamesArray() method, however it's not critical, it will be 
released by VM later anyway.

Thanks,

Artem

> [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
>>>
>>