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

[Méven Car]

meven edited the summary of this revision.

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, LeGast00n, 
cblack, michaelh, ngraham, bruns

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

[René J . V . Bertin]

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

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

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

[René J . V . Bertin]

rjvbb added a comment.


  >   ...aand that's exactly what happened. :(
  
  I'm not exhausted, my patch just works good enough (thanks also to feedback 
on here) to make me more interested in living the rest of my life rather than 
continue to explore alternatives. Maybe if an alternative patch review were 
posted (and mentioned) here I'd get myself to take a look, but maybe we'll only 
get one after this proposition has been committed and tested in the wild.
  
  Give it a couple of days. Then, if no one else reacts, decide what you 
prefer: the current fixed-width columns or my possibly suboptimal 
implementation that makes them resizable again.

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

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

[Nathaniel Graham]

ngraham added a comment.


  In D18380#409963 , @ngraham wrote:
  
  > I hope not, but that's the problem with the wall-of-text communication 
style. Eventually people get exhausted and the discussion peters out with 
nothing getting done.
  
  
  ...aand that's exactly what happened. :(
  
  What needs to get done here to land this?

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

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

[René J . V . Bertin]

rjvbb added a comment.


  >   What's the minimum viable change here?
  
  I think my the implementation of my solution is pretty minimal by now, no? :)
  
  @David Faure: any ideas/suggestions, seems right up your alley?

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

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

[Nathaniel Graham]

ngraham added a comment.


  I hope not, but that's the problem with the wall-of-text communication style. 
Eventually people get exhausted and the discussion peters out with nothing 
getting done.
  
  What's the minimum viable change here?

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

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

[René J . V . Bertin]

rjvbb added a comment.


  So this is just going to become a victim of the better-is-the-enemy-of-good 
principle?
  
  I'm not saying there can be better ways to do this but AFAIC this is already 
pretty good, certainly good enough and even too good in practice to keep me 
from more interesting things to come back and look at other implementations.
  
  So I'm really inclined either to push this in a couple of days in accordance 
with Nate's green light, or else abandon things and keep using my own solution 
until someone else pushes a fix (that suits me just as well).

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

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

[Mark Gaiser]

markg added a comment.


  In D18380#400910 , @rjvbb wrote:
  
  > > But still, isn't there another way? Now the header and view are locked 
together. One doesn't work without the other.
  >
  > What's the problem with that? The custom header class isn't public. I did 
indeed use it for stuff that were not part of a class, or of the 
KDirOperatorDetailView class in earlier iterations. There's no hard reason for 
that, I just find it tidier (and it saves me from having to compile all other 
modules that import the kdiroperatordetail view headerfile when I change a 
detail that doesn't concern them).
  >
  > >   The thing that triggered me to comment wasn't any of that though. It's 
the "narrow mode". I cannot see how that wouldn't be seen as a bug by a user. 
With that little trick you're also overruling any font spacing settings the 
user might have had (in fontconfig) which is quite likely going to cause 
unexpected behavior and therefore bug reports.
  >
  > I'd say make a test case and convince us. You may have missed the fact that 
I'm no longer doing anything to the used font (the same in all columns). I'm 
just using the event flow to let Qt determine column widths using whatever 
information it wants, and then I "fixate" those widths in order to restore 
interactive mode. This is an admittedly complex way to do something that Qt 
doesn't allow us to do: 1) set interactive mode, 2) ask QHeaderView for the 
width that would be used in one of the automatic modes, 3) set those modes 
(with a minimum imposed on the name column).
  
  
  I know. I'm not attacking you here :) I've been in the same annoying 
situation countless of times when wanting to have this very feature. It's a 
pain Qt doesn't give it. But there are ways to solve it and yours is on the 
complicated side.
  
  > I can imagine that someone might file a bug about the name column getting a 
bit larger when resizing. In practice I'm not convinced many users will even 
notice this because it will happen only once, making the column more readable 
(and how much time does one spend resizing side-bars anyway). I think this is a 
bridge to take if and when we get there. A possible workaround would be to 
calculate our own minimum width in such a way that we arrive at the width Qt 
later decides to pick for some reason (for the default font at least).
  
  This is a typical "last mile" thingy. Yes, it works. But not as perfect as it 
