D8367: Hidding place groups implementation in KFilePlacesModel

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> mlaurent wrote in kfileplacesitem.cpp:202
> This one is for avoiding duplicate code as we create in this patch isHidden method.

That was kind of my point, isHidden could have been introduced on its own with the changes needed to remove the duplication in the same commit.

> ervin wrote in kfileplacesmodel.cpp:62-74
> Just make it a const global static then... No need to recreate this hash several times.

Why the intermediate struct? Could be directly the hash as a global static, should make it easier to use you wouldn't need the function below and the extra indirection.

> ervin wrote in kfileplacesmodel.h:55
> Better have the curly brace on the same line than the enum name to be consistent with the other enum just on top.
> 
> Also the enum itself needs to carry over the changes which have been applied to previous commits.

You missed a bit, it looks like it wasn't applied on top of the latest iteration from the other patches the GroupType enum gained some values for its entries.

REVISION DETAIL
  https://phabricator.kde.org/D8367

To: mlaurent, renatoo, ngraham, franckarrecot, ervin
Cc: ngraham, mlaurent, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171031/a66fa21b/attachment.html>