Review Request 121447: Return inode/directory when isDir returns true (kfileitem)

[David Faure]

> On Dec. 11, 2014, 2:27 p.m., Mark Gaiser wrote:
> > src/core/kfileitem.cpp, line 255
> > <https://git.reviewboard.kde.org/r/121447/diff/1/?file=332652#file332652line255>
> >
> >     --> add here
> >     
> >     } else {
> >         // Fix for IO slaves that don't set UDS_MIME_TYPE for a folder.
> >         if (m_fileMode & QT_STAT_MASK) == QT_STAT_DIR) {
> >            m_entry.insert(KIO::UDSEntry::UDS_MIME_TYPE, "inode/directory");
> >            m_mimeType = db.mimeTypeForName("inode/directory");
> >            m_bMimeTypeKnown = true;
> >         }
> >     }
> >     
> >     Not tested! Just written in comment box :)
> >     
> >     I think that's about all you'd need to fix this.
> >     But if this is accaptable is probably up to David to decide.
> >     
> >     I'm also not 100% sure that you catch all cases when readUDSEntry().
> 
> Emmanuel Pescosta wrote:
>     +1
>     
>     I also think that this is better way to fix it.
>     Avoids code duplication and the correct mime type for folders is set a early as possible, so other code can rely on it.
> 
> David Faure wrote:
>     This suggestion would break the case where m_guessedMimeType is set, so it should at least that that one is empty too.
>     
>     Anyhow, the orig patch is fine, since determineMimeType is only called once. KFileItem already takes care of caching the result.
>     And you can see the cache (d->m_mimeType) in currentMimeType() too, so the new code will also only run once.
>     
>     There's no need to insert a UDS_MIME_TYPE entry. KFileItem's whole purpose in life is to encapsulate all these details with a proper API, so as long as it returns a correct mimetype everything's fine.
>     
>     I do agree on one thing though: a small unittest in kfileitemtest would be nice :)
> 
> Mark Gaiser wrote:
>     @David, Right, i missed that mimetype caching part which makes it a one time call to determineMimeType. Still, it seems better to me to move it to an initialization step since you nearly always need to know the mime type as early as possible anyway + you only need to add code in one place (right?).
>     
>     I guess the } else { ... like i said before would become an if right after m_guessedMimeType is set:
>     if (m_guessedMimeType.isEmpty() && m_fileMode & QT_STAT_MASK) == QT_STAT_DIR) {
>        m_mimeType = db.mimeTypeForName("inode/directory");
>        m_bMimeTypeKnown = true;
>     }
> 
> Àlex Fiestas wrote:
>     I will add tests and update the patch, sorry for that.

Mark: I don't agree with "as early as possible". The best time is "on demand". Some code using KFileItem doesn't care about mimetypes at all.

The current code for mimetypes is already on-demand, let's keep it that way.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121447/#review71804
-----------------------------------------------------------


On Dec. 11, 2014, 1:22 p.m., Àlex Fiestas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121447/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2014, 1:22 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> If we know that the item is a dir, return directly the correct mimetype for directories.
> 
> More info of why this is needed at:
> https://git.reviewboard.kde.org/r/120909/
> 
> 
> Diffs
> -----
> 
>   src/core/kfileitem.cpp 6a2cfa5 
> 
> Diff: https://git.reviewboard.kde.org/r/121447/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Àlex Fiestas
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20141222/36fd0bce/attachment.html>