could be which eventually is going to add up in hundreds of other "last mile" 
things. Each on their own not worth the time to fix. All together giving the 
user a "yep, they still need to work uit some bugs" feeling.
  
  > Something we (= a number of us) need to look into is what happens when you 
resize a view *after* a manual column adjustment, and what should happen in 
that case. Knowing that anything to make the manual adjustment permanent will 
only increase the code complexity.
  
  I'd say the last column will be the victim there. Just enable 
setStretchLastSection(true) (works in my example too) and let a resize be 
handled that way. This results in the view leaving all columns exactly as is 
when resizing except for the last column. The user still has to resize the name 
column to make it bigger/smaller and that very action should be stored and 
restored next time. It saves the hassle of trying to figure out what the user 
intended.
  
  > 
  > 
  >> The size calculation is not perfect
  > 
  > My initial implementation wasn't perfect either (Nate complained how it 
worked in narrow mode) ... and addressing that was what introduced most of the 
complexity.
  > 
  > At this point I have invested enough time and energy in what is essentially 
a behavioural detail. I like doing that kind of thing but I'm not motivated 
enough to start from scratch (I'll consider making little changes but not much 
more).
  
  Your work is well appreciated! It triggered me to look into it as well. 
Together we can probably figure out a perfect solution that does't leave any 
hiccups that the user will consider a bug, however short they might be. And as 
an added bonus might be used as a generic better QHeaderView version.
  
  p.s. This custom header view thing should definitely be a candidate for 
probably KItemViews. Not my version as-is, but the concept of having more 
flexibility in the header view.

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

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

[Mark Gaiser]

markg added a comment.


  In D18380#400903 , @ngraham wrote:
  
  > In D18380#400896 , @markg wrote:
  >
  > > In QHeaderView code, what we want is "QHeaderView::Interactive" [1] 
followed by QHeaderView::resizeSection [2]. Exactly as [1] described! 
QHeaderView merely lacks a convenience function for this, that is what we have 
to implement!
  >
  >
  > Using that alone results in a visual and functional regression of the 
original problem we were trying to fix (that file dialogs don't have a sensible 
default view):
  >
  > F6570515: Regression.png 
  >
  > If we go with this route, I would want to make sure we preserve the current 
behavior of other columns being right-aligned, and also make sure that the Name 
column has a good default width. If we can ensure that, I'm all for it!
  
  
  Lol :)
  It wasn't meant as a direct drop-in replacement for Rene's patch. KIO 
probably sets more on the headerview that isn't applied now (i assume). And if 
it doesn't then some things do need to be set to make it look nice.  It's 
merely meant to demonstrate that you can change the width of a column to 
whatever you desire while keeping the resizable feature and without the need to 
catch events and do magic there.
  
  Anyhow, you basically want a "stretchFirstColumn" while keeping. I named it 
"stretchColumn" but as it is now it only works if one column uses it. You need 
more complex layouting to get that to work if you want to stretch over multiple 
columns. But that's not the case here.
  Here it is. [1: header], [2: source], and a example on how to use it [3]. 
This obviously is far from done, but again only to demonstrate. If you like 
this approach i can put some time in it to make it less sensitive to errors.
  **be aware** to call "stretchColumn" after you show the dialog! Otherwise the 
sizes haven't been calculated yet. If you do exec, you can get around it by 
doing a QTimer::singleShot.
  
  [1] https://p.sc2.nl/H1S-2DomE
  [2] https://p.sc2.nl/ryQ2hwoX4
  [3] https://p.sc2.nl/Bybanvj7V

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

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

[René J . V . Bertin]

rjvbb added a comment.


  > But still, isn't there another way? Now the header and view are locked 
together. One doesn't work without the other.
  
  What's the problem with that? The custom header class isn't public. I did 
