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

[David Faure]

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.

INLINE COMMENTS

> 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.

REPOSITORY
  R241 KIO

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

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/kde-frameworks-devel/attachments/20190509/cd2ff913/attachment-0001.html>