KIO::ListJob::entries emits twice for folders with 100+ entries. Why?

[Mark]

On Thu, Nov 29, 2012 at 4:45 PM, Mark <markg85 at gmail.com> wrote:
> On Thu, Nov 29, 2012 at 4:34 PM, David Faure <faure at kde.org> wrote:
>> On Thursday 29 November 2012 16:04:21 Mark wrote:
>>> On Thu, Nov 29, 2012 at 2:23 PM, David Faure <faure at kde.org> wrote:
>>> > On Friday 23 November 2012 22:23:24 Mark wrote:
>>> >> Here it is: http://paste.kde.org/613670/
>>> >
>>> > This calls gettimeofday for every entry, even when this value isn't
>>> > needed.
>>> > Can you move this to the locations where it's actually needed, for
>>> > performance reasons?
>>> >
>>> > Even better would be to port this to QDateTime::currentDateTime or
>>> > QTime::currentTime, to make this more portable (to Windows, without
>>> > kdewin32). Or to use QTime::elapsed, which might be even simpler to use
>>> > for this purpose.
>>> Good one! I will use QTime::elapsed. The reason for moving
>>> gettimeofday to the top is because it was being called "nearly"
>>> always. Unless you had less then 100 entries, then it would only be
>>> called once in the main else statement. However, this change requires
>>> me to know the time difference every time an entry is inserted so i
>>> moved it to the top of the function.
>>
>> Ah, I see.
>> OK (still, with QTime::elapsed this will be a lot more readable and
>> maintainable anyway).
>
> Which it is using at this moment on my computer :)
> Looks a lot cleaner indeed!
>
>>
>>> >> The changes:
>>> >> - It's actually doing batching now regardless of the details level
>>> >> - Batching is time based only (300ms). So if the listing takes longer
>>> >> then 300ms then all entries thus far will be send. That continues till
>>> >> all entries are send.
>>> >
>>> > It continues ... i.e. it waits for another 300ms before sending the second
>>> > batch, right? We don't want to send them one by one, that's for sure.
>>>
>>> In the most negative situation that "might" happen. So if you are on a
>>> very slow network drive that for whatever reason only lists 1 entry
>>> every 300 ms then you would indeed get 1 signal per entry. Please do
>>> note, this is a corner case and "usually" doesn't happen. But i think
>>> broadcasting the entries signal every 300 ms with whatever data is
>>> there should be how it's done. (the current patch)
>>
>> Ah, yes, sure that's fine.
>> I meant in the case of a fast listing, your paragraph above made me wonder if
>> it was going to send everything one by one after the first 300 ms. Now I see
>> this isn't the case.
>>
>>> > 3) What happened to the code that was doing a chdir back to the orig dir?
>>>
>>> Ehh, i dropped it :p I saw no need for that to change back. The folder
>>> is being changed to the one your listing prior to actually listing the
>>> folder so i didn't see a need to put that back. Do you?
>>
>> You should really read the comments in the current code :-)
>> Hint, start at "We restore the path later...".
>
> hehehe oke, i removed the comment ;)
> Without joking, i added the path restore part back.
>
> But why is it doing:
> #if !defined(PATH_MAX) && defined(__GLIBC__)
>     char *path_buffer = ::get_current_dir_name();
>     const KDEPrivate::CharArrayDeleter path_buffer_deleter(path_buffer);
> #else
>     char path_buffer[PATH_MAX];
>     path_buffer[0] = '\0';
>     (void) getcwd(path_buffer, PATH_MAX - 1);
> #endif
>
> for getting the current path? Can't i just do QDir::currentPath() for
> the getting part, store it in a variable and put it back at the end?
>
>>
>> --
>> David Faure, faure at kde.org, http://www.davidfaure.fr
>> Working on KDE, in particular KDE Frameworks 5
>>

patch: https://git.reviewboard.kde.org/r/107520/