indeed use it for stuff that were not part of a class, or of the 
KDirOperatorDetailView class in earlier iterations. There's no hard reason for 
that, I just find it tidier (and it saves me from having to compile all other 
modules that import the kdiroperatordetail view headerfile when I change a 
detail that doesn't concern them).
  
  >   The thing that triggered me to comment wasn't any of that though. It's 
the "narrow mode". I cannot see how that wouldn't be seen as a bug by a user. 
With that little trick you're also overruling any font spacing settings the 
user might have had (in fontconfig) which is quite likely going to cause 
unexpected behavior and therefore bug reports.
  
  I'd say make a test case and convince us. You may have missed the fact that 
I'm no longer doing anything to the used font (the same in all columns). I'm 
just using the event flow to let Qt determine column widths using whatever 
information it wants, and then I "fixate" those widths in order to restore 
interactive mode. This is an admittedly complex way to do something that Qt 
doesn't allow us to do: 1) set interactive mode, 2) ask QHeaderView for the 
width that would be used in one of the automatic modes, 3) set those modes 
(with a minimum imposed on the name column).
  
  I can imagine that someone might file a bug about the name column getting a 
bit larger when resizing. In practice I'm not convinced many users will even 
notice this because it will happen only once, making the column more readable 
(and how much time does one spend resizing side-bars anyway). I think this is a 
bridge to take if and when we get there. A possible workaround would be to 
calculate our own minimum width in such a way that we arrive at the width Qt 
later decides to pick for some reason (for the default font at least).
  
  Something we (= a number of us) need to look into is what happens when you 
resize a view *after* a manual column adjustment, and what should happen in 
that case. Knowing that anything to make the manual adjustment permanent will 
only increase the code complexity.
  
  > The size calculation is not perfect
  
  My initial implementation wasn't perfect either (Nate complained how it 
worked in narrow mode) ... and addressing that was what introduced most of the 
complexity.
  
  At this point I have invested enough time and energy in what is essentially a 
behavioural detail. I like doing that kind of thing but I'm not motivated 
enough to start from scratch (I'll consider making little changes but not much 
more).

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

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

[Nathaniel Graham]

ngraham added a comment.


  In D18380#400896 , @markg wrote:
  
  > In QHeaderView code, what we want is "QHeaderView::Interactive" [1] 
followed by QHeaderView::resizeSection [2]. Exactly as [1] described! 
QHeaderView merely lacks a convenience function for this, that is what we have 
to implement!
  
  
  Using that alone results in a visual and functional regression of the 
original problem we were trying to fix (that file dialogs don't have a sensible 
default view):
  
  F6570515: Regression.png 
  
  If we go with this route, I would want to make sure we preserve the current 
behavior of other columns being right-aligned, and also make sure that the Name 
column has a good default width. If we can ensure that, I'm all for it!

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

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

[Mark Gaiser]

markg requested changes to this revision.
markg added a comment.
This revision now requires changes to proceed.


  I too would quite like the to have resizeable columns back.
  But the approach you've chosen looks a little too complicated. Also, the fact 
that KDirOperatorDetailView::event needs to know about your custom QHeaderView 
is a code smell. Sometimes you do indeed need logic like that to make things 
work. But still, isn't there another way? Now the header and view are locked 
together. One doesn't work without the other.
  
  The thing that triggered me to comment wasn't any of that though. It's the 
"narrow mode". I cannot see how that wouldn't be seen as a bug by a user. With 
that little trick you're also overruling any font spacing settings the user 
might have had (in fontconfig) which is quite likely going to cause unexpected 
behavior and therefore bug reports.
  
  Now let's take a step back. What do we really want? We want:
  
  - Have the first column resizable and calculate the optimal size when the 
user had not resized the column yet.
  - Restore that setting if the user had manually resized it.
  
  In QHeaderView code, what we want is "QHeaderView::Interactive" [1] followed 
by QHeaderView::resizeSection [2]. Exactly as [1] described! QHeaderView merely 
lacks a convenience function for this, that is what we have to implement!
  I quickly threw this in a test project (minus the state saving) and i ended 
