----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/101075/#review2771 -----------------------------------------------------------
This review has been submitted with commit 8ba86c2b6fc6f850cbc93ec59b1de6b31863e60b by David Faure. - Commit On April 20, 2011, 8:50 p.m., Thomas Fischer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/101075/ > ----------------------------------------------------------- > > (Updated April 20, 2011, 8:50 p.m.) > > > Review request for kdelibs. > > > Summary > ------- > > When using mimetypes instead of old-fashioned '*.txt|Text file' filters for > KFileDialog functions, there is no mimetype working like '*|All files'. > Mimetypes 'all/all' and 'all/allfiles' have no globs associated and as such > do not match any filenames. The current solution 'application/octet-stream' > works as an all-files filter, but does not behave nicely in KFileFilterCombo: > including this mimetype will render 'all supported files' to 'all files'. > > The attached patch improves the situation as follows: if the developer adds a > mimetype 'all/allfiles' to his/her list of mime types for filtering in e.g. > KFileDialog::getOpenUrl's second parameter, function setMimeFilter recognizes > the request for an "all files" filter option and adds it to the list of > options in the combobox. This "all files", however, is not added to the "all > support files" option as the 'application/octet-stream' would have been. > Additional minor changes (moving some lines around "+= delim") were made in > the same function for GUI consistency reasons. > > To make the patch and mimetype 'all/allfiles' working (as it has no inherent > glob like '*'), KDirLister has to be modified as well: just like > 'application/octet-stream' can act as a wildcard, so does 'all/allfiles' now. > > Regarding code consistency, I am not sure why 'application/octet-stream' was > used as a filter; this mimetype stands IMHO for binary bulk data instead of > 'any/all files'. Maybe support for application/octet-stream should be marked > as deprecated in this situation. > > If this patch does get accepted, the documentation on setMimeFilter needs to > be adopted to explain the use of 'all/allfiles'. Furthermore, I would suggest > that the documentation should suggest to prefer using mimetypes > ('text/plain') over manual globs ('*.txt|Text file') as it offers better > consistency ('*.txt|Text file' vs '*.txt|Plain Text file'), > internationalization etc. > > > Diffs > ----- > > kfile/kfilefiltercombo.cpp f32a0df > kio/kfile/kfiledialog.h af29a37 > kio/kio/kdirlister.cpp df81dc8 > > Diff: http://git.reviewboard.kde.org/r/101075/diff > > > Testing > ------- > > > Thanks, > > Thomas > >
