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