up with this fairly minimal QHeaderView subclass [3: header], [4: source] and 
this sample to see how you use it [5]. Note that there is one quirk in there in 
the source. The size calculation is not perfect and will fail on longer 
strings! That likely needs to be extended to take the style and margins into 
account. Anyhow, the goal of this example is to show a different approach with 
a far smaller code footprint and be far easier to debug. Use it as you please :)
  
  [1] http://doc.qt.io/qt-5/qheaderview.html#ResizeMode-enum
  [2] http://doc.qt.io/qt-5/qheaderview.html#resizeSection
  [3] https://p.sc2.nl/r1RJBLsQ4
  [4] https://p.sc2.nl/BJlMSLimN
  [5] https://p.sc2.nl/SJIYI8jQV

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

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 50205.
rjvbb added a comment.


  Use qobject_cast.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18380?vs=50084=50205

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

AFFECTED FILES
  src/filewidgets/kdiroperatordetailview.cpp

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R241 KIO.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  >   As far as I know, using qobject_cast is faster than comparing class 
names, because it only compares metaclass pointers. Additionally, it allows 
subclasses.
  
  Purely academic: that would be true for an ObjC-like construct like  
`isKindOfClass` but doesn't it qobject_cast also  do additional work as part of 
the casting?
  Allowing subclasses could be good here, but it could also defeat the purpose, 
that'd depend on what the subclass changes w.r.t. KFileWidget.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

[Christoph Feck]

cfeck added a comment.


  As far as I know, using qobject_cast is faster than comparing class names, 
because it only compares metaclass pointers. Additionally, it allows subclasses.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

[René J . V . Bertin]

rjvbb marked an inline comment as done.
rjvbb added inline comments.

INLINE COMMENTS

> cfeck wrote in kdiroperatordetailview.cpp:54
> Can we use qobject_cast here?

Doh, of course.

It's probably more expensive (which shouldn't matter here) but also means we 
have to include `kfilewidget.h`. Is that OK?

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> kdiroperatordetailview.cpp:54
> +}
> +m_isFileWidget = pw ? strcmp(pw->metaObject()->className(), 
> "KFileWidget") == 0 : false;
> +// install the section resize handler

Can we use qobject_cast here?

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: cfeck, dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, 
bruns

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

[Dominik Haumann]

dhaumann added a comment.


  I'm not against this change, but have the feeling another +2 from someone who 
