> 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 :)

@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;
}


- Mark


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

_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Reply via email to