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

Thu May 9 11:11:51 BST 2019

rjvbb added a comment.

  >   I didn't read the full encyclopedia of discussions here, I only looked at the patch.

  It's really a pity that you only did that now, because by now I'll have to reverse-engineer my patch (and read the encyclopedia) to remember how I arrived 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.

  Yes, it has a good perfume of a proper hack, I won't disagree with that. But sometimes that's unavoidable if the cleaner solution is by definition a long-term one (that's unlikely to appear in the current Qt LTS version, for instance).

  > This is a horrible hack, relying on the exact number of parents before getting to KFileWidget.

  I'm not sure I agree. IIRC it's based on knowledge of how the standard KIO filedialog is constructed, which seems OK in another part of KIO code.

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

  You'd probably want to put an additional end condition to that loop, no?

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

  If memory serves me well it may not be needed on every loop indeed, but it is also not UNneeded on any loop (i.e. sometimes it is).

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

  Maybe, but apparently not for the last months in my own test kitchen ...

  > Isn't that what Polish is for?

  I am tempted to say that I should have seen polish events arrive at an appropriate time during the mentioned event analysis. I'm not unfamiliar with those thanks to my work on QtCurve, so I'd hope that it would have occurred to me to exploit them.

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

  No, I most definitely didn't mean Q_ASSERT. I won't ever knowingly use anything that leads to a crash/abort if there's even a chance that the situation can be handled more gracefully.

  All in all I notice I have lost much if not all of my appetite to do much more on this beyond the couple of simpler suggestions made here... I haven't even found the time to update my installs from 5.52.0 (and am still on Qt 5.9 too, at that (though with some stuff backported from 5.10).

  Anyone wanting to take over ("commandeer") this patch, please feel free!!

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: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20190509/5b3cd637/attachment.htm>