know this stuff is a good idea.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: dhaumann, kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  David, Andreas, any idea why the name column all of a sudden jumps to a 
larger width when the widget is used in a side-bar and you're making the view 
narrower and approach the minimum width? It works in our favour here because 
the end result is that the name column becomes about as wide as the view itself 
(and I ensure it won't change size again).
  It just nags me a bit that I haven't been able to figure out why it happens...

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added reviewers: dfaure, ahartmetz.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol, dfaure, ahartmetz
Cc: kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added reviewers: Dolphin, apol.
ngraham added a comment.
This revision is now accepted and ready to land.


  In D18380#398301 , @rjvbb wrote:
  
  > > The behavior is better now, thanks.
  >
  > It's back to what you liked before I started tinkering with font squeezing 
(plus a few fixes to the behaviour in side-bars).
  
  
  Much nicer. I like it! Thanks for the explanation regarding the moc change. 
Seems sane. Code overall seems sane, but please wait for a more in-depth review 
from someone else before committing.
  
  > Do you know of other applications that use this widget/mode for/in a 
filebrowser side-bar thingy or otherwise in situations where it might have 
unexpected behaviour? Neither Dolphin nor KDevelop seem to use.
  
  Check this out: 
https://lxr.kde.org/search?_filestring=&_string=KDirOperator&_casesensitive=1
  
  A few others I know off the top of my head are Okteta and K3B.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks, #dolphin, apol
Cc: kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a reviewer: Frameworks.
rjvbb added a subscriber: kwrite-devel.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham, #frameworks
Cc: kwrite-devel, kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  > The behavior is better now, thanks.
  
  It's back to what you liked before I started tinkering with font squeezing 
(plus a few fixes to the behaviour in side-bars).
  
  Do you know of other applications that use this widget/mode for/in a 
filebrowser side-bar thingy or otherwise in situations where it might have 
unexpected behaviour? Neither Dolphin nor KDevelop seem to use.

INLINE COMMENTS

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

I didn't think anything, I did what an error message told me.

When I remove the include it comes back:

  [ 78%] Automatic MOC for target KF5KIOFileWidgets
  cd /path/to/build/src/filewidgets && /opt/local/bin/cmake -E cmake_autogen 
/path/to/build/src/filewidgets/CMakeFiles/KF5KIOFileWidgets_autogen.dir/AutogenInfo.cmake
 MacPorts
  AutoMoc error
  -
"/path/to/kio-git/src/filewidgets/kdiroperatordetailview.cpp"
  The file contains a Q_OBJECT macro, but does not include 
"kdiroperatordetailview.moc"!
  Consider to
   - add #include "kdiroperatordetailview.moc"
   - enable SKIP_AUTOMOC for this file
  
  make[2]: *** [src/filewidgets/CMakeFiles/KF5KIOFileWidgets_autogen] Error 1

It must be due to the fact that I put the entire KDirHeaderView class 
definition into the implementation file, which I did because it's really 
private and changes to it shouldn't trigger a rebuild  of a large number of 
files that are not concerned by those changes at all 
(kdiroperatordetailview_p.h is included by quite a few files).

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  The behavior is better now, thanks. I think it will be sufficient to fix the 
bug and not generate user complaints about anything!
  
  I'll let someone else do the code review. Maybe someone from #frameworks 
 or #dolphin 
? That said, one thing sticks out at 
me:

INLINE COMMENTS

> kdiroperatordetailview.cpp:296
> +
> +#include "kdiroperatordetailview.moc"

This is almost always wrong. Can you explain why you think it's required here?

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R241 KIO.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 50084.
rjvbb added a comment.


  the change to the headerfile was now redundant.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18380?vs=50083=50084

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

AFFECTED FILES
  src/filewidgets/kdiroperatordetailview.cpp

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R241 KIO.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 50083.
rjvbb added a comment.


  Well, that was "interesting".
  
  It turns out that Qt has what looks like another path through which section 
sizes are calculated and through which they're shown, which apparently isn't 
used in file dialogs but which sometimes gets used when the detail view widget 
is used in a (Kate) side-bar. That path could give (much) smaller name column 
sizes than the designated minimum but also somewhat larger sizes. All this led 
to the "jarring effect" seen in Nate's video, which did not have anything to do 
with the font  being squeezed.
  
  I've been able to catch the smaller sizes by subclassing `QHeaderView` and 
overriding `sectionSizeFromContents()` and by calling 
`QTreeView::resizeColumnToContents(0)` when resizing; when a larger name column 
width is determine while in "narrow mode" that value now becomes the designated 
minimum so that the column doesn't "hesitate" between the 2 values (more than 
once).
  Calling `QTreeView::resizeColumnToContents` can reduce resizing reactivity 
noticeably so the call is omitted when the widget is used in a file dialog
  
  The font squeezing feature has been dropped.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18380?vs=50032=50083

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

AFFECTED FILES
  src/filewidgets/kdiroperatordetailview.cpp
  src/filewidgets/kdiroperatordetailview_p.h

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  > I'm still going to try to fix this
  
  Good thing I did (am doing), because what you call the jarring exists also 
without font stretching. It's something in Qt that somehow doesn't occur in 
file dialogs but only with applications of the widget like in Kate. It is 
caused by the name column being set to a sort of minimum size, for now beyond 
my control.
  
  I now have it to the point where the state only occurs transiently when you 
resize the Kate sidebar. That's still annoying so I'm looking into what can be 
done through a custom subclass of QHeaderView. There's only 1 virtual method I 
can override and have called by QTreeView but that may be exactly the one we 
need.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  Yikes, and I can reproduce that. Did you notice this with previous versions 
that used font stretch? (Probably not if stretch had no effect for the font(s) 
you tried it with...)
  
  I'm still going to try to fix this; even if in the end it doesn't go in 
