D18380: KIO: make file dialog columns resizable again (and movable)

David Faure noreply at phabricator.kde.org
Thu May 9 09:59:32 BST 2019

dfaure requested changes to this revision.
dfaure added a comment.

  I didn't read the full encyclopedia of discussions here, I only looked at the patch.
  But really, this is far too hacky to my taste. If QHeaderview is indeed missing a feature, it might be best to implement it there, it will be much cleaner.


> kdiroperatordetailview.cpp:52
> +        auto pw = view->parentWidget();
> +        for (int pn = 0; pw && pn < 3; ++pn) {
> +            pw = pw->parentWidget();

This is a horrible hack, relying on the exact number of parents before getting to KFileWidget.
Either cast inside a while loop (i.e. "go up until a KFileWidget"), or find a way to pass the proper pointer to this class.

> kdiroperatordetailview.cpp:79
> +
> +    virtual QSize sectionSizeFromContents(int logicalIndex) const
> +    {

use override, not virtual

> kdiroperatordetailview.cpp:185
> +            }
> +            headerView->setSectionResizeMode(0, QHeaderView::Stretch);
> +            headerView->setSectionResizeMode(1, QHeaderView::ResizeToContents);

Why is this (and the following 12 lines or so) done on every resize? Surely that's not needed?

Split this into case Resize and case Polish, I don't see even one line of code that needs to be in both.

> kdiroperatordetailview.cpp:214
> +            break;
> +        case QEvent::Paint:
> +            // event analysis confirms that the 1st paint event arrives after all headers have

URGGH. This smells like a horrible hack. Widgets do layouting, and then painting. Modifying layouting parameters (hiding columns, resizing columns...) at painting time is very wrong and a recipe for infinite loops (paint -> layout -> paint -> layout...).

The comment talks about a "first paint event" - if that's all you need, then at least add a static bool so that this code is run only once. But I still won't like it very much. Isn't that what Polish is for?

> kdiroperatordetailview.cpp:260
> +            if (headerView->m_currentColumnWidth[0] > 0 && headerView->sectionResizeMode(0) != QHeaderView::Interactive) {
> +                qWarning() << "!!!";
> +            }

If you mean Q_ASSERT, use Q_ASSERT. If on the other hand, it can happen for good reasons, remove the qWarning.

> ngraham wrote in kdiroperatordetailview.cpp:296
> This is almost always wrong. Can you explain why you think it's required here?

Yeah, this is correct when defining a class with Q_OBJECT in the cpp file. The moc code needs to see the class definition, and the only way to do that is to include the moc.

  R241 KIO


To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz, markg
Cc: markg, cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20190509/cd2ff913/attachment-0001.html>

More information about the KWrite-Devel mailing list