D8243: Implement support for categories on KfilesPlacesView

[David Faure]

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

INLINE COMMENTS

> kfileplacesmodeltest.cpp:323
> +    // Trying move the remote at the end of the list, should move it to the end of places section instead
> +    // too keep it grouped
>      KBookmark last = root.first();

too -> to

> kfileplacesitem.cpp:127
> +    default:
> +        Q_ASSERT(false);
> +        break;

Q_UNREACHABLE() exists for this purpose

> kfileplacesview.cpp:108
> +
> +    mutable bool m_dragStarted;
> +

move this bool next to the other one (m_showHoverIndication), to save on padding.

> kfileplacesview.cpp:110
> +
> +    QString groupNameFromIndex(const QModelIndex &index) const;
> +    QModelIndex previousVisibleIndex(const QModelIndex &index) const;

Please move all these methods before the member variables.

This file was doing it correctly, with member vars at the end of the class, I'd like this to be kept.
Otherwise it becomes hard to see all member vars, when they are in the middle of methods.

> kfileplacesview.cpp:168
> +    if (indexIsSectionHeader(index)) {
> +        // if is drawing the floating element used by drag/drop, does not draw the header
> +        if (!m_dragStarted) {

The grammar is a bit strange.
I'd say

  "When drawing"

or

  "If we are drawing"

and later on "do not draw".

> kfileplacesview.cpp:178
> +
> +    if (m_dragStarted) {
> +        m_dragStarted = false;

this if() doesn't seem very useful to me.

> kfileplacesview.cpp:354
> +{
> +    // we only accept drag events starting from item boddy, ignore drag request from header
> +    QModelIndex index = m_view->indexAt(pos);

boddy -> body

> kfileplacesview.cpp:461
> +{
> +    QFontMetrics fm(QApplication::font());
> +    return fm.height();

Use QApplication::fontMetrics() instead, it'll be much faster.

> kfileplacesview.cpp:737
>  
> -    if (index.isValid()) {
> +    const bool clickOverItemArea = !delegate->pointIsHeaderArea(event->pos());
> +

what if we click into an empty space? then it's neither header nor item, right?

If I'm right then the naming here is confusing, at least, and should be

  const bool clickOverHeader = delegate->pointIsHeaderArea(event->pos());
  if (!clickOverHeader && ....)

> kfileplacesview.cpp:1258
> +
> +    for(int i = 0; i < q->model()->rowCount(); i++) {
> +        if (!q->isRowHidden(i)) {

Move rowCount to local variable. This is a non-inline virtual method, no way the compiler can know that it's constant.

> kfileplacesview.cpp:1260
> +        if (!q->isRowHidden(i)) {
> +            QModelIndex index = q->model()->index(i, 0);
> +            const QString sectionName = index.data(KFilePlacesModel::GroupRole).toString();

const

REPOSITORY
  R241 KIO

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

To: renatoo, #frameworks, dfaure, ervin, #vdg
Cc: ngraham, mlaurent, ervin, anthonyfieroni, cfeck, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171018/f0905b01/attachment.html>