there's be a properly working version on record here.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  Sorry, I really don't think this text squeezing feature is actually 
desirable. Here's how it works in Kate:
  
  F6561155: squeezed-2019-01-21_16.03.33.webm 

  
  The transitions are jarring and seem to happen at arbitrary points. The 
result is really unattractive. And the space gained by all of this is 
practically nothing. Let's forget about squeezing the text and ship without it. 
No need to overcomplicate things when we already have a good solution! :)

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 50032.
rjvbb added a comment.


  This version uses letterspacing rather than font stretch, so works regardless 
of whether a stylename has been set on the font.
  
  Letterspacing reduction has a stronger impact on readability than stretch (if 
you cannot know the average default letter spacing) so there cannot be as much 
gain from it.

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18380?vs=49947=50032

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

AFFECTED FILES
  src/filewidgets/kdiroperatordetailview.cpp
  src/filewidgets/kdiroperatordetailview_p.h

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R241 KIO.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  > The discussion in that bug kind of petered out, unfortunately.
  
  It did, and indeed. I thought a fix had been implemented for KFontRequester, 
but either I'm mistaken, or I have a fix locally.
  
  > I think you're the only one who understands what's going on there, so would 
you care to have another go at fixing it?
  
  I seem to recall that some people (the Plasma guys?) thought that the 
stylename should be specified. At least my fix to plasma-integration (D9070 
) didn't remove the stylenames from the 
DefaultFontData table. That's how I realised what was happening here: I forced 
a default font config by removing all the font configs from my kdeglobals, and 
thus got the fonts from that table - styleNames included.
  I
  
  > 
  > 
  >   Alternatively, we could ship the part of this patch that doesn't depend 
on font squeezing, and then ship that part later once 378523 is fixed.
  
  I'm close (hopefully) to having a working alternative based on letterspacing. 
That property is not affected by stylenames and shouldn't be affected by 
FontConfig settings either (space between letters is more a text rendering than 
a font property).

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  If this code depends on https://bugs.kde.org/show_bug.cgi?id=378523 being 
fixed, then we need to fix that first. The discussion in that bug kind of 
petered out, unfortunately. I think you're the only one who understands what's 
going on there, so would you care to have another go at fixing it?
  
  Alternatively, we could ship the part of this patch that doesn't depend on 
font squeezing, and then ship that part later once 378523 is fixed.

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  Oh sh@@t, I understand what's happening here.
  
  The default look-and-feel still appends the Regular stylename to the font, 
and since font stretch is a programmatic way to set a font style ("condensed") 
this too is overridden when a stylename is set.
  
  That does NOT apply to letterspacing.
  
  Noto Sans, left 83% stretch, right 88% letterspacing (same width reduction):
  F6559654: noto-stretch-vs-lspacing.png 
  
  Seeing this now it does look more like what I see in the Mac GUI, and 
probably looks less different than a condensed font. I'll check this in the 
code later today.

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  >   I don't see the squeezed text when using Breeze and Noto Sans. Am I 
missing something?
  
  No, I don't think so. This must correspond to the 2nd pair of images I 
attached.
  
  >   I kind of hope so, because I predict that if we ship with text that gets 
squeezed at various sizes, we'll totally get bug reports about it. :-)
  
  No, this is mostly a font-specific thing; Noto Sans apparently doesn't 
stretch (nor squeeze) at typical UI sizes.
  And do you know of examples where this widget does NOT use the generic text 
font for the directory list? IOW, what are the chances that a single user can 
get instances of this widget where the font doesn't stretch and other instances 
where the font does stretch (because of being at another size or family)? Would 
they even notice?
  
  I kind of agree with the principle of not complicating things overly, but I 
also kind of like the effect I created here. Who could we add to this review to 
get some more opinions?
  
  I have a little font tinker app at github:RJVB/fontweightissue-qt5 (and a 
plasma-integration patch that adds support for alternative "kdeglobals" 
configurations at 
https://github.com/RJVB/macstrop/blob/master/Linux/kf5/kf5-plasma-integration/files/patch-support-alt-config.diff
 ;) )
  
  FWIW, there's also a possibility to reduce letter spacing which can help 
here, but there the margin for error is a lot smaller.

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  I don't see the squeezed text when using Breeze and Noto Sans. Am I missing 
something?
  
  I kind of hope so, because I predict that if we ship with text that gets 
squeezed at various sizes, we'll totally get bug reports about it. :-) No need 
to overcomplicate things IMHO. The prior state of the patch seemed just about 
perfect to me with the removal of the Size column shrinking.

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  My QtCurve-based theme, "narrow mode" just activated:
  F6558463: kdialog-narrow-mode.png 
  Idem, "narrow mode" just deactivated
  F6558464: kdialog-regular-mode.png 
  
  Default theme, narrow mode just activated
  F6558466: kdialog-narrow-mode-default.png 

  Idem, narrow mode just deactivated
  F6558467: kdialog-regular-mode-default.png 


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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 49947.
rjvbb added a comment.


  Final attempt at being a bit clever :)
  
  Entering "narrow mode" now also sets the font stretch (exitting the mode will 
reset the stretch), the same as the Mac Finder will do. I use  factor 83, which 
corresponds roughly to making a 12pt font as wide as a 10pt font.
  Setting stretch does not seem to have any effect when done in the handler 
slot, so it has to be done together with the width adaptation. That means the 
size and date columns need to be stretched too otherwise they will be too wide; 
I find that condensing them by half as much as the font gives a satisfactory 
result. The gained pixels are added to the name column.
  
  Tested with a range of fonts and font sizes I find this to work pretty well. 
NB: the default UI font doesn't seem to support stretch; the code detect this 
by comparing `QFontMetrics::avCharWidth()` before and after setting the stretch.
  
  The look *is* slightly different with the condensed font, but theoretically 
it's only a quantitative difference ;)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18380?vs=49908=49947

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

AFFECTED FILES
  src/filewidgets/kdiroperatordetailview.cpp
  src/filewidgets/kdiroperatordetailview_p.h

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  One thing we might be able to do is use QFont::setStretch() to make the text 
a bit more compact. I'll follow that thought for a bit.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  I agree that reducing the size column width isn't as easy as I thought. I 
find it takes up more space than necessary when horizontal space is at a 
premium and I was hoping to get a more compact read-out that can still be 
interpreted ... and that would show itself in full in a tooltip.
  
  Maybe something can be done at a higher level, just like some systems will 
switch from full to compact date format when available space requires it (Mac 
OS will even switch to a narrow font). I'm not very motivated though to spend 
much time in figuring out how and where to do such a thing so if it's not 
straightforward I'll be leaving that approach to someone else.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  This version works pretty well!
  
  I'm not sure I like the bit where you make the Size column 20% smaller under 
certain circumstances though. Since the Size column was previously sized 
perfectly, making it 20% smaller makes all of its items get elided and become 
useless. Removing that bit makes the behavior feel better to me since the view 
just gets a scrollbar a bit sooner, which I think it a nicer behavior than 
making the Size column useless.
  
  With that change, I'd be pretty close to a "ship it!"

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  >   If there's still enough room to show everything without a horizontal 
scrollbar, then we're still good.
  
  No, resizing a view so that it becomes taller than wide shouldn't make it 
behave as if it's become very narrow all of a sudden. Similarly, a very narrow 
view isn't always taller than it's high (you could have a very small file 
side-bar in a dolphin window, for instance).
  
  I've uploaded a new version which has what I think is probably the best and 
easiest option.
  
  >   It is a very significant real-life issue for vertical sidebars
  
  Yes, of course, I didn't realise those use the same widget.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb set the repository for this revision to R241 KIO.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb updated this revision to Diff 49908.
rjvbb added a comment.


  tentative fix for "very narrow views".
  
  This imposes a simple minimum width on the name column. The value is taken 
from (33% wider than) the date column which has a useful width (which is 
already based on the current font). An additional compensation is applied to 
the size column which is made 20% narrower. This may not be entirely necessary; 
as an alternative we could suppress it completely for this kind of view?
  (It would also be useful if the tooltips were column specific!)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18380?vs=49869=49908

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

AFFECTED FILES
  src/filewidgets/kdiroperatordetailview.cpp
  src/filewidgets/kdiroperatordetailview_p.h

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  In D18380#396578 , @rjvbb wrote:
  
  > > Maybe we can incorporate some more intelligence here
  >
  > Same commit? It is a separate issue in a sense, no - you say "it *still* 
starts" so not a regression I introduced?
  
  
  Well, I would say this patch doesn't actually resolve the issue yet. Making 
the headers manually resizable again is good, but providing a good default size 
so that users don't have to is better. Without that, we'll still get bug 
reports about the bad default size in vertical sidebars (e.g. Kate's filesystem 
browser plugin)
  
  > Danger! What happens if you maximise the dialog vertically (I have WM 
shortcuts for that and use them often)?! I think that no one would expect that 
the name column starts behaving differently all of a sudden when you do that.
  
  If there's still enough room to show everything without a horizontal 
scrollbar, then we're still good. If not, then it should start showing a 
horizontal scrollbar and pick a reasonable-ish width for the name column.
  
  > The problem here is that as far as I can tell we cannot ask Qt to calculate 
the column outside of the normal displaying loop, and the dialog doesn't help 
by not adding all items at once. Maybe a minimum width can be set (during the 
auto-sizing phase, to be lifted when interactive mode is enabled)?
  > 
  > How "very narrow" are we talking about, and to what extent is this a 
real-life issue?
  
  It is a very significant real-life issue for vertical sidebars, such as 
Kate's Filesystem Browser plugin when showing Detailed View or Detailed Tree 
View. Play around with that and see if you can come up with something that 
makes those views look good out of the box with no adjustment needed.
  
  Basically we want for this widget to do display a sane view out-of-the-box 
with the file dialog and vertical sidebars, fall back to a reasonable behavior 
if there still isn't enough room, and finally allow the user to resize the 
headers manually as a last resort.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb added a comment.


  > Maybe we can incorporate some more intelligence here
  
  Same commit? It is a separate issue in a sense, no - you say "it *still* 
starts" so not a regression I introduced?
  
  > and set a sensible default width for the Name column when the height of the 
view is greater than the width.
  
  Danger! What happens if you maximise the dialog vertically (I have WM 
shortcuts for that and use them often)?! I think that no one would expect that 
the name column starts behaving differently all of a sudden when you do that.
  
  The problem here is that as far as I can tell we cannot ask Qt to calculate 
the column outside of the normal displaying loop, and the dialog doesn't help 
by not adding all items at once. Maybe a minimum width can be set (during the 
auto-sizing phase, to be lifted when interactive mode is enabled)?
  
  How "very narrow" are we talking about, and to what extent is this a 
real-life issue? I myself tend not to be amazed when a widget like this gets 
garbled when resized too small. Esp. when it's like here where 2 columns remain 
at the same size and the left most just keeps getting smaller.
  
  Thought: if the width of the name column becomes less than the width of the 
date column, reduce the size of the date and size columns. Question is, by how 
much, and will it work to do that in the resize handler/slot. This can also be 
done just before interactive mode is activated. For instance, reduce the date 
column by 1/3rd, the size column by 50% and add the recuperated pixels to the 
name column width. I'll have a look tomorrow how that behaves.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[Nathaniel Graham]

ngraham added a comment.


  This does indeed make the Name column resizable again (+1) but when the view 
is very narrow, it still starts out with a poor size by default. Maybe we can 
incorporate some more intelligence here and set a sensible default width for 
the Name column when the height of the view is greater than the width.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

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

[René J . V . Bertin]

rjvbb retitled this revision from "KIO: make file dialog columns resizable 
again" to "KIO: make file dialog columns resizable again (and movable)".
rjvbb set the repository for this revision to R241 KIO.

REPOSITORY
  R241 KIO

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

To: rjvbb, ngraham
Cc: kde-frameworks-devel, michaelh, ngraham, bruns